[PATCH 1/2] gdb: check for empty offsets vector in inherit_abstract_dies

[PATCH 1/2] gdb: check for empty offsets vector in inherit_abstract_dies

[Simon Marchi]

When building GDB with clang and --enable-ubsan, I get:

  UNRESOLVED: gdb.dwarf2/frame-inlined-in-outer-frame.exp: starti prompt

The cause being:

    $ ./gdb --data-directory=data-directory -nx -q -readnow testsuite/outputs/gdb.dwarf2/frame-inlined-in-outer-frame/frame-inlined-in-outer-frame
    Reading symbols from testsuite/outputs/gdb.dwarf2/frame-inlined-in-outer-frame/frame-inlined-in-outer-frame...
    Expanding full symbols from testsuite/outputs/gdb.dwarf2/frame-inlined-in-outer-frame/frame-inlined-in-outer-frame...
    /home/simark/src/binutils-gdb/gdb/dwarf2/read.c:11954:47: runtime error: applying non-zero offset 8 to null pointer

I found this to happen with ld-linux on at least Arch Linux and Ubuntu
22.04:

    $ ./gdb --data-directory=data-directory -nx -q -readnow -iex "set debuginfod enabled on" /lib64/ld-linux-x86-64.so.2
    Reading symbols from /lib64/ld-linux-x86-64.so.2...
    Reading symbols from /home/simark/.cache/debuginfod_client/22bd7a2c03d8cfc05ef7092bfae5932223189bc1/debuginfo...
    Expanding full symbols from /home/simark/.cache/debuginfod_client/22bd7a2c03d8cfc05ef7092bfae5932223189bc1/debuginfo...
    /home/simark/src/binutils-gdb/gdb/dwarf2/read.c:11954:47: runtime error: applying non-zero offset 8 to null pointer

The problem happens when doing this:

    sect_offset *offsetp = offsets.data () + 1

When `offsets` is an empty vector, `offsets.data ()` returns nullptr.
Fix it by wrapping that in a `!offsets.empty ()` check.

Change-Id: I6d29ba2fe80ba4308f68effd9c57d4ee8d67c29f
---
 gdb/dwarf2/read.c | 23 ++++++++++++++---------
 1 file changed, 14 insertions(+), 9 deletions(-)

diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
index 89ba9122e910..bf52354c4260 100644
--- a/gdb/dwarf2/read.c
+++ b/gdb/dwarf2/read.c
@@ -11949,17 +11949,22 @@ inherit_abstract_dies (struct die_info *die, struct dwarf2_cu *cu)
 	corresponding_abstract_child = corresponding_abstract_child->sibling;
     }
 
-  std::sort (offsets.begin (), offsets.end ());
-  sect_offset *offsets_end = offsets.data () + offsets.size ();
-  for (sect_offset *offsetp = offsets.data () + 1;
-       offsetp < offsets_end;
-       offsetp++)
-    if (offsetp[-1] == *offsetp)
-      complaint (_("Multiple children of DIE %s refer "
-		   "to DIE %s as their abstract origin"),
-		 sect_offset_str (die->sect_off), sect_offset_str (*offsetp));
+  if (!offsets.empty ())
+    {
+      std::sort (offsets.begin (), offsets.end ());
+      sect_offset *offsets_end = offsets.data () + offsets.size ();
+      for (sect_offset *offsetp = offsets.data () + 1;
+	   offsetp < offsets_end;
+	   offsetp++)
+	if (offsetp[-1] == *offsetp)
+	  complaint (_("Multiple children of DIE %s refer "
+		       "to DIE %s as their abstract origin"),
+		     sect_offset_str (die->sect_off),
+		     sect_offset_str (*offsetp));
+    }
 
   sect_offset *offsetp = offsets.data ();
+  sect_offset *offsets_end = offsets.data () + offsets.size ();
   die_info *origin_child_die = origin_die->child;
   while (origin_child_die != nullptr && origin_child_die->tag != 0)
     {

base-commit: 75436c534bfd7f548a13b5f926c3bd234b23b8d0
-- 
2.38.0

[PATCH 2/2] gdb: make inherit_abstract_dies use vector iterators

[Simon Marchi]

Small cleanup to use std::vector iterators rather than raw pointers.

Change-Id: I8d50dbb3f2d8dad7ff94066a578d523f1f31b590
---
 gdb/dwarf2/read.c | 25 ++++++++++++-------------
 1 file changed, 12 insertions(+), 13 deletions(-)

diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
index bf52354c4260..c233b3743140 100644
--- a/gdb/dwarf2/read.c
+++ b/gdb/dwarf2/read.c
@@ -11952,29 +11952,28 @@ inherit_abstract_dies (struct die_info *die, struct dwarf2_cu *cu)
   if (!offsets.empty ())
     {
       std::sort (offsets.begin (), offsets.end ());
-      sect_offset *offsets_end = offsets.data () + offsets.size ();
-      for (sect_offset *offsetp = offsets.data () + 1;
-	   offsetp < offsets_end;
-	   offsetp++)
-	if (offsetp[-1] == *offsetp)
+
+      for (auto offsets_it = offsets.begin () + 1;
+	   offsets_it < offsets.end ();
+	   ++offsets_it)
+	if (*(offsets_it - 1) == *offsets_it)
 	  complaint (_("Multiple children of DIE %s refer "
 		       "to DIE %s as their abstract origin"),
 		     sect_offset_str (die->sect_off),
-		     sect_offset_str (*offsetp));
+		     sect_offset_str (*offsets_it));
     }
 
