diff mbox

Fix heap-use-after-free in typename_concat

Message ID 20190517214645.GX2568@embecosm.com
State New
Headers show

Commit Message

Andrew Burgess May 17, 2019, 9:46 p.m. UTC
* Tom de Vries <tdevries@suse.de> [2019-05-17 09:40:57 +0200]:

> On 16-05-19 17:37, Andrew Burgess wrote:
> > * Tom de Vries <tdevries@suse.de> [2019-05-03 11:31:26 +0200]:
> 
> > This all sounds good.  I have a couple of small suggestions inline
> > below...
> > 
> >>
> >> ---
> >>  gdb/dwarf2read.c | 49 +++++++++++++++++++++++++++++++++++++++----------
> >>  1 file changed, 39 insertions(+), 10 deletions(-)
> >>
> >> diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
> >> index b0bdecf96f..442b618f6e 100644
> >> --- a/gdb/dwarf2read.c
> >> +++ b/gdb/dwarf2read.c
> >> @@ -1518,8 +1518,14 @@ static unsigned int peek_abbrev_code (bfd *, const gdb_byte *);
> >>  static struct partial_die_info *load_partial_dies
> >>    (const struct die_reader_specs *, const gdb_byte *, int);
> >>  
> >> -static struct partial_die_info *find_partial_die (sect_offset, int,
> >> -						  struct dwarf2_cu *);
> >> +struct cu_partial_die_info
> >> +{
> >> +  struct dwarf2_cu *cu;
> >> +  struct partial_die_info *pdi;
> >> +};
> > 
> > This needs at least a header comment describing its use, and ideally
> > each field documented too.
> > 
> 
> Done.
> 
> > I wonder though if you should also provide this with a 2 argument
> > constructor, and delete the default constructor, like:
> > 
> >   /* blah blah blah...  */
> > 
> >   struct cu_partial_die_info
> >   {
> >     /* mumble.. */
> >     struct dwarf2_cu *cu;
> > 
> >     /* mutter...  */
> >     struct partial_die_info *pdi;
> > 
> >     cu_partial_die_info (struct dwarf2_cu *cu,
> >   		       struct partial_die_info *pdi)
> >       : cu (cu),
> >         pdi (pdi)
> >     { /* Nothing.  */ }
> > 
> >   private:
> >     cu_partial_die_info () = delete;
> >   };
> > 
> > This will lead to some obvious knock on changes in the rest of the
> > code, which I think are probably improvements.
> > 
> 
> I've tried this out, and the only effect was this type of changes:
> ...
> -  struct cu_partial_die_info res;
> +  struct cu_partial_die_info res (NULL, NULL);
> ...
> So, I've left this out for now.

Sorry, I didn't explain myself very well.

> 
> Committed as below.

I plan to apply the patch below to master (once its completed a test
run) unless you have any strong objections.

Thanks,
Andrew

---

[PATCH] gdb: Add constructor to struct cu_partial_die_info

Adds a constructor to 'struct cu_partial_die_info' and disables the
default constructor, preventing partially initialised instances from
being created.

Update 'find_partial_die' to return a const struct.

Users of 'find_partial_die' are updated to take account of the above
two changes.

There should be no user visible changes after this commit.

gdb/ChangeLog:

	* dwarf2read.c (struct cu_partial_die_info): Add constructor,
	delete default constructor.
	(find_partial_die): Update to return const struct.
	(partial_die_parent_scope): Move variable declaration into scope
	of its use and change its type to auto.
	(guess_partial_die_structure_name): Likewise.
	(partial_die_info::fixup): Likewise.
---
 gdb/ChangeLog    | 10 ++++++++++
 gdb/dwarf2read.c | 27 ++++++++++++++++-----------
 2 files changed, 26 insertions(+), 11 deletions(-)

Comments

Andrew Burgess May 18, 2019, 8:51 a.m. UTC | #1
* Andrew Burgess <andrew.burgess@embecosm.com> [2019-05-17 22:46:45 +0100]:

