diff mbox series

Avoid dangling pointer in variant part reader

Message ID 20250320130659.2258240-1-tromey@adacore.com
State New
Headers
Series Avoid dangling pointer in variant part reader |

Checks

Context Check Description
linaro-tcwg-bot/tcwg_gdb_build--master-arm success Build passed
linaro-tcwg-bot/tcwg_gdb_check--master-arm success Test passed
linaro-tcwg-bot/tcwg_gdb_build--master-aarch64 success Build passed
linaro-tcwg-bot/tcwg_gdb_check--master-aarch64 success Test passed

Commit Message

Tom Tromey March 20, 2025, 1:06 p.m. UTC 
  I think the DWARF reader's variant part code may have a latent bug
with dangling pointers.

handle_variant_part does:

      variant_field &current = fi->current_variant_part->variants.back ();
      new_part = &current.variant_parts.emplace_back ();

Then stores a pointer to this:

  scoped_restore save_current_variant_part
    = make_scoped_restore (&fi->current_variant_part, new_part);

However, it then proceeds to call handle_struct_member_die -- which is
potentially recursive, if the type contains nested variant parts.

It seems to me that if the recursion causes the vector to be
reallocated, then the 'new_part' pointer will be invalidated.

There is a similar problem in handle_variant, where this reference:

  variant_field &variant = fi->current_variant_part->variants.emplace_back ();

could conceivably be invalidated.

This patch fixes the problem by using a std::list instead of a
std::vector.  This ensures that additions won't cause pointer or
reference invalidation.
---
 gdb/dwarf2/read.c | 40 +++++++++++++++++++++++++---------------
 1 file changed, 25 insertions(+), 15 deletions(-)


base-commit: 1b7a712f236504fdc4f741759eb45a21f122aec2

Comments

Simon Marchi March 20, 2025, 4:53 p.m. UTC | #1 
On 3/20/25 9:06 AM, Tom Tromey wrote:
> I think the DWARF reader's variant part code may have a latent bug
> with dangling pointers.
> 
> handle_variant_part does:
> 
>       variant_field &current = fi->current_variant_part->variants.back ();
>       new_part = &current.variant_parts.emplace_back ();
> 
> Then stores a pointer to this:
> 
>   scoped_restore save_current_variant_part
>     = make_scoped_restore (&fi->current_variant_part, new_part);
> 
> However, it then proceeds to call handle_struct_member_die -- which is
> potentially recursive, if the type contains nested variant parts.
> 
> It seems to me that if the recursion causes the vector to be
> reallocated, then the 'new_part' pointer will be invalidated.
> 
> There is a similar problem in handle_variant, where this reference:
> 
>   variant_field &variant = fi->current_variant_part->variants.emplace_back ();
> 
> could conceivably be invalidated.
> 
> This patch fixes the problem by using a std::list instead of a
> std::vector.  This ensures that additions won't cause pointer or
> reference invalidation.

I do see the problem with the "variant_parts" vector, but not with the
"variants" vector.  I don't see exactly when an entry would be added to
the "variants" vector while holding a reference to an existing element
of it.

Simon
Tom Tromey March 20, 2025, 5:55 p.m. UTC | #2 
>>>>> "Simon" == Simon Marchi <simark@simark.ca> writes:

Simon> I do see the problem with the "variant_parts" vector, but not with the
Simon> "variants" vector.  I don't see exactly when an entry would be added to
Simon> the "variants" vector while holding a reference to an existing element
Simon> of it.

In handle_variant:

  variant_field &variant = fi->current_variant_part->variants.emplace_back ();
[...]
// possibly recursive call
    handle_struct_member_die (variant_child, type, fi, template_args, cu);
[...]
  variant.last_field = fi->fields.size ();

Tom
Simon Marchi March 21, 2025, 2:34 a.m. UTC | #3 
On 2025-03-20 13:55, Tom Tromey wrote:
>>>>>> "Simon" == Simon Marchi <simark@simark.ca> writes:
> 
> Simon> I do see the problem with the "variant_parts" vector, but not with the
> Simon> "variants" vector.  I don't see exactly when an entry would be added to
> Simon> the "variants" vector while holding a reference to an existing element
> Simon> of it.
> 
> In handle_variant:
> 
>   variant_field &variant = fi->current_variant_part->variants.emplace_back ();
> [...]
> // possibly recursive call
>     handle_struct_member_die (variant_child, type, fi, template_args, cu);
> [...]
>   variant.last_field = fi->fields.size ();

But if some variants get added to a variant_part_builder::variants
vector during that recursion, won't it be another variant_part_builder
instance?  Because you'll have something like:

  <1> DW_TAG_variant_part
    <2> DW_TAG_variant
      <3> DW_TAG_variant_part
        <4> DW_TAG_variant

During the handling of the inner DW_TAG_variant_part,
fi->current_variant_part will get temporarily replaced by another
variant_part_builder.

