Re: [PATCH v2 1/3] libdw: Add dwarf_debugaltlink function

[Florian Weimer <fweimer at redhat dot com>]

On 04/10/2014 06:10 PM, Mark Wielaard wrote:
> Hi Florian,
>
> On Thu, 2014-04-10 at 13:19 +0200, Florian Weimer wrote:
>> +2014-04-10  Florian Weimer  <fweimer@redhat.com>
>> +
>> +	* libdw.h (dwarf_debugaltlink): New function.
>> +	* libdwP.h (IDX_debug_info): Add IDX_gnu_debugaltlink.
>> +	(dwarf_debugaltlink): Internal declaration.
>> +	* Makefile.am (libdw_a_SOURCES): Add dwarf_debugaltlink.c.
>> +	* libdw.map (ELFUTILS_0.159): Export dwarf_debugaltlink.
>> +	* dwarf_debugaltlink.c: New file.
>> +	* dwarf_begin_elf.c (dwarf_scnnames): Add IDX_gnu_debugaltlink
>> +	element.
>> +	(check_section): Call open_debugaltlink.
>
> I like the helper function, but I think it is a little more low-level
> than what we normally provide. We should at least (also) have a
> dwarf_get_alt () that returns a Dwarf *.

This would just return the stored Dwarf *, and not attempt to open any 
files?

> If we provide this low-level function then it would be good to also
> provide a similar one for .gnu_debuglink (which already exists privately
> as find_debuglink in libdwfl) and for the build_id of an Elf file (which
> also exist privately as __libdwfl_find_elf_build_id in libdwfl, but that
> does a bit more by also determining the load address). If we provide
> those helper functions because they might indeed be useful to people
> wanting to write their own Elf/Dwarf file search functions, I am not
> sure how to call them. I don't want people to confuse these lower-level
> helper functions (which expose Elf file details) with the higher-level
> libdw/Dwarf functionality.

We could add them at the ELF layer, but then we'd have to iterate over 
all the ELF section headers in each of these functions.  The section 
header array in libdw avoids this.  I'm also not sure where to put such 
convenience functions.  libebl?  But I currently don't use that, and I 
think some downstreams assign send-class status to it.

>> +int dwarf_debugaltlink (Dwarf *dwarf,
>> +			const char **alt_name,
>> +			const void **build_id,
>> +			size_t *build_id_len)
>> +{
>> +  Elf_Data *data = dwarf->sectiondata[IDX_gnu_debugaltlink];
>> +  if (data == NULL)
>> +    {
>> +      __libdw_seterrno (DWARF_E_NO_ALT_DEBUGLINK);
>> +      return -1;
>> +    }
>> +
>> +  const void *ptr = memchr (data->d_buf, '\0', data->d_size);
>> +  if (ptr == NULL)
>> +    {
>> +      __libdw_seterrno (DWARF_E_INVALID_ELF);
>> +      return -1;
>> +    }
>> +  *alt_name = data->d_buf;
>> +  *build_id = ptr + 1;
>> +  *build_id_len = data->d_size - (ptr - data->d_buf + 1);
>> +  return 0;
>> +}
>
> I would make build_id_len the return value. Then you can return:
> - 0 No debugaltlink found.
> - > 0 Length of build_id, success.
> - < 0 Error.

Hmm, I wonder if the interface is similar enough to read(2) so that the 
tri-state return value isn't a problem.  Usually, it's problematic 
because callers are tempted to treat the result as boolean, interpreting 
-1 as success.

I've picked up the other suggestions.

-- 
Florian Weimer / Red Hat Product Security Team