-  sect_offset *offsetp = offsets.data ();
-  sect_offset *offsets_end = offsets.data () + offsets.size ();
+  auto offsets_it = offsets.begin ();
   die_info *origin_child_die = origin_die->child;
   while (origin_child_die != nullptr && origin_child_die->tag != 0)
     {
       /* Is ORIGIN_CHILD_DIE referenced by any of the DIE children?  */
-      while (offsetp < offsets_end
-	     && *offsetp < origin_child_die->sect_off)
-	offsetp++;
+      while (offsets_it < offsets.end ()
+	     && *offsets_it < origin_child_die->sect_off)
+	++offsets_it;
 
-      if (offsetp >= offsets_end
-	  || *offsetp > origin_child_die->sect_off)
+      if (offsets_it == offsets.end ()
+	  || *offsets_it > origin_child_die->sect_off)
 	{
 	  /* Found that ORIGIN_CHILD_DIE is really not referenced.
 	     Check whether we're already processing ORIGIN_CHILD_DIE.
-- 
2.38.0

Re: [PATCH 2/2] gdb: make inherit_abstract_dies use vector iterators

[Tom Tromey]

>>>>> "Simon" == Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:

Simon> Small cleanup to use std::vector iterators rather than raw pointers.

Looks good.

I think my previous note about hoisting the loop might be wrong after
all.

Simon> -      if (offsetp >= offsets_end
Simon> -	  || *offsetp > origin_child_die->sect_off)
Simon> +      if (offsets_it == offsets.end ()

... like I guess this could trigger.

This code is extremely hard to read IMO.

Tom

Re: [PATCH 2/2] gdb: make inherit_abstract_dies use vector iterators

[Tom Tromey]

>>>>> "Simon" == Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:

Simon> Small cleanup to use std::vector iterators rather than raw pointers.

Looks good.

I think my previous note about hoisting the loop might be wrong after
all.

Simon> -      if (offsetp >= offsets_end
Simon> -	  || *offsetp > origin_child_die->sect_off)
Simon> +      if (offsets_it == offsets.end ()

... like I guess this could trigger.

This code is extremely hard to read IMO.

Tom

Re: [PATCH 2/2] gdb: make inherit_abstract_dies use vector iterators

[Simon Marchi]

On 2022-10-21 14:04, Tom Tromey wrote:
>>>>>> "Simon" == Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:
> 
> Simon> Small cleanup to use std::vector iterators rather than raw pointers.
> 
> Looks good.
> 
> I think my previous note about hoisting the loop might be wrong after
> all.
> 
> Simon> -      if (offsetp >= offsets_end
> Simon> -	  || *offsetp > origin_child_die->sect_off)
> Simon> +      if (offsets_it == offsets.end ()
> 
> ... like I guess this could trigger.

That's what I thought.  I will push the series, leaving this part as is
just to stay on the safe side.

Thanks,

Simon

Re: [PATCH 1/2] gdb: check for empty offsets vector in inherit_abstract_dies

[Tom Tromey]

>>>>> "Simon" == Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:

Simon> +  if (!offsets.empty ())
Simon> +    {
Simon> +      std::sort (offsets.begin (), offsets.end ());
Simon> +      sect_offset *offsets_end = offsets.data () + offsets.size ();
Simon> +      for (sect_offset *offsetp = offsets.data () + 1;
Simon> +	   offsetp < offsets_end;
Simon> +	   offsetp++)
Simon> +	if (offsetp[-1] == *offsetp)
Simon> +	  complaint (_("Multiple children of DIE %s refer "
Simon> +		       "to DIE %s as their abstract origin"),
Simon> +		     sect_offset_str (die->sect_off),
Simon> +		     sect_offset_str (*offsetp));
Simon> +    }
 
Simon>    sect_offset *offsetp = offsets.data ();
Simon> +  sect_offset *offsets_end = offsets.data () + offsets.size ();
Simon>    die_info *origin_child_die = origin_die->child;
Simon>    while (origin_child_die != nullptr && origin_child_die->tag != 0)
Simon>      {

This subsequent loop does stuff like:

      while (offsetp < offsets_end
	     && *offsetp < origin_child_die->sect_off)
	offsetp++;

which in this scenario would be comparing null pointers using "<"?

I'm guessing this whole loop should be hoisted into the 'if'.

Tom

Re: [PATCH 1/2] gdb: check for empty offsets vector in inherit_abstract_dies

[Tom Tromey]

>>>>> "Simon" == Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:

Simon> +  if (!offsets.empty ())
Simon> +    {
Simon> +      std::sort (offsets.begin (), offsets.end ());
Simon> +      sect_offset *offsets_end = offsets.data () + offsets.size ();
Simon> +      for (sect_offset *offsetp = offsets.data () + 1;
Simon> +	   offsetp < offsets_end;
Simon> +	   offsetp++)
Simon> +	if (offsetp[-1] == *offsetp)
Simon> +	  complaint (_("Multiple children of DIE %s refer "
Simon> +		       "to DIE %s as their abstract origin"),
Simon> +		     sect_offset_str (die->sect_off),
Simon> +		     sect_offset_str (*offsetp));
Simon> +    }
 
Simon>    sect_offset *offsetp = offsets.data ();
Simon> +  sect_offset *offsets_end = offsets.data () + offsets.size ();
Simon>    die_info *origin_child_die = origin_die->child;
Simon>    while (origin_child_die != nullptr && origin_child_die->tag != 0)
Simon>      {

This subsequent loop does stuff like:

      while (offsetp < offsets_end
	     && *offsetp < origin_child_die->sect_off)
	offsetp++;

which in this scenario would be comparing null pointers using "<"?

I'm guessing this whole loop should be hoisted into the 'if'.

Tom

Re: [PATCH 1/2] gdb: check for empty offsets vector in inherit_abstract_dies

[Simon Marchi]

On 2022-10-21 11:35, Tom Tromey wrote:
>>>>>> "Simon" == Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:
> 
> Simon> +  if (!offsets.empty ())
> Simon> +    {
> Simon> +      std::sort (offsets.begin (), offsets.end ());
> Simon> +      sect_offset *offsets_end = offsets.data () + offsets.size ();
> Simon> +      for (sect_offset *offsetp = offsets.data () + 1;
> Simon> +	   offsetp < offsets_end;
> Simon> +	   offsetp++)
> Simon> +	if (offsetp[-1] == *offsetp)
> Simon> +	  complaint (_("Multiple children of DIE %s refer "
> Simon> +		       "to DIE %s as their abstract origin"),
> Simon> +		     sect_offset_str (die->sect_off),
> Simon> +		     sect_offset_str (*offsetp));
> Simon> +    }
>  
> Simon>    sect_offset *offsetp = offsets.data ();
> Simon> +  sect_offset *offsets_end = offsets.data () + offsets.size ();
> Simon>    die_info *origin_child_die = origin_die->child;
> Simon>    while (origin_child_die != nullptr && origin_child_die->tag != 0)
> Simon>      {
> 
> This subsequent loop does stuff like:
> 
>       while (offsetp < offsets_end
> 	     && *offsetp < origin_child_die->sect_off)
> 	offsetp++;
> 
> which in this scenario would be comparing null pointers using "<"?

Is it undefined behavior to compre two null pointers (both offsetp and
offset_end would be nullptr or both wouldn't be nullptr)?  At least
clang's UB sanitizer doesn't panic.

> I'm guessing this whole loop should be hoisted into the 'if'.

I don't understand enough what the code does to be convinced this would
be a correct change.

Note that the following patch changes this to use iterators, so it makes
the worry about comparing null pointers being undefined behavior go
away.  It becomes comparing offsets.end() with offsets.end().

Simon

Re: [PATCH 1/2] gdb: check for empty offsets vector in inherit_abstract_dies

[Tom Tromey]

>>>>> "Simon" == Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:

>> This subsequent loop does stuff like:
>> 
>> while (offsetp < offsets_end
>> && *offsetp < origin_child_die->sect_off)
>> offsetp++;
>> 
>> which in this scenario would be comparing null pointers using "<"?

Simon> Is it undefined behavior to compre two null pointers (both offsetp and
Simon> offset_end would be nullptr or both wouldn't be nullptr)?  At least
Simon> clang's UB sanitizer doesn't panic.

I don't know but it's certainly weird.

>> I'm guessing this whole loop should be hoisted into the 'if'.

Simon> I don't understand enough what the code does to be convinced this would
Simon> be a correct change.

Any real work done in the loop is conditional on checking 'offsetp',
which will always be NULL in this scenario.

The loop also updates origin_child_die, but that's not used after the
loop completes.

So while I also think the code is super obscure, it also seems correct
to do this hoisting.

Simon> Note that the following patch changes this to use iterators, so it makes
Simon> the worry about comparing null pointers being undefined behavior go
Simon> away.  It becomes comparing offsets.end() with offsets.end().

Yeah.

Tom