Re: [PATCH v3 4/6] tests/allfcts.c: Install alternate debug information

[Florian Weimer <fweimer@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2175 bytes --]

On 04/18/2014 01:58 PM, Mark Wielaard wrote:
> On Tue, 2014-04-15 at 16:58 +0200, Florian Weimer wrote:
>> This change also adds more error checking and reporting.
>>
>> +	* allfcts.c (setup_alt): New function.
>> +	(main): Call it.  Implementation additional error checking and
>> +	reporting.
>
>> +static Dwarf *
>> +setup_alt (Dwarf *main)
>> +{
>> +  const void *build_id;
>> +  size_t build_id_len;
>> +  const char *alt_name = dwelf_dwarf_gnu_debugaltlink
>> +    (main, &build_id, &build_id_len);
>> +  if (alt_name == NULL)
>> +    return NULL;
>> +  int fd = open (alt_name, O_RDONLY);
>> +  if (fd < 0)
>> +    err (1, "open (%s)", alt_name);
>> +  Dwarf *dbg_alt = dwarf_begin (fd, DWARF_C_READ);
>> +  close (fd);
>
> I am slightly surprised this actually works. Normally you cannot just
> close the fd of the underlying Dwarf (or Elf). For an Elf you can if you
> force it to have been read completely into memory first by doing
> elf_cntl (elf, ELF_C_FDREAD) as is currently done in try_debugaltlink.

Yes, now I'm surprised as well. :-)

> Which you could do here (on the Elf and then call dwarf_begin_elf) if
> you want to make sure to not leak the fd.

I went this route.

>>   		  doff = dwarf_getfuncs (die, cb, NULL, doff);
>> +		  if (dwarf_errno () != 0)
>> +		    errx (1, "dwarf_getfuncs (%s): %s",
>> +			  argv[i], dwarf_errmsg (-1));
>
> I think this works fine, and the original code did this, but in a less
> convenient way (you wouldn't get any error reported). So keep it as is.
>
> But the way dwarf_getfuncs returns an error state is by returning -1.
> Which isn't really documented and somewhat awkward, so what you do is
> nicer. It is just that theoretically a callback could trigger
> dwarf_errno being set and ignore that as being fine, which might be
> silly, but "legal". You would then pick up a non-fatal dwarf_errno.

I added this check because when I still had an actual error code (not 
zero) for the missing .gnu_debugaltlink section in 
dwelf_dwarf_gnu_debugaltlink, the test case broke in a slightly 
non-obvious way.

-- 
Florian Weimer / Red Hat Product Security Team