[PATCH 0/2] Avoid resolving dynamic properties for non-allocated arrays

[PATCH 0/2] Avoid resolving dynamic properties for non-allocated arrays

[Andrew Burgess]

Fix for issue PR gdb/27059 in patch #2, with a small helper patch in #1.

All feedback welcome.

Thanks,
Andrew


---

Andrew Burgess (2):
  gdb: include allocated/associated properties in 'maint print type'
  gdb: avoid resolving dynamic properties for non-allocated arrays

 gdb/ChangeLog                                 |  19 +++
 gdb/eval.c                                    |  12 +-
 gdb/f-lang.c                                  |   4 +
 gdb/gdbtypes.c                                |  87 ++++++++++---
 gdb/testsuite/ChangeLog                       |   6 +
 .../gdb.dwarf2/dyn-type-unallocated.c         |  40 ++++++
 .../gdb.dwarf2/dyn-type-unallocated.exp       | 122 ++++++++++++++++++
 7 files changed, 266 insertions(+), 24 deletions(-)
 create mode 100644 gdb/testsuite/gdb.dwarf2/dyn-type-unallocated.c
 create mode 100644 gdb/testsuite/gdb.dwarf2/dyn-type-unallocated.exp

-- 
2.25.4

[PATCH 1/2] gdb: include allocated/associated properties in 'maint print type'

[Andrew Burgess]

Adds the allocated and associated dynamic properties into the output
of the 'maintenance print type' command.

gdb/ChangeLog:

	* gdbtypes (recursive_dump_type): Include allocated and associated
	properties.
---
 gdb/ChangeLog  |  5 +++++
 gdb/gdbtypes.c | 18 ++++++++++++++++++
 2 files changed, 23 insertions(+)

diff --git a/gdb/gdbtypes.c b/gdb/gdbtypes.c
index 569e7a3e659..8e90c4b108b 100644
--- a/gdb/gdbtypes.c
+++ b/gdb/gdbtypes.c
@@ -5269,6 +5269,24 @@ recursive_dump_type (struct type *type, int spaces)
     }
   puts_filtered ("\n");
   printf_filtered ("%*snfields %d ", spaces, "", type->num_fields ());
+  if (TYPE_ASSOCIATED_PROP (type) != nullptr
+      || TYPE_ALLOCATED_PROP (type) != nullptr)
+    {
+      printf_filtered ("%*s", spaces, "");
+      if (TYPE_ASSOCIATED_PROP (type) != nullptr)
+	{
+	  printf_filtered ("associated ");
+	  dump_dynamic_prop (*TYPE_ASSOCIATED_PROP (type));
+	}
+      if (TYPE_ALLOCATED_PROP (type) != nullptr)
+	{
+	  if (TYPE_ASSOCIATED_PROP (type))
+	    printf_filtered ("  ");
+	  printf_filtered ("allocated ");
+	  dump_dynamic_prop (*TYPE_ALLOCATED_PROP (type));
+	}
+      printf_filtered ("\n");
+    }
   gdb_print_host_address (type->fields (), gdb_stdout);
   puts_filtered ("\n");
   for (idx = 0; idx < type->num_fields (); idx++)
-- 
2.25.4

Re: [PATCH 1/2] gdb: include allocated/associated properties in 'maint print type'

[Simon Marchi via Gdb-patches]

