On 04/18/2014 01:02 PM, Mark Wielaard wrote:
On Tue, 2014-04-15 at 14:49 +0200, Florian Weimer wrote:
+2014-04-15  Florian Weimer  <[email protected]>
+
+       * dwelf_dwarf_gnu_debugaltlink.c: New file.
+       * Makefile.am (libdwelf_a_SOURCES): Add it.
+       * libdwelf.h (dwelf_dwarf_gnu_debugaltlink): Declare new function.
+       * libdwelfP.h (dwelf_dwarf_gnu_debugaltlink): Add internal
+       declaration.

The code looks fine. But I still like the "tri-state" return more to
indicate why/how things went. Precisely because then you don't have to
do:

+  Elf_Data *data = dwarf->sectiondata[IDX_gnu_debugaltlink];
+  if (data == NULL)
+    {
+      /* Missing data is not actually an error. */
+      __libdw_seterrno (0);
+      return NULL;
+    }
[...]
+/* Returns the name of the alternate debug information file from the
+   .gnu_debugaltlink section if found in the ELF.  On success, writes
+   a pointer to the build ID to *BUILD_IDP, and its length to
+   *BUILD_ID_LENP.  Returns NULL if the ELF file didn't have a
+   .gnu_debuglink section (and sets the dwarf_errcode error code to
+   0), had malformed data in the section or some other error occured.
+   The returned pointers are valid while the ELF handle is valid.  */
+extern const char *dwelf_dwarf_gnu_debugaltlink (Dwarf *dwarf,
+                                                const void **build_idp,
+                                                size_t *build_id_lenp);

I'm not sure if this addresses this completely addresses the issue. A return value of 0 could mean "no debugaltlink data" or "the build ID has zero length".

I don't like setting the error code to zero, but as far as I can tell, the error codes are not exposed, and a caller would have to resort to string comparisons to check if it's the expected "missing section" error, or something about lack of resources or malformed input data.

+2014-04-15  Florian Weimer  <[email protected]>
+
+       * debugaltlink.c, run-debugaltlink.sh: New files.
+       * Makefile.am (check_PROGRAMS): Add debugaltlink.
+       (TESTS): Add run-debugaltlink.sh.
+       (debugaltlink_LDADD): New variable.

Thanks for adding the testcase. run-debugaltlink.sh should also be added
to EXTRA_DIST (I also forgot that in my dwelf_elf_gnu_debuglink patch
(fixed on the mjw/dwelf branch).

Can't we handle this in a better way with make variables, so that the file needs to be listed in just one place?

Anyway, I've fixed it in my copy.

--
Florian Weimer / Red Hat Product Security Team

Reply via email to