From mboxrd@z Thu Jan  1 00:00:00 1970
Content-Type: multipart/mixed; boundary="===============8116391638304218248=="
MIME-Version: 1.0
From: Florian Weimer <fweimer@redhat.com>
To: elfutils-devel@lists.fedorahosted.org
Subject: Re: [PATCH v3 4/6] tests/allfcts.c: Install alternate debug
 information
Date: Tue, 22 Apr 2014 09:37:40 +0200
Message-ID: <53561C44.4050006@redhat.com>
In-Reply-To: 1397822324.29199.86.camel@bordewijk.wildebeest.org

--===============8116391638304218248==
Content-Type: text/plain; charset="utf-8"
MIME-Version: 1.0
Content-Transfer-Encoding: quoted-printable

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 =3D dwelf_dwarf_gnu_debugaltlink
>> +    (main, &build_id, &build_id_len);
>> +  if (alt_name =3D=3D NULL)
>> +    return NULL;
>> +  int fd =3D open (alt_name, O_RDONLY);
>> +  if (fd < 0)
>> +    err (1, "open (%s)", alt_name);
>> +  Dwarf *dbg_alt =3D 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 =3D dwarf_getfuncs (die, cb, NULL, doff);
>> +		  if (dwarf_errno () !=3D 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

--===============8116391638304218248==--