On 2020-12-23 6:53 p.m., Andrew Burgess wrote:
> Adds the allocated and associated dynamic properties into the output
> of the 'maintenance print type' command.
> 
> gdb/ChangeLog:
> 
> 	* gdbtypes (recursive_dump_type): Include allocated and associated
> 	properties.
> ---
>  gdb/ChangeLog  |  5 +++++
>  gdb/gdbtypes.c | 18 ++++++++++++++++++
>  2 files changed, 23 insertions(+)
> 
> diff --git a/gdb/gdbtypes.c b/gdb/gdbtypes.c
> index 569e7a3e659..8e90c4b108b 100644
> --- a/gdb/gdbtypes.c
> +++ b/gdb/gdbtypes.c
> @@ -5269,6 +5269,24 @@ recursive_dump_type (struct type *type, int spaces)
>      }
>    puts_filtered ("\n");
>    printf_filtered ("%*snfields %d ", spaces, "", type->num_fields ());
> +  if (TYPE_ASSOCIATED_PROP (type) != nullptr
> +      || TYPE_ALLOCATED_PROP (type) != nullptr)
> +    {
> +      printf_filtered ("%*s", spaces, "");
> +      if (TYPE_ASSOCIATED_PROP (type) != nullptr)
> +	{
> +	  printf_filtered ("associated ");
> +	  dump_dynamic_prop (*TYPE_ASSOCIATED_PROP (type));
> +	}
> +      if (TYPE_ALLOCATED_PROP (type) != nullptr)
> +	{
> +	  if (TYPE_ASSOCIATED_PROP (type))

!= nullptr

Otherwise, LGTM.

Simon

[PATCH 2/2] gdb: avoid resolving dynamic properties for non-allocated arrays

[Andrew Burgess]

In PR gdb/27059 an issue was discovered where GDB would sometimes
trigger undefined behaviour in the form of signed integer overflow.
The problem here is that GDB was reading random garbage from the
inferior memory space, assuming this data was valid, and performing
arithmetic on it.

This bug raises an interesting general problem with GDB's DWARF
expression evaluator, which is this:

We currently assume that the DWARF expressions being evaluated are
well formed, and well behaving.  As an example, this is the expression
that the bug was running into problems on, this was used as the
expression for a DW_AT_byte_stride of a DW_TAG_subrange_type:

	DW_OP_push_object_address;
	DW_OP_plus_uconst: 88;
	DW_OP_deref;
	DW_OP_push_object_address;
	DW_OP_plus_uconst: 32;
	DW_OP_deref;
	DW_OP_mul

Two values are read from the inferior and multiplied together.  GDB
should not assume that any value read from the inferior is in any way
sane, as such the implementation of DW_OP_mul should be guarding
against overflow and doing something semi-sane here.

However, it turns out that the original bug PR gdb/27059, is hitting a
more specific case, which doesn't require changes to the DWARF
expression evaluator, so I'm going to leave the above issue for
another day.

In the test mentioned in the bug GDB is actually trying to resolve the
dynamic type of a Fortran array that is NOT allocated.  A
non-allocated Fortran array is one that does not have any data
allocated for it yet, and even the upper and lower bounds of the array
are not yet known.

It turns out that, at least for gfortran compiled code, the data
fields that describe the byte-stride are not initialised until the
array is allocated.

This leads me to the following conclusion: GDB should not try to
resolve the bounds, or stride information for an array that is not
allocated (or not associated, a similar, but slightly different
Fortran feature).  Instead, each of these properties should be set to
undefined if the array is not allocated (or associated).

That is what this commit does.  There's a new flag that is passed
around during the dynamic array resolution.  When this flag is true
the dynamic properties are resolved using the DWARF expressions as
they currently are, but when this flag is false the expressions are
not evaluated, and instead the properties are set to undefined.

gdb/ChangeLog:

	PR gdb/27059
	* eval.c (evaluate_subexp_for_sizeof): Handle not allocated and
	not associated arrays.
	* f-lang.c (fortran_adjust_dynamic_array_base_address_hack): Don't
	adjust arrays that are not allocated/associated.
	* gdbtypes.c (resolve_dynamic_range): Update header comment.  Add
	new parameter which is used to sometimes set dynamic properties to
	undefined.
	(resolve_dynamic_array_or_string): Update header comment.  Add new
	parameter which is used to guard evaluating dynamic properties.
	Resolve allocated/associated properties first.

gdb/testsuite/ChangeLog:

	PR gdb/27059
	* gdb.dwarf2/dyn-type-unallocated.c: New file.
	* gdb.dwarf2/dyn-type-unallocated.exp: New file.
---
 gdb/ChangeLog                                 |  14 ++
 gdb/eval.c                                    |  12 +-
 gdb/f-lang.c                                  |   4 +
 gdb/gdbtypes.c                                |  69 +++++++---
 gdb/testsuite/ChangeLog                       |   6 +
 .../gdb.dwarf2/dyn-type-unallocated.c         |  40 ++++++
 .../gdb.dwarf2/dyn-type-unallocated.exp       | 122 ++++++++++++++++++
 7 files changed, 243 insertions(+), 24 deletions(-)
 create mode 100644 gdb/testsuite/gdb.dwarf2/dyn-type-unallocated.c
 create mode 100644 gdb/testsuite/gdb.dwarf2/dyn-type-unallocated.exp

diff --git a/gdb/eval.c b/gdb/eval.c
index c781fde0614..dadadbb8353 100644
--- a/gdb/eval.c
+++ b/gdb/eval.c
@@ -2961,10 +2961,14 @@ evaluate_subexp_for_sizeof (struct expression *exp, int *pos,
 	{
 	  val = evaluate_subexp (nullptr, exp, pos, EVAL_NORMAL);
 	  type = value_type (val);
-	  if (type->code () == TYPE_CODE_ARRAY
-	      && is_dynamic_type (type->index_type ())
-	      && type->bounds ()->high.kind () == PROP_UNDEFINED)
-	    return allocate_optimized_out_value (size_type);
+	  if (type->code () == TYPE_CODE_ARRAY)
+	    {
+	      if (type_not_allocated (type) || type_not_associated (type))
+		return value_zero (size_type, not_lval);
+	      else if (is_dynamic_type (type->index_type ())
+		       && type->bounds ()->high.kind () == PROP_UNDEFINED)
+		return allocate_optimized_out_value (size_type);
+	    }
 	}
       else
 	(*pos) += 4;
diff --git a/gdb/f-lang.c b/gdb/f-lang.c
index 28a66fdde94..e06bbb42956 100644
--- a/gdb/f-lang.c
+++ b/gdb/f-lang.c
@@ -1391,6 +1391,10 @@ fortran_adjust_dynamic_array_base_address_hack (struct type *type,
 {
   gdb_assert (type->code () == TYPE_CODE_ARRAY);
 
+  /* We can't adjust the base address for arrays that have no content.  */
+  if (type_not_allocated (type) || type_not_associated (type))
+    return address;
+
   int ndimensions = calc_f77_array_dims (type);
   LONGEST total_offset = 0;
 
diff --git a/gdb/gdbtypes.c b/gdb/gdbtypes.c
index 8e90c4b108b..8237eedaec8 100644
--- a/gdb/gdbtypes.c
+++ b/gdb/gdbtypes.c
@@ -2177,11 +2177,20 @@ static struct type *resolve_dynamic_type_internal
 
 /* Given a dynamic range type (dyn_range_type) and a stack of
    struct property_addr_info elements, return a static version
-   of that type.  */
+   of that type.
+
+   When RESOLVE_P is true then the returned static range is created by
+   actually evaluating any dynamic properties within the range type, while
+   when RESOLVE_P is false the returned static range has all of the bounds
+   and stride information set to undefined.  The RESOLVE_P set to false
+   case will be used when evaluating a dynamic array that is not
+   allocated, or not associated, i.e. the bounds information might not be
+   initialized yet.  */
 
 static struct type *
 resolve_dynamic_range (struct type *dyn_range_type,
-		       struct property_addr_info *addr_stack)
+		       struct property_addr_info *addr_stack,
+		       bool resolve_p = true)
 {
   CORE_ADDR value;
   struct type *static_range_type, *static_target_type;
@@ -2190,13 +2199,13 @@ resolve_dynamic_range (struct type *dyn_range_type,
   gdb_assert (dyn_range_type->code () == TYPE_CODE_RANGE);
 
   const struct dynamic_prop *prop = &dyn_range_type->bounds ()->low;
-  if (dwarf2_evaluate_property (prop, NULL, addr_stack, &value))
+  if (resolve_p && dwarf2_evaluate_property (prop, NULL, addr_stack, &value))
     low_bound.set_const_val (value);
   else
     low_bound.set_undefined ();
 
   prop = &dyn_range_type->bounds ()->high;
-  if (dwarf2_evaluate_property (prop, NULL, addr_stack, &value))
+  if (resolve_p && dwarf2_evaluate_property (prop, NULL, addr_stack, &value))
     {
       high_bound.set_const_val (value);
 
@@ -2209,7 +2218,7 @@ resolve_dynamic_range (struct type *dyn_range_type,
 
   bool byte_stride_p = dyn_range_type->bounds ()->flag_is_byte_stride;
   prop = &dyn_range_type->bounds ()->stride;
-  if (dwarf2_evaluate_property (prop, NULL, addr_stack, &value))
+  if (resolve_p && dwarf2_evaluate_property (prop, NULL, addr_stack, &value))
     {
       stride.set_const_val (value);
 
@@ -2242,11 +2251,16 @@ resolve_dynamic_range (struct type *dyn_range_type,
 
 /* Resolves dynamic bound values of an array or string type TYPE to static
    ones.  ADDR_STACK is a stack of struct property_addr_info to be used if
-   needed during the dynamic resolution.  */
+   needed during the dynamic resolution.
+
+   When RESOLVE_P is true then the dynamic properties of TYPE are
+   evaluated, otherwise the dynamic properties of TYPE are not evaluated,
+   instead we assume the array is not allocated/associated yet.  */
 
 static struct type *
 resolve_dynamic_array_or_string (struct type *type,
-				 struct property_addr_info *addr_stack)
+				 struct property_addr_info *addr_stack,
+				 bool resolve_p = true)
 {
   CORE_ADDR value;
   struct type *elt_type;
@@ -2262,29 +2276,44 @@ resolve_dynamic_array_or_string (struct type *type,
 
   type = copy_type (type);
 
-  elt_type = type;
-  range_type = check_typedef (elt_type->index_type ());
-  range_type = resolve_dynamic_range (range_type, addr_stack);
-
-  /* Resolve allocated/associated here before creating a new array type, which
-     will update the length of the array accordingly.  */
+  /* Resolve the allocated and associated properties before doing anything
+     else.  If an array is not allocated or not associated then (at least
+     for Fortran) there is no guarantee that the data to define the upper
+     bound, lower bound, or stride will be correct.  If RESOLVE_P is
+     already false at this point then this is not the first dimension of
+     the array and a more outer dimension has already marked this array as
+     not allocated/associated, as such we just ignore this property.  This
+     is fine as GDB only checks the allocated/associated on the outer most
+     dimension of the array.  */
   prop = TYPE_ALLOCATED_PROP (type);
-  if (prop != NULL && dwarf2_evaluate_property (prop, NULL, addr_stack, &value))
-    prop->set_const_val (value);
+  if (prop != NULL && resolve_p
+      && dwarf2_evaluate_property (prop, NULL, addr_stack, &value))
+    {
+      prop->set_const_val (value);
+      if (value == 0)
+	resolve_p = false;
+    }
 
   prop = TYPE_ASSOCIATED_PROP (type);
-  if (prop != NULL && dwarf2_evaluate_property (prop, NULL, addr_stack, &value))
-    prop->set_const_val (value);
+  if (prop != NULL && resolve_p
+      && dwarf2_evaluate_property (prop, NULL, addr_stack, &value))
+    {
+      prop->set_const_val (value);
+      if (value == 0)
+	resolve_p = false;
+    }
 
-  ary_dim = check_typedef (TYPE_TARGET_TYPE (elt_type));
+  range_type = check_typedef (type->index_type ());
+  range_type = resolve_dynamic_range (range_type, addr_stack, resolve_p);
 
+  ary_dim = check_typedef (TYPE_TARGET_TYPE (type));
   if (ary_dim != NULL && ary_dim->code () == TYPE_CODE_ARRAY)
-    elt_type = resolve_dynamic_array_or_string (ary_dim, addr_stack);
+    elt_type = resolve_dynamic_array_or_string (ary_dim, addr_stack, resolve_p);
   else
     elt_type = TYPE_TARGET_TYPE (type);
 
   prop = type->dyn_prop (DYN_PROP_BYTE_STRIDE);
-  if (prop != NULL)
+  if (prop != NULL && resolve_p)
     {
       if (dwarf2_evaluate_property (prop, NULL, addr_stack, &value))
 	{
diff --git a/gdb/testsuite/gdb.dwarf2/dyn-type-unallocated.c b/gdb/testsuite/gdb.dwarf2/dyn-type-unallocated.c
new file mode 100644
index 00000000000..453c54d626b
--- /dev/null
+++ b/gdb/testsuite/gdb.dwarf2/dyn-type-unallocated.c
@@ -0,0 +1,40 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2020 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+#include "../lib/attributes.h"
+
+/* Our fake dynamic object.  */
+void *dyn_object;
+
+void __attribute__((noinline)) ATTRIBUTE_NOCLONE
+marker ()
+{ /* Nothing.  */ }
+
+int
+main ()
+{
+  asm ("main_label: .globl main_label");
+
+  /* Initialise the dynamic object.  */
+  dyn_object = 0;
+
+  asm ("marker_label: .globl marker_label");
+  marker ();	/* Break here.  */
+
+  return 0;
+}
+
diff --git a/gdb/testsuite/gdb.dwarf2/dyn-type-unallocated.exp b/gdb/testsuite/gdb.dwarf2/dyn-type-unallocated.exp
new file mode 100644
index 00000000000..60cf8abc899
--- /dev/null
+++ b/gdb/testsuite/gdb.dwarf2/dyn-type-unallocated.exp
@@ -0,0 +1,122 @@
+# Copyright 2020 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+# Test for issue PR gdb/27059.  The problem was that when resolving a
+# dynamic type that was not-allocated GDB would still try to execute
+# the DWARF expressions for the upper, lower, and byte-stride values.
+#
+# The problem is that, at least in some gfortran compiled programs,
+# these values are undefined until the array is allocated.
+#
+# As a result, executing the dwarf expressions was triggering integer
+# overflow in some cases.
+#
+# This test aims to make the sometimes occurring integer overflow a
+# more noticeable error by creating an array that is always marked as
+# not-allocated.
+#
+# The dwarf expressions for the various attributes then contains an
+# infinite loop.  If GDB ever tries to execute these expressions we
+# will get a test timeout.  With this issue fixed the expressions are
+# never executed and the test completes as we'd expect.
+
+load_lib dwarf.exp
+
+if {![dwarf2_support]} {
+    return 0
+}
+
+standard_testfile .c -dw.S
+
+if { [prepare_for_testing "failed to prepare" ${testfile} ${srcfile}] } {
+    return -1
+}
+
+set asm_file [standard_output_file $srcfile2]
+Dwarf::assemble $asm_file {
+    cu {} {
+	global srcfile
+
+	compile_unit {
+	    {producer "gcc" }
+            {language @DW_LANG_Fortran90}
+            {name ${srcfile}}
+            {low_pc 0 addr}
+        } {
+	    declare_labels array_type_label integer_type_label
+
+	    set int_size [get_sizeof "int" "UNKNOWN"]
+	    set voidp_size [get_sizeof "void *" "UNKNOWN"]
+
+            integer_type_label: DW_TAG_base_type {
+                {DW_AT_byte_size $int_size DW_FORM_sdata}
+                {DW_AT_encoding  @DW_ATE_signed}
+                {DW_AT_name      integer}
+            }
+
+	    array_type_label: DW_TAG_array_type {
+		{DW_AT_type :$integer_type_label}
+		{DW_AT_data_location {
+		    DW_OP_push_object_address
+		    DW_OP_deref
+		} SPECIAL_expr}
+		{DW_AT_allocated {
+		    DW_OP_push_object_address
+		    DW_OP_deref_size ${voidp_size}
+		    DW_OP_lit0
+		    DW_OP_ne
+		} SPECIAL_expr}
+	    } {
+		DW_TAG_subrange_type {
+		    {DW_AT_type        :$integer_type_label}
+		    {DW_AT_lower_bound {
+			DW_OP_skip -3
+		    } SPECIAL_expr}
+		    {DW_AT_upper_bound {
+			DW_OP_skip -3
+		    } SPECIAL_expr}
+		    {DW_AT_byte_stride {
+			DW_OP_skip -3
+		    } SPECIAL_expr}
+		}
+	    }
+
+	    DW_TAG_variable {
+                {DW_AT_location {
+                    DW_OP_addr [gdb_target_symbol dyn_object]
+                } SPECIAL_expr}
+		{name "dyn_object"}
+		{type :$array_type_label}
+	    }
+            subprogram {
+                {external 1 flag}
+                {MACRO_AT_func {main}}
+            }
+        }
+    }
+}
+
+if { [prepare_for_testing "failed to prepare" "${testfile}" \
+	  [list $srcfile $asm_file] {nodebug}] } {
+    return -1
+}
+
+if ![runto_main] {
+    return -1
+}
+
+gdb_breakpoint "marker_label"
+gdb_continue_to_breakpoint "stop at marker_label"
+gdb_test "ptype dyn_object" "type = integer, allocatable \\(:\\)"
-- 
2.25.4

Re: [PATCH 2/2] gdb: avoid resolving dynamic properties for non-allocated arrays

[Simon Marchi via Gdb-patches]

On 2020-12-23 6:53 p.m., Andrew Burgess wrote:
> In PR gdb/27059 an issue was discovered where GDB would sometimes
> trigger undefined behaviour in the form of signed integer overflow.
> The problem here is that GDB was reading random garbage from the
> inferior memory space, assuming this data was valid, and performing
> arithmetic on it.
> 
> This bug raises an interesting general problem with GDB's DWARF
> expression evaluator, which is this:
> 
> We currently assume that the DWARF expressions being evaluated are
> well formed, and well behaving.  As an example, this is the expression
> that the bug was running into problems on, this was used as the
> expression for a DW_AT_byte_stride of a DW_TAG_subrange_type:
> 
> 	DW_OP_push_object_address;
> 	DW_OP_plus_uconst: 88;
> 	DW_OP_deref;
> 	DW_OP_push_object_address;
> 	DW_OP_plus_uconst: 32;
> 	DW_OP_deref;
> 	DW_OP_mul
> 
> Two values are read from the inferior and multiplied together.  GDB
> should not assume that any value read from the inferior is in any way
> sane, as such the implementation of DW_OP_mul should be guarding
> against overflow and doing something semi-sane here.
> 
> However, it turns out that the original bug PR gdb/27059, is hitting a
> more specific case, which doesn't require changes to the DWARF
> expression evaluator, so I'm going to leave the above issue for
> another day.

Ok, so if I understand correctly, I could craft a DWARF expression that
makes an overflowing multiplication on purpose to hit that undefined
behavior bug, right?

If so, it would be good to close 27059 with your fix and open a new bug
specifically for the fact that the DWARF expression evaluator does not
protect against multiplication overflows.

> diff --git a/gdb/testsuite/gdb.dwarf2/dyn-type-unallocated.exp b/gdb/testsuite/gdb.dwarf2/dyn-type-unallocated.exp
> new file mode 100644
> index 00000000000..60cf8abc899
> --- /dev/null
> +++ b/gdb/testsuite/gdb.dwarf2/dyn-type-unallocated.exp
> @@ -0,0 +1,122 @@
> +# Copyright 2020 Free Software Foundation, Inc.
> +
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 3 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +
> +# Test for issue PR gdb/27059.  The problem was that when resolving a
> +# dynamic type that was not-allocated GDB would still try to execute
> +# the DWARF expressions for the upper, lower, and byte-stride values.
> +#
> +# The problem is that, at least in some gfortran compiled programs,
> +# these values are undefined until the array is allocated.
> +#
> +# As a result, executing the dwarf expressions was triggering integer
> +# overflow in some cases.
> +#
> +# This test aims to make the sometimes occurring integer overflow a
> +# more noticeable error by creating an array that is always marked as
> +# not-allocated.
> +#
> +# The dwarf expressions for the various attributes then contains an
> +# infinite loop.  If GDB ever tries to execute these expressions we
> +# will get a test timeout.  With this issue fixed the expressions are
> +# never executed and the test completes as we'd expect.
> +
> +load_lib dwarf.exp
> +
> +if {![dwarf2_support]} {
> +    return 0
> +}
> +
> +standard_testfile .c -dw.S
> +
> +if { [prepare_for_testing "failed to prepare" ${testfile} ${srcfile}] } {
> +    return -1
> +}
> +
> +set asm_file [standard_output_file $srcfile2]
> +Dwarf::assemble $asm_file {
> +    cu {} {
> +	global srcfile
> +
> +	compile_unit {
> +	    {producer "gcc" }
> +            {language @DW_LANG_Fortran90}
> +            {name ${srcfile}}
> +            {low_pc 0 addr}
> +        } {
> +	    declare_labels array_type_label integer_type_label
> +
> +	    set int_size [get_sizeof "int" "UNKNOWN"]
> +	    set voidp_size [get_sizeof "void *" "UNKNOWN"]
> +
> +            integer_type_label: DW_TAG_base_type {
> +                {DW_AT_byte_size $int_size DW_FORM_sdata}
> +                {DW_AT_encoding  @DW_ATE_signed}
> +                {DW_AT_name      integer}
> +            }
> +
> +	    array_type_label: DW_TAG_array_type {
> +		{DW_AT_type :$integer_type_label}
> +		{DW_AT_data_location {
> +		    DW_OP_push_object_address
> +		    DW_OP_deref
> +		} SPECIAL_expr}
> +		{DW_AT_allocated {
> +		    DW_OP_push_object_address
> +		    DW_OP_deref_size ${voidp_size}
> +		    DW_OP_lit0
> +		    DW_OP_ne

Can't this expression just be `DW_OP_lit0`, to make the array always
unallocated?

I also looked at the fix itself, I can't really claim to understand
it perfectly, but nothing stood out as wrong to me, so LGTM.

Thanks!

Simon

Re: [PATCH 2/2] gdb: avoid resolving dynamic properties for non-allocated arrays

[Andrew Burgess]

* Simon Marchi <simon.marchi@polymtl.ca> [2020-12-24 09:46:13 -0500]:

> 
> 
> On 2020-12-23 6:53 p.m., Andrew Burgess wrote:
> > In PR gdb/27059 an issue was discovered where GDB would sometimes
> > trigger undefined behaviour in the form of signed integer overflow.
> > The problem here is that GDB was reading random garbage from the
> > inferior memory space, assuming this data was valid, and performing
> > arithmetic on it.
> > 
> > This bug raises an interesting general problem with GDB's DWARF
> > expression evaluator, which is this:
> > 
> > We currently assume that the DWARF expressions being evaluated are
> > well formed, and well behaving.  As an example, this is the expression
> > that the bug was running into problems on, this was used as the
> > expression for a DW_AT_byte_stride of a DW_TAG_subrange_type:
> > 
> > 	DW_OP_push_object_address;
> > 	DW_OP_plus_uconst: 88;
> > 	DW_OP_deref;
> > 	DW_OP_push_object_address;
> > 	DW_OP_plus_uconst: 32;
> > 	DW_OP_deref;
> > 	DW_OP_mul
> > 
> > Two values are read from the inferior and multiplied together.  GDB
> > should not assume that any value read from the inferior is in any way
> > sane, as such the implementation of DW_OP_mul should be guarding
> > against overflow and doing something semi-sane here.
> > 
> > However, it turns out that the original bug PR gdb/27059, is hitting a
> > more specific case, which doesn't require changes to the DWARF
> > expression evaluator, so I'm going to leave the above issue for
> > another day.
> 
> Ok, so if I understand correctly, I could craft a DWARF expression that
> makes an overflowing multiplication on purpose to hit that undefined
> behavior bug, right?
> 
> If so, it would be good to close 27059 with your fix and open a new bug
> specifically for the fact that the DWARF expression evaluator does not
> protect against multiplication overflows.

Done, bug 27114.  It's not just multiplication, but any arithmetic
operator that might overflow (or have other undefined behaviour).

We do already catch some of these cases, for example in scalar_binop
we do check for divide by zero for example.

> 
> > diff --git a/gdb/testsuite/gdb.dwarf2/dyn-type-unallocated.exp b/gdb/testsuite/gdb.dwarf2/dyn-type-unallocated.exp
> > new file mode 100644
> > index 00000000000..60cf8abc899
> > --- /dev/null
> > +++ b/gdb/testsuite/gdb.dwarf2/dyn-type-unallocated.exp
> > @@ -0,0 +1,122 @@
> > +# Copyright 2020 Free Software Foundation, Inc.
> > +
> > +# This program is free software; you can redistribute it and/or modify
> > +# it under the terms of the GNU General Public License as published by
> > +# the Free Software Foundation; either version 3 of the License, or
> > +# (at your option) any later version.
> > +#
> > +# This program is distributed in the hope that it will be useful,
> > +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> > +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > +# GNU General Public License for more details.
> > +#
> > +# You should have received a copy of the GNU General Public License
> > +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> > +
> > +# Test for issue PR gdb/27059.  The problem was that when resolving a
> > +# dynamic type that was not-allocated GDB would still try to execute
> > +# the DWARF expressions for the upper, lower, and byte-stride values.
> > +#
> > +# The problem is that, at least in some gfortran compiled programs,
> > +# these values are undefined until the array is allocated.
> > +#
> > +# As a result, executing the dwarf expressions was triggering integer
> > +# overflow in some cases.
> > +#
> > +# This test aims to make the sometimes occurring integer overflow a
> > +# more noticeable error by creating an array that is always marked as
> > +# not-allocated.
> > +#
> > +# The dwarf expressions for the various attributes then contains an
> > +# infinite loop.  If GDB ever tries to execute these expressions we
> > +# will get a test timeout.  With this issue fixed the expressions are
> > +# never executed and the test completes as we'd expect.
> > +
> > +load_lib dwarf.exp
> > +
> > +if {![dwarf2_support]} {
> > +    return 0
> > +}
> > +
> > +standard_testfile .c -dw.S
> > +
> > +if { [prepare_for_testing "failed to prepare" ${testfile} ${srcfile}] } {
> > +    return -1
> > +}
> > +
> > +set asm_file [standard_output_file $srcfile2]
> > +Dwarf::assemble $asm_file {
> > +    cu {} {
> > +	global srcfile
> > +
> > +	compile_unit {
> > +	    {producer "gcc" }
> > +            {language @DW_LANG_Fortran90}
> > +            {name ${srcfile}}
> > +            {low_pc 0 addr}
> > +        } {
> > +	    declare_labels array_type_label integer_type_label
> > +
> > +	    set int_size [get_sizeof "int" "UNKNOWN"]
> > +	    set voidp_size [get_sizeof "void *" "UNKNOWN"]
> > +
> > +            integer_type_label: DW_TAG_base_type {
> > +                {DW_AT_byte_size $int_size DW_FORM_sdata}
> > +                {DW_AT_encoding  @DW_ATE_signed}
> > +                {DW_AT_name      integer}
> > +            }
> > +
> > +	    array_type_label: DW_TAG_array_type {
> > +		{DW_AT_type :$integer_type_label}
> > +		{DW_AT_data_location {
> > +		    DW_OP_push_object_address
> > +		    DW_OP_deref
> > +		} SPECIAL_expr}
> > +		{DW_AT_allocated {
> > +		    DW_OP_push_object_address
> > +		    DW_OP_deref_size ${voidp_size}
> > +		    DW_OP_lit0
> > +		    DW_OP_ne
> 
> Can't this expression just be `DW_OP_lit0`, to make the array always
> unallocated?

Yes.  I simplified this.

> 
> I also looked at the fix itself, I can't really claim to understand
> it perfectly, but nothing stood out as wrong to me, so LGTM.

Thanks, I went ahead and merged this.

Thanks,
Andrew