Fix heap-use-after-free in typename_concat
Commit Message
* 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 <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
>
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;
>
>
@@ -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;