> * Tom de Vries <tdevries@suse.de> [2019-05-17 09:40:57 +0200]:
> 
> > On 16-05-19 17:37, Andrew Burgess wrote:
> > > * Tom de Vries <tdevries@suse.de> [2019-05-03 11:31:26 +0200]:
> > 
> > > This all sounds good.  I have a couple of small suggestions inline
> > > below...
> > > 
> > >>
> > >> ---
> > >>  gdb/dwarf2read.c | 49 +++++++++++++++++++++++++++++++++++++++----------
> > >>  1 file changed, 39 insertions(+), 10 deletions(-)
> > >>
> > >> diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
> > >> index b0bdecf96f..442b618f6e 100644
> > >> --- a/gdb/dwarf2read.c
> > >> +++ b/gdb/dwarf2read.c
> > >> @@ -1518,8 +1518,14 @@ static unsigned int peek_abbrev_code (bfd *, const gdb_byte *);
> > >>  static struct partial_die_info *load_partial_dies
> > >>    (const struct die_reader_specs *, const gdb_byte *, int);
> > >>  
> > >> -static struct partial_die_info *find_partial_die (sect_offset, int,
> > >> -						  struct dwarf2_cu *);
> > >> +struct cu_partial_die_info
> > >> +{
> > >> +  struct dwarf2_cu *cu;
> > >> +  struct partial_die_info *pdi;
> > >> +};
> > > 
> > > This needs at least a header comment describing its use, and ideally
> > > each field documented too.
> > > 
> > 
> > Done.
> > 
> > > I wonder though if you should also provide this with a 2 argument
> > > constructor, and delete the default constructor, like:
> > > 
> > >   /* blah blah blah...  */
> > > 
> > >   struct cu_partial_die_info
> > >   {
> > >     /* mumble.. */
> > >     struct dwarf2_cu *cu;
> > > 
> > >     /* mutter...  */
> > >     struct partial_die_info *pdi;
> > > 
> > >     cu_partial_die_info (struct dwarf2_cu *cu,
> > >   		       struct partial_die_info *pdi)
> > >       : cu (cu),
> > >         pdi (pdi)
> > >     { /* Nothing.  */ }
> > > 
> > >   private:
> > >     cu_partial_die_info () = delete;
> > >   };
> > > 
> > > This will lead to some obvious knock on changes in the rest of the
> > > code, which I think are probably improvements.
> > > 
> > 
> > I've tried this out, and the only effect was this type of changes:
> > ...
> > -  struct cu_partial_die_info res;
> > +  struct cu_partial_die_info res (NULL, NULL);
> > ...
> > So, I've left this out for now.
> 
> Sorry, I didn't explain myself very well.
> 
> > 
> > Committed as below.
> 
> I plan to apply the patch below to master (once its completed a test
> run) unless you have any strong objections.

Passed testing, so pushed.

Thanks,
Andrew


