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

[Nick Alcock]

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)