Re: [PATCH] dwarf2read.c: Check type of linkage name attribute prior to decoding

[Doug Evans <dje at google dot com>]

Kevin Buettner writes:
 > On Mon, 3 Aug 2015 16:31:08 -0700
 > Doug Evans <dje@google.com> wrote:
 >
 > > I wonder, though, if this is a good place for using the dwarf  
assembler.
 > > Seems so. We just need a MIPS_linkage_name attribute
 > > that isn't a string. The dwarf assembler test would be a lot
 > > smaller.
 >
 > My updated patch, below, does this.
 >
 > > What if there was a wrapper on dwarf2_attr, dwarf2_string_attr
 > > or some such, and it returned either the attribute (if the attribute
 > > is present *and* is a string) or NULL.
 > > And if the attribute is present but not a string it logs a
 > > complaint (standard bad debug info complaint) and returns NULL.
 >
 > I've introduced the wrapper that you recommend and have used it in
 > all places that made sense to me.  There were a few spots where using
 > it would have made things more complicated, so I left those alone.
 >
 > Here's the updated change / patch:
 >
 > dwarf2read.c: Check type of string valued attributes prior to decoding.
 >
 > This change introduces a new function, dwarf2_string_attr(), which is
 > a wrapper for dwarf2_attr().  dwarf2read.c has been updated to
 > call dwarf2_string_attr in most instances where a string-valued
 > attribute is decoded to produce a string value.  In most cases, it
 > simplifies the code; in some instances, the complexity of the code
 > remains unchanged.
 >
 > I performed this change by looking for instances where the
 > result of DW_STRING was used in an assignment.  Many of these
 > had a pattern which (roughly) looks something like this:
 >
 >   struct attribute *attr = NULL;
 >
 >   attr = dwarf2_attr (die, name, cu);
 >   if (attr != NULL && DW_STRING (attr))
 >     {
 >       const char *str;
 >       ...
 >       str = DW_STRING (attr);
 >       ... /* Use str in some fashion.  */
 >     }
 >
 > Code of this form is transformed to look like this instead:
 >
 >   const char *str;
 >
 >   str = dwarf2_string_attr (die, name, cu)
 >   if (str != NULL)
 >     {
 >        ...
 >        /* Use str in some fashion.  */
 >        ...
 >     }
 >
 > In addition to invoking dwarf2_attr() and DW_STRING(),
 > dwarf2_string_attr() checks to make sure that the attribute's
 > `form' field matches one of DW_FORM_strp, DW_FORM_string, or
 > DW_FORM_GNU_strp_alt.  If it does not match one of these forms,
 > it will return a NULL value in addition to calling complaint().
 >
 > An earlier version of this patch did this type checking for one
 > particular instance where a string attribute was being decoded.
 > The situation that I was attempting to handle in that earlier patch is
 > this:
 >
 > The Texas Instruments compiler uses the encoding for
 > DW_AT_MIPS_linkage_name for other purposes.  TI uses the encoding,
 > 0x2007, for TI_AT_TI_end_line which, unlike DW_AT_MIPS_linkage_name,
 > does not have a string-typed value.  In this instance, GDB was attempting
 > to use an integer value as a string pointer, with predictable results.
 > (GDB would die with a segmentation fault.)
 >
 > I've added a test which reproduces the problem that I was orignally
 > wanting to fix.  It uses DW_AT_MIPS_linkage name with an associate
 > value which is a string, and again, where the value is a small
 > integer.
 >
 > My test case causes GDB to segfault in an unpatched GDB.  There
 > will be two PASSes in a patched GDB.
 >
 > Unpatched GDB:
 >
 > (gdb) ptype f
 > ERROR: Process no longer exists
 > UNRESOLVED: gdb.dwarf2/dw2-bad-mips-linkage-name.exp: ptype f
 > ERROR: Couldn't send ptype g to GDB.
 > UNRESOLVED: gdb.dwarf2/dw2-bad-mips-linkage-name.exp: ptype g
 >
 > Patched GDB:
 >
 > (gdb) ptype f
 > type = bool ()
 > (gdb) PASS: gdb.dwarf2/dw2-bad-mips-linkage-name.exp: ptype f
 > ptype g
 > type = bool ()
 > (gdb) PASS: gdb.dwarf2/dw2-bad-mips-linkage-name.exp: ptype g
 >
 > I see no regressions on an x86_64 native target.
 >
 > gdb/ChangeLog:
 >
 > 	* dwarf2read.c (dwarf2_string_attr): New function.
 > 	(lookup_dwo_unit, process_psymtab_comp_unit_reader)
 > 	(dwarf2_compute_name, dwarf2_physname, find_file_and_directory)
 > 	(read_call_site_scope, namespace_name, guess_full_die_structure_name)
 > 	(anonymous_struct_prefix, prepare_one_comp_unit): Use
 > 	dwarf2_string_attr in place of dwarf2_attr and DW_STRING.
 >
 > gdb/testsuite/ChangeLog:
 >
 > 	* gdb.dwarf2/dw2-bad-mips-linkage-name.c: New file.
 > 	* gdb.dwarf2/dw2-bad-mips-linkage-name.exp: New file.

Thanks for the ping!

LGTM