Unless you can have:

  <1> DW_TAG_variant_part
    <2> DW_TAG_variant
      <3> DW_TAG_variant

But I'm not sure what it would mean.

Actually, even for the variant_part_builder vector, it looks like the
same situation.  While handling one variant part, if recursion happens
and we process a nested variant part, that variant part will get added to
a different vector than the top-level variant part is in.  The nested
variant part will be added to the vector owned by the variant just above
(<2>, in the first example).

Simon
Tom Tromey April 22, 2025, 2:56 p.m. UTC | #4 
Simon> Actually, even for the variant_part_builder vector, it looks like the
Simon> same situation.  While handling one variant part, if recursion happens
Simon> and we process a nested variant part, that variant part will get added to
Simon> a different vector than the top-level variant part is in.  The nested
Simon> variant part will be added to the vector owned by the variant just above
Simon> (<2>, in the first example).

Yeah, I think you're right -- oops.
At least this explains why we never saw a bug here.

Tom
diff mbox series

Patch

diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
index b9040a57cf9..8bef7ab54a0 100644
--- a/gdb/dwarf2/read.c
+++ b/gdb/dwarf2/read.c
@@ -97,6 +97,7 @@ 
 #include "dwarf2/error.h"
 #include "gdbsupport/unordered_set.h"
 #include "extract-store-integer.h"
+#include <list>
 
 /* When == 1, print basic high level tracing messages.
    When > 1, be more verbose.
@@ -619,8 +620,9 @@  struct variant_field
   int first_field = -1;
   int last_field = -1;
 
-  /* A variant can contain other variant parts.  */
-  std::vector<variant_part_builder> variant_parts;
+  /* A variant can contain other variant parts.  A list is used so
+     that the contained objects don't move during reallocation.  */
+  std::list<variant_part_builder> variant_parts;
 
   /* If we see a DW_TAG_variant, then this will be set if this is the
      default branch.  */
@@ -640,8 +642,10 @@  struct variant_part_builder
   /* The offset of the discriminant field.  */
   sect_offset discriminant_offset {};
 
-  /* Variants that are direct children of this variant part.  */
-  std::vector<variant_field> variants;
+  /* Variants that are direct children of this variant part.  A list
+     is used so that the contained objects don't move during
+     reallocation.  */
+  std::list<variant_field> variants;
 
   /* True if we're currently reading a variant.  */
   bool processing_variant = false;
@@ -689,7 +693,7 @@  struct field_info
   variant_part_builder *current_variant_part = nullptr;
   /* This holds all the top-level variant parts attached to the type
      we're reading.  */
-  std::vector<variant_part_builder> variant_parts;
+  std::list<variant_part_builder> variant_parts;
 
   /* Return the total number of fields (including baseclasses).  */
   int nfields () const
@@ -10426,7 +10430,7 @@  static const gdb::array_view<variant_part> create_variant_parts
   (struct obstack *obstack,
    const offset_map_type &offset_map,
    struct field_info *fi,
-   const std::vector<variant_part_builder> &variant_parts);
+   const std::list<variant_part_builder> &variant_parts);
 
 /* Fill in a "struct variant" for a given variant field.  RESULT is
    the variant to fill in.  OBSTACK is where any needed allocations
@@ -10475,12 +10479,15 @@  create_one_variant_part (variant_part &result,
     }
 
   size_t n = builder.variants.size ();
-  variant *output = new (obstack) variant[n];
-  for (size_t i = 0; i < n; ++i)
-    create_one_variant (output[i], obstack, offset_map, fi,
-			builder.variants[i]);
+  variant *new_variants = new (obstack) variant[n];
+  variant *out_iter = new_variants;
+  for (const variant_field &field : builder.variants)
+    {
+      create_one_variant (*out_iter, obstack, offset_map, fi, field);
+      ++out_iter;
+    }
 
-  result.variants = gdb::array_view<variant> (output, n);
+  result.variants = gdb::array_view<variant> (new_variants, n);
 }
 
 /* Create a vector of variant parts that can be attached to a type.
@@ -10493,16 +10500,19 @@  static const gdb::array_view<variant_part>
 create_variant_parts (struct obstack *obstack,
 		      const offset_map_type &offset_map,
 		      struct field_info *fi,
-		      const std::vector<variant_part_builder> &variant_parts)
+		      const std::list<variant_part_builder> &variant_parts)
 {
   if (variant_parts.empty ())
     return {};
 
   size_t n = variant_parts.size ();
   variant_part *result = new (obstack) variant_part[n];
-  for (size_t i = 0; i < n; ++i)
-    create_one_variant_part (result[i], obstack, offset_map, fi,
-			     variant_parts[i]);
+  variant_part *out_iter = result;
+  for (const variant_part_builder &builder : variant_parts)
+    {
+      create_one_variant_part (*out_iter, obstack, offset_map, fi, builder);
+      ++out_iter;
+    }
 
   return gdb::array_view<variant_part> (result, n);
 }