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