> 
> Thanks,
> Andrew
> 
> ---
> 
> [PATCH] gdb: Add constructor to struct cu_partial_die_info
> 
> Adds a constructor to 'struct cu_partial_die_info' and disables the
> default constructor, preventing partially initialised instances from
> being created.
> 
> Update 'find_partial_die' to return a const struct.
> 
> Users of 'find_partial_die' are updated to take account of the above
> two changes.
> 
> There should be no user visible changes after this commit.
> 
> gdb/ChangeLog:
> 
> 	* dwarf2read.c (struct cu_partial_die_info): Add constructor,
> 	delete default constructor.
> 	(find_partial_die): Update to return const struct.
> 	(partial_die_parent_scope): Move variable declaration into scope
> 	of its use and change its type to auto.
> 	(guess_partial_die_structure_name): Likewise.
> 	(partial_die_info::fixup): Likewise.
> ---
>  gdb/ChangeLog    | 10 ++++++++++
>  gdb/dwarf2read.c | 27 ++++++++++++++++-----------
>  2 files changed, 26 insertions(+), 11 deletions(-)
> 
> diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
> index 004238a6775..f48b931a3f3 100644
> --- a/gdb/dwarf2read.c
> +++ b/gdb/dwarf2read.c
> @@ -1514,10 +1514,18 @@ struct cu_partial_die_info
>    struct dwarf2_cu *cu;
>    /* A partial_die_info.  */
>    struct partial_die_info *pdi;
> +
> +  cu_partial_die_info (struct dwarf2_cu *cu, struct partial_die_info *pdi)
> +    : cu (cu),
> +      pdi (pdi)
> +  { /* Nothhing.  */ }
> +
> +private:
> +  cu_partial_die_info () = delete;
>  };
>  
> -static struct cu_partial_die_info find_partial_die (sect_offset, int,
> -						    struct dwarf2_cu *);
> +static const struct cu_partial_die_info find_partial_die (sect_offset, int,
> +							  struct dwarf2_cu *);
>  
>  static const gdb_byte *read_attribute (const struct die_reader_specs *,
>  				       struct attribute *, struct attr_abbrev *,
> @@ -8763,7 +8771,6 @@ partial_die_parent_scope (struct partial_die_info *pdi,
>  {
>    const char *grandparent_scope;
>    struct partial_die_info *parent, *real_pdi;
> -  struct cu_partial_die_info res;
>  
>    /* We need to look at our parent DIE; if we have a DW_AT_specification,
>       then this means the parent of the specification DIE.  */
> @@ -8771,8 +8778,8 @@ partial_die_parent_scope (struct partial_die_info *pdi,
>    real_pdi = pdi;
>    while (real_pdi->has_specification)
>      {
> -      res = find_partial_die (real_pdi->spec_offset,
> -			      real_pdi->spec_is_dwz, cu);
> +      auto res = find_partial_die (real_pdi->spec_offset,
> +				   real_pdi->spec_is_dwz, cu);
>        real_pdi = res.pdi;
>        cu = res.cu;
>      }
> @@ -18919,7 +18926,7 @@ dwarf2_cu::find_partial_die (sect_offset sect_off)
>     outside their CU (they do however referencing other types via
>     DW_FORM_ref_sig8).  */
>  
> -static struct cu_partial_die_info
> +static const struct cu_partial_die_info
>  find_partial_die (sect_offset sect_off, int offset_in_dwz, struct dwarf2_cu *cu)
>  {
>    struct dwarf2_per_objfile *dwarf2_per_objfile
> @@ -19000,7 +19007,6 @@ guess_partial_die_structure_name (struct partial_die_info *struct_pdi,
>  
>    struct partial_die_info *real_pdi;
>    struct partial_die_info *child_pdi;
> -  struct cu_partial_die_info res;
>  
>    /* If this DIE (this DIE's specification, if any) has a parent, then
>       we should not do this.  We'll prepend the parent's fully qualified
> @@ -19009,8 +19015,8 @@ guess_partial_die_structure_name (struct partial_die_info *struct_pdi,
>    real_pdi = struct_pdi;
>    while (real_pdi->has_specification)
>      {
> -      res = find_partial_die (real_pdi->spec_offset,
> -			      real_pdi->spec_is_dwz, cu);
> +      auto res = find_partial_die (real_pdi->spec_offset,
> +				   real_pdi->spec_is_dwz, cu);
>        real_pdi = res.pdi;
>        cu = res.cu;
>      }
> @@ -19058,9 +19064,8 @@ partial_die_info::fixup (struct dwarf2_cu *cu)
>    if (name == NULL && has_specification)
>      {
>        struct partial_die_info *spec_die;
> -      struct cu_partial_die_info res;
>  
> -      res = find_partial_die (spec_offset, spec_is_dwz, cu);
> +      auto res = find_partial_die (spec_offset, spec_is_dwz, cu);
>        spec_die = res.pdi;
>        cu = res.cu;
>  
> -- 
> 2.14.5
>
Tom de Vries May 21, 2019, 1:08 p.m. UTC | #2
On 17-05-19 23:46, Andrew Burgess wrote:
> * Tom de Vries <tdevries@suse.de> [2019-05-17 09:40:57 +0200]:
> 
>> On 16-05-19 17:37, Andrew Burgess wrote:
>>> * Tom de Vries <tdevries@suse.de> [2019-05-03 11:31:26 +0200]:
>>
>>> This all sounds good.  I have a couple of small suggestions inline
>>> below...
>>>
>>>>
>>>> ---
>>>>  gdb/dwarf2read.c | 49 +++++++++++++++++++++++++++++++++++++++----------
>>>>  1 file changed, 39 insertions(+), 10 deletions(-)
>>>>
>>>> diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
>>>> index b0bdecf96f..442b618f6e 100644
>>>> --- a/gdb/dwarf2read.c
>>>> +++ b/gdb/dwarf2read.c
>>>> @@ -1518,8 +1518,14 @@ static unsigned int peek_abbrev_code (bfd *, const gdb_byte *);
>>>>  static struct partial_die_info *load_partial_dies
>>>>    (const struct die_reader_specs *, const gdb_byte *, int);
>>>>  
>>>> -static struct partial_die_info *find_partial_die (sect_offset, int,
>>>> -						  struct dwarf2_cu *);
>>>> +struct cu_partial_die_info
>>>> +{
>>>> +  struct dwarf2_cu *cu;
>>>> +  struct partial_die_info *pdi;
>>>> +};
>>>
>>> This needs at least a header comment describing its use, and ideally
>>> each field documented too.
>>>
>>
>> Done.
>>
>>> I wonder though if you should also provide this with a 2 argument
>>> constructor, and delete the default constructor, like:
>>>
>>>   /* blah blah blah...  */
>>>
>>>   struct cu_partial_die_info
>>>   {
>>>     /* mumble.. */
>>>     struct dwarf2_cu *cu;
>>>
>>>     /* mutter...  */
>>>     struct partial_die_info *pdi;
>>>
>>>     cu_partial_die_info (struct dwarf2_cu *cu,
>>>   		       struct partial_die_info *pdi)
>>>       : cu (cu),
>>>         pdi (pdi)
>>>     { /* Nothing.  */ }
>>>
>>>   private:
>>>     cu_partial_die_info () = delete;
>>>   };
>>>
>>> This will lead to some obvious knock on changes in the rest of the
>>> code, which I think are probably improvements.
>>>
>>
>> I've tried this out, and the only effect was this type of changes:
>> ...
>> -  struct cu_partial_die_info res;
>> +  struct cu_partial_die_info res (NULL, NULL);
>> ...
>> So, I've left this out for now.
> 
> Sorry, I didn't explain myself very well.
> 

Ah, I see what you meant now. Agreed, that's a nice cleanup.

Thanks,
- Tom

>>
>> Committed as below.
> 
> I plan to apply the patch below to master (once its completed a test
> run) unless you have any strong objections.
> 
> Thanks,
> Andrew
> 
> ---
> 
> [PATCH] gdb: Add constructor to struct cu_partial_die_info
> 
> Adds a constructor to 'struct cu_partial_die_info' and disables the
> default constructor, preventing partially initialised instances from
> being created.
> 
> Update 'find_partial_die' to return a const struct.
> 
> Users of 'find_partial_die' are updated to take account of the above
> two changes.
> 
> There should be no user visible changes after this commit.
> 
> gdb/ChangeLog:
> 
> 	* dwarf2read.c (struct cu_partial_die_info): Add constructor,
> 	delete default constructor.
> 	(find_partial_die): Update to return const struct.
> 	(partial_die_parent_scope): Move variable declaration into scope
> 	of its use and change its type to auto.
> 	(guess_partial_die_structure_name): Likewise.
> 	(partial_die_info::fixup): Likewise.
> ---
>  gdb/ChangeLog    | 10 ++++++++++
>  gdb/dwarf2read.c | 27 ++++++++++++++++-----------
>  2 files changed, 26 insertions(+), 11 deletions(-)
> 
> diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
> index 004238a6775..f48b931a3f3 100644
> --- a/gdb/dwarf2read.c
> +++ b/gdb/dwarf2read.c
> @@ -1514,10 +1514,18 @@ struct cu_partial_die_info
>    struct dwarf2_cu *cu;
>    /* A partial_die_info.  */
>    struct partial_die_info *pdi;
> +
> +  cu_partial_die_info (struct dwarf2_cu *cu, struct partial_die_info *pdi)
> +    : cu (cu),
> +      pdi (pdi)
> +  { /* Nothhing.  */ }
> +
> +private:
> +  cu_partial_die_info () = delete;
>  };
>  
> -static struct cu_partial_die_info find_partial_die (sect_offset, int,
> -						    struct dwarf2_cu *);
> +static const struct cu_partial_die_info find_partial_die (sect_offset, int,
> +							  struct dwarf2_cu *);
>  
>  static const gdb_byte *read_attribute (const struct die_reader_specs *,
>  				       struct attribute *, struct attr_abbrev *,
> @@ -8763,7 +8771,6 @@ partial_die_parent_scope (struct partial_die_info *pdi,
>  {
>    const char *grandparent_scope;
>    struct partial_die_info *parent, *real_pdi;
> -  struct cu_partial_die_info res;
>  
>    /* We need to look at our parent DIE; if we have a DW_AT_specification,
>       then this means the parent of the specification DIE.  */
> @@ -8771,8 +8778,8 @@ partial_die_parent_scope (struct partial_die_info *pdi,
>    real_pdi = pdi;
>    while (real_pdi->has_specification)
>      {
> -      res = find_partial_die (real_pdi->spec_offset,
> -			      real_pdi->spec_is_dwz, cu);
> +      auto res = find_partial_die (real_pdi->spec_offset,
> +				   real_pdi->spec_is_dwz, cu);
>        real_pdi = res.pdi;
>        cu = res.cu;
>      }
> @@ -18919,7 +18926,7 @@ dwarf2_cu::find_partial_die (sect_offset sect_off)
>     outside their CU (they do however referencing other types via
>     DW_FORM_ref_sig8).  */
>  
> -static struct cu_partial_die_info
> +static const struct cu_partial_die_info
>  find_partial_die (sect_offset sect_off, int offset_in_dwz, struct dwarf2_cu *cu)
>  {
>    struct dwarf2_per_objfile *dwarf2_per_objfile
> @@ -19000,7 +19007,6 @@ guess_partial_die_structure_name (struct partial_die_info *struct_pdi,
>  
>    struct partial_die_info *real_pdi;
>    struct partial_die_info *child_pdi;
> -  struct cu_partial_die_info res;
>  
>    /* If this DIE (this DIE's specification, if any) has a parent, then
>       we should not do this.  We'll prepend the parent's fully qualified
> @@ -19009,8 +19015,8 @@ guess_partial_die_structure_name (struct partial_die_info *struct_pdi,
>    real_pdi = struct_pdi;
>    while (real_pdi->has_specification)
>      {
> -      res = find_partial_die (real_pdi->spec_offset,
> -			      real_pdi->spec_is_dwz, cu);
> +      auto res = find_partial_die (real_pdi->spec_offset,
> +				   real_pdi->spec_is_dwz, cu);
>        real_pdi = res.pdi;
>        cu = res.cu;
>      }
> @@ -19058,9 +19064,8 @@ partial_die_info::fixup (struct dwarf2_cu *cu)
>    if (name == NULL && has_specification)
>      {
>        struct partial_die_info *spec_die;
> -      struct cu_partial_die_info res;
>  
> -      res = find_partial_die (spec_offset, spec_is_dwz, cu);
> +      auto res = find_partial_die (spec_offset, spec_is_dwz, cu);
>        spec_die = res.pdi;
>        cu = res.cu;
>  
>
diff mbox

Patch

diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
index 004238a6775..f48b931a3f3 100644
--- a/gdb/dwarf2read.c
+++ b/gdb/dwarf2read.c
@@ -1514,10 +1514,18 @@  struct cu_partial_die_info
   struct dwarf2_cu *cu;
   /* A partial_die_info.  */
   struct partial_die_info *pdi;
+
+  cu_partial_die_info (struct dwarf2_cu *cu, struct partial_die_info *pdi)
+    : cu (cu),
+      pdi (pdi)
+  { /* Nothhing.  */ }
+
+private:
+  cu_partial_die_info () = delete;
 };
 
-static struct cu_partial_die_info find_partial_die (sect_offset, int,
-						    struct dwarf2_cu *);
+static const struct cu_partial_die_info find_partial_die (sect_offset, int,
+							  struct dwarf2_cu *);
 
 static const gdb_byte *read_attribute (const struct die_reader_specs *,
 				       struct attribute *, struct attr_abbrev *,
@@ -8763,7 +8771,6 @@  partial_die_parent_scope (struct partial_die_info *pdi,
 {
   const char *grandparent_scope;
   struct partial_die_info *parent, *real_pdi;
-  struct cu_partial_die_info res;
 
   /* We need to look at our parent DIE; if we have a DW_AT_specification,
      then this means the parent of the specification DIE.  */
@@ -8771,8 +8778,8 @@  partial_die_parent_scope (struct partial_die_info *pdi,
   real_pdi = pdi;
   while (real_pdi->has_specification)
     {
-      res = find_partial_die (real_pdi->spec_offset,
-			      real_pdi->spec_is_dwz, cu);
+      auto res = find_partial_die (real_pdi->spec_offset,
+				   real_pdi->spec_is_dwz, cu);
       real_pdi = res.pdi;
       cu = res.cu;
     }
@@ -18919,7 +18926,7 @@  dwarf2_cu::find_partial_die (sect_offset sect_off)
    outside their CU (they do however referencing other types via
    DW_FORM_ref_sig8).  */
 
-static struct cu_partial_die_info
+static const struct cu_partial_die_info
 find_partial_die (sect_offset sect_off, int offset_in_dwz, struct dwarf2_cu *cu)
 {
   struct dwarf2_per_objfile *dwarf2_per_objfile
@@ -19000,7 +19007,6 @@  guess_partial_die_structure_name (struct partial_die_info *struct_pdi,
 
   struct partial_die_info *real_pdi;
   struct partial_die_info *child_pdi;
-  struct cu_partial_die_info res;
 
   /* If this DIE (this DIE's specification, if any) has a parent, then
      we should not do this.  We'll prepend the parent's fully qualified
@@ -19009,8 +19015,8 @@  guess_partial_die_structure_name (struct partial_die_info *struct_pdi,
   real_pdi = struct_pdi;
   while (real_pdi->has_specification)
     {
-      res = find_partial_die (real_pdi->spec_offset,
-			      real_pdi->spec_is_dwz, cu);
+      auto res = find_partial_die (real_pdi->spec_offset,
+				   real_pdi->spec_is_dwz, cu);
       real_pdi = res.pdi;
       cu = res.cu;
     }
@@ -19058,9 +19064,8 @@  partial_die_info::fixup (struct dwarf2_cu *cu)
   if (name == NULL && has_specification)
     {
       struct partial_die_info *spec_die;
-      struct cu_partial_die_info res;
 
-      res = find_partial_die (spec_offset, spec_is_dwz, cu);
+      auto res = find_partial_die (spec_offset, spec_is_dwz, cu);
       spec_die = res.pdi;
       cu = res.cu;