[v3,REVIEW,06/33] binutils: readelf: when dumping CTF, load strtab and symtab automatically

Message ID 20190906225520.169680-7-nick.alcock@oracle.com
State Superseded
Headers show
Series
  • CTF linking support
Related show

Commit Message

Nick Alcock Sept. 6, 2019, 10:54 p.m.
We were only loading them when explicitly requested, which leads to
strings that point off into empty space (into the non-loaded "external"
ELF string table).  Avoid this unfortunate consequence by loading the
strtab and symtab by default, unless a blank name is given.

binutils/
	* readelf.c (dump_ctf_symtab_name): Give default value.
	(dump_ctf_strtab_name): Likewise.
	(dump_section_as_ctf): Allow for the null string.
---
 binutils/doc/binutils.texi |  3 ++-
 binutils/readelf.c         | 10 ++++++++--
 2 files changed, 10 insertions(+), 3 deletions(-)

-- 
2.23.0.239.g28aa4420fd

Comments

Alan Modra Sept. 8, 2019, 11:40 p.m. | #1
On Fri, Sep 06, 2019 at 11:54:53PM +0100, Nick Alcock wrote:
> --- a/binutils/readelf.c

> +++ b/binutils/readelf.c

> @@ -13944,7 +13944,13 @@ dump_section_as_ctf (Elf_Internal_Shdr * section, Filedata * filedata)

>    data = get_section_contents (section, filedata);

>    ctfsect.cts_data = data;

>  

> -  if (dump_ctf_symtab_name)

> +  if (!dump_ctf_symtab_name)

> +    dump_ctf_symtab_name = strdup (".symtab");


Please use xstrdup here and elsewhere, fixing the other uses of strdup
you added in a previous patch.

Note that the alternative of testing the result and printing an error
message is more suited to library code where you want to leave the
application some means of recovery.

-- 
Alan Modra
Australia Development Lab, IBM
Nick Alcock Sept. 11, 2019, 8:44 a.m. | #2
On 9 Sep 2019, Alan Modra outgrape:

> On Fri, Sep 06, 2019 at 11:54:53PM +0100, Nick Alcock wrote:

>> --- a/binutils/readelf.c

>> +++ b/binutils/readelf.c

>> @@ -13944,7 +13944,13 @@ dump_section_as_ctf (Elf_Internal_Shdr * section, Filedata * filedata)

>>    data = get_section_contents (section, filedata);

>>    ctfsect.cts_data = data;

>>  

>> -  if (dump_ctf_symtab_name)

>> +  if (!dump_ctf_symtab_name)

>> +    dump_ctf_symtab_name = strdup (".symtab");

>

> Please use xstrdup here and elsewhere, fixing the other uses of strdup

> you added in a previous patch.


I actually used strdup here because readelf had *no* uses of xstrdup in
it, but did use strdup, making me think that perhaps there was some
reason I wasn't getting why strdup was preferred here.

I'll flip them all.

(I can't see any previous-patch uses of strdup. Recently introduced uses
in libctf are divided into two classes: ctf_type_aname_raw, which
specifically wants to return a newly-dupped string or NULL, so strdup is
right, and the ctf-link machinery, which is *not* intended purely for
use from ld and so should use good library philosophy and return errors
rather than dying. However, I have been too cavalier in a few places
there and failed to check strdup() for errors well enough: will fix.)

> Note that the alternative of testing the result and printing an error

> message is more suited to library code where you want to leave the

> application some means of recovery.


Quite so, which is why I feel guilty whenever I cop out and use xstrdup
in libctf :) it does happen, though less often now we have the atoms
table to hang on to strings persistently.

I think I'll decommission ctf_strdup soon: it's getting in the way of
audits like this. ctf_alloc too -- I've been hoping it might be useful
but it's proving to be pure obfuscation and not helpful at all. If I
want to interpose malloc for debugging, there are better ways.

-- 
NULL && (void)
Nick Alcock Sept. 17, 2019, 6:02 a.m. | #3
On 11 Sep 2019, Nick Alcock told this:

> On 9 Sep 2019, Alan Modra outgrape:

>

>> On Fri, Sep 06, 2019 at 11:54:53PM +0100, Nick Alcock wrote:

>>> --- a/binutils/readelf.c

>>> +++ b/binutils/readelf.c

>>> @@ -13944,7 +13944,13 @@ dump_section_as_ctf (Elf_Internal_Shdr * section, Filedata * filedata)

>>>    data = get_section_contents (section, filedata);

>>>    ctfsect.cts_data = data;

>>>  

>>> -  if (dump_ctf_symtab_name)

>>> +  if (!dump_ctf_symtab_name)

>>> +    dump_ctf_symtab_name = strdup (".symtab");

>>

>> Please use xstrdup here and elsewhere, fixing the other uses of strdup

>> you added in a previous patch.

>

> I actually used strdup here because readelf had *no* uses of xstrdup in

> it, but did use strdup, making me think that perhaps there was some

> reason I wasn't getting why strdup was preferred here.


There were several uses where we interned into hashes by calling
strdup *nested in the function call*, which is just great: if strdup
fails we silently end up hashing NULL -> something. NO.

> I think I'll decommission ctf_strdup soon: it's getting in the way of

> audits like this. ctf_alloc too -- I've been hoping it might be useful

> but it's proving to be pure obfuscation and not helpful at all. If I

> want to interpose malloc for debugging, there are better ways.


This is now done and will be in the next series I post for review (not
this series, which is quite long enough). This makes auditing easier and
removes the huge irregular surface of "ctf_malloc or malloc? ctf_free or
free? ctf_strdup or strdup?". They did not match up and their use was
more or less random -- mostly my fault, since when libctf was in Solaris
the usage was fairly consistent(ish), but honestly by this point the
layer of wrappers adds no value.

Fixing that revealed more leaks on error paths, and a refcount leak if
you try to use ctf_import to make one dictionary the parent of another
and then use ctf_import (fp, NULL) to cancel it out again. (Why would
anyone ever do that? Search me, but the code supports it, and we
shouldn't leak the parent dictionary and make it unfreeable if that
happens). Also all fixed in the next series.

-- 
NULL && (void)

Patch

diff --git a/binutils/doc/binutils.texi b/binutils/doc/binutils.texi
index bb37cb09884..a548180fd68 100644
--- a/binutils/doc/binutils.texi
+++ b/binutils/doc/binutils.texi
@@ -4850,7 +4850,8 @@  command to @command{ar}, but without using the BFD library.  @xref{ar}.
 @item --ctf-symbols=@var{section}
 @item --ctf-strings=@var{section}
 Specify the name of another section from which the CTF file can inherit
-strings and symbols.
+strings and symbols.  By default, the @code{.symtab} and its linked
+string table are used.
 
 If either of @option{--ctf-symbols} or @option{--ctf-strings} is specified, the
 other must be specified as well.
diff --git a/binutils/readelf.c b/binutils/readelf.c
index 40957b01a8e..459706805c5 100644
--- a/binutils/readelf.c
+++ b/binutils/readelf.c
@@ -13944,7 +13944,13 @@  dump_section_as_ctf (Elf_Internal_Shdr * section, Filedata * filedata)
   data = get_section_contents (section, filedata);
   ctfsect.cts_data = data;
 
-  if (dump_ctf_symtab_name)
+  if (!dump_ctf_symtab_name)
+    dump_ctf_symtab_name = strdup (".symtab");
+
+  if (!dump_ctf_strtab_name)
+    dump_ctf_strtab_name = strdup (".strtab");
+
+  if (dump_ctf_symtab_name && dump_ctf_symtab_name[0] != 0)
     {
       if ((symtab_sec = find_section (filedata, dump_ctf_symtab_name)) == NULL)
 	{
@@ -13959,7 +13965,7 @@  dump_section_as_ctf (Elf_Internal_Shdr * section, Filedata * filedata)
       symsectp = shdr_to_ctf_sect (&symsect, symtab_sec, filedata);
       symsect.cts_data = symdata;
     }
-  if (dump_ctf_strtab_name)
+  if (dump_ctf_strtab_name && dump_ctf_symtab_name[0] != 0)
     {
       if ((strtab_sec = find_section (filedata, dump_ctf_strtab_name)) == NULL)
 	{