[PATCH v2] Fixed core dump from incorrect location expression on bad dwarfs

[PATCH v2] Fixed core dump from incorrect location expression on bad dwarfs

[Bruno Larsen]

Some incorrectly constructed location expressions in inheritance members
of a class could lead to a core dump, or printing garbage instead of a
correct value. The added test case always core dumped during my
testing, but it could be changed to print garbage by changing the
location expression on the exp file to not include DW_OP_stack_value,
but just use a large constant value.

The solution is, when copying contents of a value struct, check if
contents will actually be copied (ie length > 0) and if the
offset of the copied member is greater than the size of the struct
itself, raising an error if so.
---

Changes for V2:
* fixed compilation issue, brought up by ahajkova

---
 .../dw2-inheritance-stack-location.c          |  50 +++++
 .../dw2-inheritance-stack-location.exp        | 185 ++++++++++++++++++
 gdb/value.c                                   |   5 +
 3 files changed, 240 insertions(+)
 create mode 100644 gdb/testsuite/gdb.dwarf2/dw2-inheritance-stack-location.c
 create mode 100644 gdb/testsuite/gdb.dwarf2/dw2-inheritance-stack-location.exp

diff --git a/gdb/testsuite/gdb.dwarf2/dw2-inheritance-stack-location.c b/gdb/testsuite/gdb.dwarf2/dw2-inheritance-stack-location.c
new file mode 100644
index 00000000000..ef661af9d92
--- /dev/null
+++ b/gdb/testsuite/gdb.dwarf2/dw2-inheritance-stack-location.c
@@ -0,0 +1,50 @@
+/* Copyright (C) 2018-2021 Free Software Foundation, Inc.
+
+   This file is part of GDB.
+
+   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/>.  */
+
+typedef struct A {
+    long a;
+    long x;
+} A;
+
+struct A g_A = {3, 4};
+
+typedef struct B {
+    long a;
+    long x1;
+    long b;
+} B;
+
+struct B g_B = { 8, 9, 10 };
+
+B
+foo ()
+{						/* foo prologue */
+  asm ("foo_label: .globl foo_label");
+  return g_B;					/* foo return */
+}						/* foo end */
+
+int
+main (void)
+{						/* main prologue */
+  B v;
+  asm ("main_label: .globl main_label");
+
+  v = foo ();					/* main foo call */
+
+  asm ("main_label2: .globl main_label2");
+  return 0;					/* main return */
+}						/* main end */
diff --git a/gdb/testsuite/gdb.dwarf2/dw2-inheritance-stack-location.exp b/gdb/testsuite/gdb.dwarf2/dw2-inheritance-stack-location.exp
new file mode 100644
index 00000000000..5b8aba3ef6c
--- /dev/null
+++ b/gdb/testsuite/gdb.dwarf2/dw2-inheritance-stack-location.exp
@@ -0,0 +1,185 @@
+# Copyright 2018-2021 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/>.
+
+load_lib dwarf.exp
+
+# This test can only be run on targets which support DWARF-2 and use gas.
+if ![dwarf2_support] {
+    return 0
+}
+
+# We'll place the output of Dwarf::assemble in pr28030.S.
+standard_testfile .c .S
+
+# ${testfile} is now "pr28030".  srcfile2 is "pr28030.S".
+set executable ${testfile}
+set asm_file [standard_output_file ${srcfile2}]
+
+# We need to know the size of integer and address types in order
+# to write some of the debugging info we'd like to generate.
+#
+# For that, we ask GDB by debugging our pr28030 program.
+# Any program would do, but since we already have pr28030
+# specifically for this testcase, might as well use that.
+if [prepare_for_testing "failed to prepare" ${testfile} ${srcfile}] {
+    return -1
+}
+set long_size [get_sizeof "long" -1]
+# gdb always assumes references are implemented as pointers.
+set addr_size [get_sizeof "void *" -1]
+set struct_A_size [get_sizeof "g_A" 4]
+set struct_B_size [get_sizeof "g_B" 4]
+
+# Create the DWARF.
+Dwarf::assemble ${asm_file} {
+    global srcdir subdir srcfile srcfile2
+    global long_size addr_size struct_A_size struct_B_size
+    declare_labels L
+
+    cu {} {
+	DW_TAG_compile_unit {
+	    {DW_AT_language @DW_LANG_C_plus_plus}
+	    {name pr28030.c}
+	    {stmt_list $L DW_FORM_sec_offset}
+        } {
+	    declare_labels int_label class_A_label class_B_label
+
+	    int_label: DW_TAG_base_type {
+		{DW_AT_byte_size ${long_size} DW_FORM_udata}
+		{DW_AT_encoding @DW_ATE_signed}
+		{DW_AT_name "int"}
+	    }
+
+	    class_A_label: DW_TAG_class_type {
+		{DW_AT_name "A"}
+		{DW_AT_byte_size $struct_A_size DW_FORM_sdata}
+	    } {
+		DW_TAG_member {
+		    {DW_AT_name "a"}
+		    {DW_AT_type :$int_label}
+		    {DW_AT_data_member_location 0*$long_size DW_FORM_udata}
+		}
+		DW_TAG_member {
+		    {DW_AT_name "x"}
+		    {DW_AT_type :$int_label}
+		    {DW_AT_data_member_location 1*$long_size DW_FORM_udata}
+		}
+	    }
+
+	    class_B_label: DW_TAG_class_type {
+		{DW_AT_name "B"}
+		{DW_AT_byte_size $struct_B_size DW_FORM_sdata}
+	    } {
+		DW_TAG_inheritance {
+		    {DW_AT_type :$class_A_label}
+		    {DW_AT_data_member_location {
+			DW_OP_constu 0
+			DW_OP_plus
+			DW_OP_stack_value } SPECIAL_expr}
+		    {DW_AT_accessibility 1 DW_FORM_data1}
+		}
+		DW_TAG_member {
+		    {DW_AT_name "a"}
+		    {DW_AT_type :$int_label}
+		    {DW_AT_data_member_location 2*$long_size DW_FORM_udata}
+		}
+	    }
+
+	    DW_TAG_variable {
+		{DW_AT_name "g_A"}
+		{DW_AT_type :$class_A_label}
+		{DW_AT_external 1 flag}
+		{DW_AT_location {DW_OP_addr [gdb_target_symbol "g_A"]} SPECIAL_expr}
+	    }
+
+	    DW_TAG_variable {
+		{DW_AT_name "g_B"}
+		{DW_AT_type :$class_B_label}
+		{DW_AT_external 1 flag}
+		{DW_AT_location {DW_OP_addr [gdb_target_symbol "g_B"]} SPECIAL_expr}
+	    }
+
+	    DW_TAG_subprogram {
+		{MACRO_AT_func { "foo" }}
+		{DW_AT_type :${class_B_label}}
+		{DW_AT_external 1 flag}
+	    }
+	    DW_TAG_subprogram {
+		{MACRO_AT_func { "main" }}
+		{DW_AT_type :${int_label}}
+		{DW_AT_external 1 DW_FORM_flag}
+	    }
+	}
+    }
+
+    lines {version 2} L {
+	include_dir "${srcdir}/${subdir}"
+	file_name "$srcfile" 1
+
+	lassign [function_range main [list ${srcdir}/${subdir}/$srcfile]] \
+	    main_start main_len
+	set main_end "$main_start + $main_len"
+	lassign [function_range foo [list ${srcdir}/${subdir}/$srcfile]] \
+	    foo_start foo_len
+	set foo_end "$foo_start + $foo_len"
+
+	# Generate a line table program.  An attempt was made to make it
+	# reasonably accurate as it made debugging the test case easier.
+	program {
+	    {DW_LNE_set_address $main_start}
+	    {line [gdb_get_line_number "main prologue"]}
+	    {DW_LNS_copy}
+	    {DW_LNE_set_address main_label}
+	    {line [gdb_get_line_number "main foo call"]}
+	    {DW_LNS_copy}
+	    {DW_LNE_set_address main_label2}
+	    {line [gdb_get_line_number "main return"]}
+	    {DW_LNS_copy}
+	    {DW_LNE_set_address $main_end}
+	    {line [expr [gdb_get_line_number "main end"] + 1]}
+	    {DW_LNS_copy}
+	    {DW_LNE_end_sequence}
+
+	    {DW_LNE_set_address $foo_start}
+	    {line [gdb_get_line_number "foo prologue"]}
+	    {DW_LNS_copy}
+	    {DW_LNE_set_address foo_label}
+	    {line [gdb_get_line_number "foo return"]}
+	    {DW_LNS_copy}
+	    {line [gdb_get_line_number "foo end"]}
+	    {DW_LNS_copy}
+	    {DW_LNE_set_address $foo_end}
+	    {DW_LNS_advance_line 1}
+	    {DW_LNS_copy}
+	    {DW_LNE_end_sequence}
+
+	}
+    }
+}
+
+if [prepare_for_testing "failed to prepare" ${executable} [list ${asm_file} ${srcfile}] {}] {
+    return -1
+}
+
+if ![runto_main] {
+    return -1
+}
+
+gdb_test "step" "foo .. at .* foo end.*" "step into foo"
+gdb_test "finish" "= {<A> = {a = <error reading variable>" "finish out of foo"
+
+if ![runto_main] {
+    return -1
+}
diff --git a/gdb/value.c b/gdb/value.c
index 2cbaadc3641..4de3074157a 100644
--- a/gdb/value.c
+++ b/gdb/value.c
@@ -1328,6 +1328,11 @@ value_contents_copy_raw (struct value *dst, LONGEST dst_offset,
 					     TARGET_CHAR_BIT * dst_offset,
 					     TARGET_CHAR_BIT * length));
 
+  if ((length > 0)
+      && (dst_offset >= TYPE_LENGTH (value_enclosing_type (dst))
+      || src_offset >= TYPE_LENGTH (value_enclosing_type (src))))
+      error (_("overly large offset found while copying value."));
+
   /* Copy the data.  */
   memcpy (value_contents_all_raw (dst) + dst_offset * unit_size,
 	  value_contents_all_raw (src) + src_offset * unit_size,
-- 
2.27.0

[PING] [PATCH v2] Fixed core dump from incorrect location expression on bad dwarfs

[Bruno Larsen]

ping

On 8/30/21 5:10 PM, Bruno Larsen wrote:
> Some incorrectly constructed location expressions in inheritance members
> of a class could lead to a core dump, or printing garbage instead of a
> correct value. The added test case always core dumped during my
> testing, but it could be changed to print garbage by changing the
> location expression on the exp file to not include DW_OP_stack_value,
> but just use a large constant value.
> 
> The solution is, when copying contents of a value struct, check if
> contents will actually be copied (ie length > 0) and if the
> offset of the copied member is greater than the size of the struct
> itself, raising an error if so.
> ---
> 
> Changes for V2:
> * fixed compilation issue, brought up by ahajkova
> 
> ---
>   .../dw2-inheritance-stack-location.c          |  50 +++++
>   .../dw2-inheritance-stack-location.exp        | 185 ++++++++++++++++++
>   gdb/value.c                                   |   5 +
>   3 files changed, 240 insertions(+)
>   create mode 100644 gdb/testsuite/gdb.dwarf2/dw2-inheritance-stack-location.c
>   create mode 100644 gdb/testsuite/gdb.dwarf2/dw2-inheritance-stack-location.exp
> 
> diff --git a/gdb/testsuite/gdb.dwarf2/dw2-inheritance-stack-location.c b/gdb/testsuite/gdb.dwarf2/dw2-inheritance-stack-location.c
> new file mode 100644
> index 00000000000..ef661af9d92
> --- /dev/null
> +++ b/gdb/testsuite/gdb.dwarf2/dw2-inheritance-stack-location.c
> @@ -0,0 +1,50 @@
> +/* Copyright (C) 2018-2021 Free Software Foundation, Inc.
> +
> +   This file is part of GDB.
> +
> +   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/>.  */
> +
> +typedef struct A {
> +    long a;
> +    long x;
> +} A;
> +
> +struct A g_A = {3, 4};
> +
> +typedef struct B {
> +    long a;
> +    long x1;
> +    long b;
> +} B;
> +
> +struct B g_B = { 8, 9, 10 };
> +
> +B
> +foo ()
> +{						/* foo prologue */
> +  asm ("foo_label: .globl foo_label");
> +  return g_B;					/* foo return */
> +}						/* foo end */
> +
> +int
> +main (void)
> +{						/* main prologue */
> +  B v;
> +  asm ("main_label: .globl main_label");
> +
> +  v = foo ();					/* main foo call */
> +
> +  asm ("main_label2: .globl main_label2");
> +  return 0;					/* main return */
> +}						/* main end */
> diff --git a/gdb/testsuite/gdb.dwarf2/dw2-inheritance-stack-location.exp b/gdb/testsuite/gdb.dwarf2/dw2-inheritance-stack-location.exp
> new file mode 100644
> index 00000000000..5b8aba3ef6c
> --- /dev/null
> +++ b/gdb/testsuite/gdb.dwarf2/dw2-inheritance-stack-location.exp
> @@ -0,0 +1,185 @@
> +# Copyright 2018-2021 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/>.
> +
> +load_lib dwarf.exp
> +
> +# This test can only be run on targets which support DWARF-2 and use gas.
> +if ![dwarf2_support] {
> +    return 0
> +}
> +
> +# We'll place the output of Dwarf::assemble in pr28030.S.
> +standard_testfile .c .S
> +
> +# ${testfile} is now "pr28030".  srcfile2 is "pr28030.S".
> +set executable ${testfile}
> +set asm_file [standard_output_file ${srcfile2}]
> +
> +# We need to know the size of integer and address types in order
> +# to write some of the debugging info we'd like to generate.
> +#
> +# For that, we ask GDB by debugging our pr28030 program.
> +# Any program would do, but since we already have pr28030
> +# specifically for this testcase, might as well use that.
> +if [prepare_for_testing "failed to prepare" ${testfile} ${srcfile}] {
> +    return -1
> +}
> +set long_size [get_sizeof "long" -1]
> +# gdb always assumes references are implemented as pointers.
> +set addr_size [get_sizeof "void *" -1]
> +set struct_A_size [get_sizeof "g_A" 4]
> +set struct_B_size [get_sizeof "g_B" 4]
> +
> +# Create the DWARF.
> +Dwarf::assemble ${asm_file} {
> +    global srcdir subdir srcfile srcfile2
> +    global long_size addr_size struct_A_size struct_B_size
> +    declare_labels L
> +
> +    cu {} {
> +	DW_TAG_compile_unit {
> +	    {DW_AT_language @DW_LANG_C_plus_plus}
> +	    {name pr28030.c}
> +	    {stmt_list $L DW_FORM_sec_offset}
> +        } {
> +	    declare_labels int_label class_A_label class_B_label
> +
> +	    int_label: DW_TAG_base_type {
> +		{DW_AT_byte_size ${long_size} DW_FORM_udata}
> +		{DW_AT_encoding @DW_ATE_signed}
> +		{DW_AT_name "int"}
> +	    }
> +
> +	    class_A_label: DW_TAG_class_type {
> +		{DW_AT_name "A"}
> +		{DW_AT_byte_size $struct_A_size DW_FORM_sdata}
> +	    } {
> +		DW_TAG_member {
> +		    {DW_AT_name "a"}
> +		    {DW_AT_type :$int_label}
> +		    {DW_AT_data_member_location 0*$long_size DW_FORM_udata}
> +		}
> +		DW_TAG_member {
> +		    {DW_AT_name "x"}
> +		    {DW_AT_type :$int_label}
> +		    {DW_AT_data_member_location 1*$long_size DW_FORM_udata}
> +		}
> +	    }
> +
> +	    class_B_label: DW_TAG_class_type {
> +		{DW_AT_name "B"}
> +		{DW_AT_byte_size $struct_B_size DW_FORM_sdata}
> +	    } {
> +		DW_TAG_inheritance {
> +		    {DW_AT_type :$class_A_label}
> +		    {DW_AT_data_member_location {
> +			DW_OP_constu 0
> +			DW_OP_plus
> +			DW_OP_stack_value } SPECIAL_expr}
> +		    {DW_AT_accessibility 1 DW_FORM_data1}
> +		}
> +		DW_TAG_member {
> +		    {DW_AT_name "a"}
> +		    {DW_AT_type :$int_label}
> +		    {DW_AT_data_member_location 2*$long_size DW_FORM_udata}
> +		}
> +	    }
> +
> +	    DW_TAG_variable {
> +		{DW_AT_name "g_A"}
> +		{DW_AT_type :$class_A_label}
> +		{DW_AT_external 1 flag}
> +		{DW_AT_location {DW_OP_addr [gdb_target_symbol "g_A"]} SPECIAL_expr}
> +	    }
> +
> +	    DW_TAG_variable {
> +		{DW_AT_name "g_B"}
> +		{DW_AT_type :$class_B_label}
> +		{DW_AT_external 1 flag}
> +		{DW_AT_location {DW_OP_addr [gdb_target_symbol "g_B"]} SPECIAL_expr}
> +	    }
> +
> +	    DW_TAG_subprogram {
> +		{MACRO_AT_func { "foo" }}
> +		{DW_AT_type :${class_B_label}}
> +		{DW_AT_external 1 flag}
> +	    }
> +	    DW_TAG_subprogram {
> +		{MACRO_AT_func { "main" }}
> +		{DW_AT_type :${int_label}}
> +		{DW_AT_external 1 DW_FORM_flag}
> +	    }
> +	}
> +    }
> +
> +    lines {version 2} L {
> +	include_dir "${srcdir}/${subdir}"
> +	file_name "$srcfile" 1
> +
> +	lassign [function_range main [list ${srcdir}/${subdir}/$srcfile]] \
> +	    main_start main_len
> +	set main_end "$main_start + $main_len"
> +	lassign [function_range foo [list ${srcdir}/${subdir}/$srcfile]] \
> +	    foo_start foo_len
> +	set foo_end "$foo_start + $foo_len"
> +
> +	# Generate a line table program.  An attempt was made to make it
> +	# reasonably accurate as it made debugging the test case easier.
> +	program {
> +	    {DW_LNE_set_address $main_start}
> +	    {line [gdb_get_line_number "main prologue"]}
> +	    {DW_LNS_copy}
> +	    {DW_LNE_set_address main_label}
> +	    {line [gdb_get_line_number "main foo call"]}
> +	    {DW_LNS_copy}
> +	    {DW_LNE_set_address main_label2}
> +	    {line [gdb_get_line_number "main return"]}
> +	    {DW_LNS_copy}
> +	    {DW_LNE_set_address $main_end}
> +	    {line [expr [gdb_get_line_number "main end"] + 1]}
> +	    {DW_LNS_copy}
> +	    {DW_LNE_end_sequence}
> +
> +	    {DW_LNE_set_address $foo_start}
> +	    {line [gdb_get_line_number "foo prologue"]}
> +	    {DW_LNS_copy}
> +	    {DW_LNE_set_address foo_label}
> +	    {line [gdb_get_line_number "foo return"]}
> +	    {DW_LNS_copy}
> +	    {line [gdb_get_line_number "foo end"]}
> +	    {DW_LNS_copy}
> +	    {DW_LNE_set_address $foo_end}
> +	    {DW_LNS_advance_line 1}
> +	    {DW_LNS_copy}
> +	    {DW_LNE_end_sequence}
> +
> +	}
> +    }
> +}
> +
> +if [prepare_for_testing "failed to prepare" ${executable} [list ${asm_file} ${srcfile}] {}] {
> +    return -1
> +}
> +
> +if ![runto_main] {
> +    return -1
> +}
> +
> +gdb_test "step" "foo .. at .* foo end.*" "step into foo"
> +gdb_test "finish" "= {<A> = {a = <error reading variable>" "finish out of foo"
> +
> +if ![runto_main] {
> +    return -1
> +}
> diff --git a/gdb/value.c b/gdb/value.c
> index 2cbaadc3641..4de3074157a 100644
> --- a/gdb/value.c
> +++ b/gdb/value.c
> @@ -1328,6 +1328,11 @@ value_contents_copy_raw (struct value *dst, LONGEST dst_offset,
>   					     TARGET_CHAR_BIT * dst_offset,
>   					     TARGET_CHAR_BIT * length));
>   
> +  if ((length > 0)
> +      && (dst_offset >= TYPE_LENGTH (value_enclosing_type (dst))
> +      || src_offset >= TYPE_LENGTH (value_enclosing_type (src))))
> +      error (_("overly large offset found while copying value."));
> +
>     /* Copy the data.  */
>     memcpy (value_contents_all_raw (dst) + dst_offset * unit_size,
>   	  value_contents_all_raw (src) + src_offset * unit_size,
> 

Re: [PATCH v2] Fixed core dump from incorrect location expression on bad dwarfs

[Simon Marchi]

On 2021-08-30 4:10 p.m., Bruno Larsen via Gdb-patches wrote:
> Some incorrectly constructed location expressions in inheritance members
> of a class could lead to a core dump, or printing garbage instead of a
> correct value. The added test case always core dumped during my
> testing, but it could be changed to print garbage by changing the
> location expression on the exp file to not include DW_OP_stack_value,
> but just use a large constant value.
> 
> The solution is, when copying contents of a value struct, check if
> contents will actually be copied (ie length > 0) and if the
> offset of the copied member is greater than the size of the struct
> itself, raising an error if so.

Thanks for the test, that makes it really easy to reproduce and
investigate.

I think the way you fix it only hides the real problem.  The
particularity of your debug info appears to be that a DWARF expression
is used in the DW_TAG_inheritance's DW_AT_data_member_location
attribute, instead of a constant (for non-virtual inheritance, that is).

Right now I'm stopped here:

    #0  0x000055892cf7023d in type::field (this=0x621000190550, idx=0) at /home/simark/src/binutils-gdb/gdb/gdbtypes.h:973
    #1  0x000055892d9ced8e in gnuv3_baseclass_offset (type=0x621000190550, index=0, valaddr=0x6030001fbbb0 "\b", embedded_offset=0, address=0x0, val=0x6110000aba80) at /home/simark/src/binutils-gdb/gdb/gnu-v3-abi.c:464
    #2  0x000055892d56a368 in baseclass_offset (type=0x621000190550, index=0, valaddr=0x6030001fbbb0 "\b", embedded_offset=0, address=0x0, val=0x6110000aba80) at /home/simark/src/binutils-gdb/gdb/cp-abi.c:78
    #3  0x000055892d5998fe in cp_print_value (val=0x6110000aba80, stream=0x7ffde7bd1270, recurse=1, options=0x7ffde7bd0da0, dont_print_vb=0x0) at /home/simark/src/binutils-gdb/gdb/cp-valprint.c:436
    #4  0x000055892d59718e in cp_print_value_fields (val=0x6110000aba80, stream=0x7ffde7bd1270, recurse=0, options=0x7ffde7bd0da0, dont_print_vb=0x0, dont_print_statmem=0) at /home/simark/src/binutils-gdb/gdb/cp-valprint.c:161

... as GDB is trying to get the offset of base class A in class B.  GDB
represents base classes as fields, so frame #0 is getting the field
representing base class A of class B.  Here, $13 represents that struct
field:

    $13 = {
      loc = {
        bitpos = 107820860638704,
        enumval = 107820860638704,
        physaddr = 0x6210001905f0,
        physname = 0x6210001905f0 "L!\v",
        dwarf_block = 0x6210001905f0
      },
      artificial = 0,
      loc_kind = FIELD_LOC_KIND_DWARF_BLOCK,
      bitsize = 0,
      m_type = 0x621000190380,
      name = 0x62100016aa50 "A"
    }

loc_kind being FIELD_LOC_KIND_DWARF_BLOCK, the active field of the `loc`
union is `dwarf_block`.  It points to the dwarf_block (the expression)
to evaluate to get the offset, created in handle_data_member_location,
in dwarf2/read.c.

If we look at code of frame 1, in gnuv3_baseclass_offset:

  /* If it isn't a virtual base, this is easy.  The offset is in the
     type definition.  */
  if (!BASETYPE_VIA_VIRTUAL (type, index))
    return TYPE_BASECLASS_BITPOS (type, index) / 8;

This code accesses blindly the `bitpos` field of the union.  So the
value it returns for the offset of base class A in class B is actually
the pointer to the dwarf_block, cast to an int (and divided by 8), and
therefore bogus.  That seems to end up creating an impossible
value_embedded_offset, and is the cause for the out of bounds read you
see.

Simon

Re: [PATCH v2] Fixed core dump from incorrect location expression on bad dwarfs

[Simon Marchi]

On 2021-09-23 10:45 p.m., Simon Marchi via Gdb-patches wrote:
> On 2021-08-30 4:10 p.m., Bruno Larsen via Gdb-patches wrote:
>> Some incorrectly constructed location expressions in inheritance members
>> of a class could lead to a core dump, or printing garbage instead of a
>> correct value. The added test case always core dumped during my
>> testing, but it could be changed to print garbage by changing the
>> location expression on the exp file to not include DW_OP_stack_value,
>> but just use a large constant value.
>>
>> The solution is, when copying contents of a value struct, check if
>> contents will actually be copied (ie length > 0) and if the
>> offset of the copied member is greater than the size of the struct
>> itself, raising an error if so.
> 
> Thanks for the test, that makes it really easy to reproduce and
> investigate.
> 
> I think the way you fix it only hides the real problem.  The
> particularity of your debug info appears to be that a DWARF expression
> is used in the DW_TAG_inheritance's DW_AT_data_member_location
> attribute, instead of a constant (for non-virtual inheritance, that is).
> 
> Right now I'm stopped here:
> 
>     #0  0x000055892cf7023d in type::field (this=0x621000190550, idx=0) at /home/simark/src/binutils-gdb/gdb/gdbtypes.h:973
>     #1  0x000055892d9ced8e in gnuv3_baseclass_offset (type=0x621000190550, index=0, valaddr=0x6030001fbbb0 "\b", embedded_offset=0, address=0x0, val=0x6110000aba80) at /home/simark/src/binutils-gdb/gdb/gnu-v3-abi.c:464
>     #2  0x000055892d56a368 in baseclass_offset (type=0x621000190550, index=0, valaddr=0x6030001fbbb0 "\b", embedded_offset=0, address=0x0, val=0x6110000aba80) at /home/simark/src/binutils-gdb/gdb/cp-abi.c:78
>     #3  0x000055892d5998fe in cp_print_value (val=0x6110000aba80, stream=0x7ffde7bd1270, recurse=1, options=0x7ffde7bd0da0, dont_print_vb=0x0) at /home/simark/src/binutils-gdb/gdb/cp-valprint.c:436
>     #4  0x000055892d59718e in cp_print_value_fields (val=0x6110000aba80, stream=0x7ffde7bd1270, recurse=0, options=0x7ffde7bd0da0, dont_print_vb=0x0, dont_print_statmem=0) at /home/simark/src/binutils-gdb/gdb/cp-valprint.c:161
> 
> ... as GDB is trying to get the offset of base class A in class B.  GDB
> represents base classes as fields, so frame #0 is getting the field
> representing base class A of class B.  Here, $13 represents that struct
> field:
> 
>     $13 = {
>       loc = {
>         bitpos = 107820860638704,
>         enumval = 107820860638704,
>         physaddr = 0x6210001905f0,
>         physname = 0x6210001905f0 "L!\v",
>         dwarf_block = 0x6210001905f0
>       },
>       artificial = 0,
>       loc_kind = FIELD_LOC_KIND_DWARF_BLOCK,
>       bitsize = 0,
>       m_type = 0x621000190380,
>       name = 0x62100016aa50 "A"
>     }
> 
> loc_kind being FIELD_LOC_KIND_DWARF_BLOCK, the active field of the `loc`
> union is `dwarf_block`.  It points to the dwarf_block (the expression)
> to evaluate to get the offset, created in handle_data_member_location,
> in dwarf2/read.c.
> 
> If we look at code of frame 1, in gnuv3_baseclass_offset:
> 
>   /* If it isn't a virtual base, this is easy.  The offset is in the
>      type definition.  */
>   if (!BASETYPE_VIA_VIRTUAL (type, index))
>     return TYPE_BASECLASS_BITPOS (type, index) / 8;
> 
> This code accesses blindly the `bitpos` field of the union.  So the
> value it returns for the offset of base class A in class B is actually
> the pointer to the dwarf_block, cast to an int (and divided by 8), and
> therefore bogus.  That seems to end up creating an impossible
> value_embedded_offset, and is the cause for the out of bounds read you
> see.

I made a patch that adds some asserts in struct field, when getting the
location, to ensure we don't get the location as a wrong kind:

  https://sourceware.org/pipermail/gdb-patches/2021-September/182136.html

It catches the problem when running your test:

    313 finish^M
    314 Run till exit from #0  foo () at /home/simark/src/binutils-gdb/gdb/testsuite/gdb.dwarf2/dw2-inheritance-stack-location.c:38^M
    315 main () at /home/simark/src/binutils-gdb/gdb/testsuite/gdb.dwarf2/dw2-inheritance-stack-location.c:49^M
    316 49        return 0;                                     /* main return */^M
    317 Value returned is $1 = /home/simark/src/binutils-gdb/gdb/gdbtypes.h:667: internal-error: LONGEST field::loc_bitpos() const: Assertion `m_loc_kind == FIELD_LOC_KIND_BITPOS' failed.^M

So I think this will be the root issue to fix.

Simon

Re: [PATCH v2] Fixed core dump from incorrect location expression on bad dwarfs

[Bruno Larsen]

On 9/24/21 1:12 AM, Simon Marchi wrote:
> On 2021-09-23 10:45 p.m., Simon Marchi via Gdb-patches wrote:
>> On 2021-08-30 4:10 p.m., Bruno Larsen via Gdb-patches wrote:
>>> Some incorrectly constructed location expressions in inheritance members
>>> of a class could lead to a core dump, or printing garbage instead of a
>>> correct value. The added test case always core dumped during my
>>> testing, but it could be changed to print garbage by changing the
>>> location expression on the exp file to not include DW_OP_stack_value,
>>> but just use a large constant value.
>>>
>>> The solution is, when copying contents of a value struct, check if
>>> contents will actually be copied (ie length > 0) and if the
>>> offset of the copied member is greater than the size of the struct
>>> itself, raising an error if so.
>>
>> Thanks for the test, that makes it really easy to reproduce and
>> investigate.
>>
>> I think the way you fix it only hides the real problem.  The
>> particularity of your debug info appears to be that a DWARF expression
>> is used in the DW_TAG_inheritance's DW_AT_data_member_location
>> attribute, instead of a constant (for non-virtual inheritance, that is).
>>
>> Right now I'm stopped here:
>>
>>      #0  0x000055892cf7023d in type::field (this=0x621000190550, idx=0) at /home/simark/src/binutils-gdb/gdb/gdbtypes.h:973
>>      #1  0x000055892d9ced8e in gnuv3_baseclass_offset (type=0x621000190550, index=0, valaddr=0x6030001fbbb0 "\b", embedded_offset=0, address=0x0, val=0x6110000aba80) at /home/simark/src/binutils-gdb/gdb/gnu-v3-abi.c:464
>>      #2  0x000055892d56a368 in baseclass_offset (type=0x621000190550, index=0, valaddr=0x6030001fbbb0 "\b", embedded_offset=0, address=0x0, val=0x6110000aba80) at /home/simark/src/binutils-gdb/gdb/cp-abi.c:78
>>      #3  0x000055892d5998fe in cp_print_value (val=0x6110000aba80, stream=0x7ffde7bd1270, recurse=1, options=0x7ffde7bd0da0, dont_print_vb=0x0) at /home/simark/src/binutils-gdb/gdb/cp-valprint.c:436
>>      #4  0x000055892d59718e in cp_print_value_fields (val=0x6110000aba80, stream=0x7ffde7bd1270, recurse=0, options=0x7ffde7bd0da0, dont_print_vb=0x0, dont_print_statmem=0) at /home/simark/src/binutils-gdb/gdb/cp-valprint.c:161
>>
>> ... as GDB is trying to get the offset of base class A in class B.  GDB
>> represents base classes as fields, so frame #0 is getting the field
>> representing base class A of class B.  Here, $13 represents that struct
>> field:
>>
>>      $13 = {
>>        loc = {
>>          bitpos = 107820860638704,
>>          enumval = 107820860638704,
>>          physaddr = 0x6210001905f0,
>>          physname = 0x6210001905f0 "L!\v",
>>          dwarf_block = 0x6210001905f0
>>        },
>>        artificial = 0,
>>        loc_kind = FIELD_LOC_KIND_DWARF_BLOCK,
>>        bitsize = 0,
>>        m_type = 0x621000190380,
>>        name = 0x62100016aa50 "A"
>>      }
>>
>> loc_kind being FIELD_LOC_KIND_DWARF_BLOCK, the active field of the `loc`
>> union is `dwarf_block`.  It points to the dwarf_block (the expression)
>> to evaluate to get the offset, created in handle_data_member_location,
>> in dwarf2/read.c.
>>
>> If we look at code of frame 1, in gnuv3_baseclass_offset:
>>
>>    /* If it isn't a virtual base, this is easy.  The offset is in the
>>       type definition.  */
>>    if (!BASETYPE_VIA_VIRTUAL (type, index))
>>      return TYPE_BASECLASS_BITPOS (type, index) / 8;
>>
>> This code accesses blindly the `bitpos` field of the union.  So the
>> value it returns for the offset of base class A in class B is actually
>> the pointer to the dwarf_block, cast to an int (and divided by 8), and
>> therefore bogus.  That seems to end up creating an impossible
>> value_embedded_offset, and is the cause for the out of bounds read you
>> see.
> 
> I made a patch that adds some asserts in struct field, when getting the
> location, to ensure we don't get the location as a wrong kind:
> 
>    https://sourceware.org/pipermail/gdb-patches/2021-September/182136.html
> 
> It catches the problem when running your test:
> 
>      313 finish^M
>      314 Run till exit from #0  foo () at /home/simark/src/binutils-gdb/gdb/testsuite/gdb.dwarf2/dw2-inheritance-stack-location.c:38^M
>      315 main () at /home/simark/src/binutils-gdb/gdb/testsuite/gdb.dwarf2/dw2-inheritance-stack-location.c:49^M
>      316 49        return 0;                                     /* main return */^M
>      317 Value returned is $1 = /home/simark/src/binutils-gdb/gdb/gdbtypes.h:667: internal-error: LONGEST field::loc_bitpos() const: Assertion `m_loc_kind == FIELD_LOC_KIND_BITPOS' failed.^M
> 
> So I think this will be the root issue to fix.
> 
> Simon
> 
First of all, thanks for the review and explanation of what was going on. This will definitely help.

Second, from your explanation, it sounds like I can solve the root cause of the core-dump/assertion by detecting if the non-trivial location expression for the inheritance is accompanied by a DW_AT_virtuality, so my plan would be to check for that as we read the DIE and issue a complain if something like this happens. Does that sound reasonable?

Third, I still think this patch could be useful because if the location expression is incorrect but doesn't trigger that assertion, GDB can end up printing garbage (which would make people blame GDB, rather than looking at their dwarf for incorrect offsets). To test that, you can just change the inheritance DIE to:

DW_TAG_inheritance {
   {DW_AT_type :$class_A_label}
   {DW_AT_data_member_location {DW_OP_constu 9000} SPECIAL_expr}
   {DW_AT_accessibility 1 DW_FORM_data1}
}

--
Cheers!
Bruno

Re: [PATCH v2] Fixed core dump from incorrect location expression on bad dwarfs

[Simon Marchi]

> Second, from your explanation, it sounds like I can solve the root cause of the core-dump/assertion by detecting if the non-trivial location expression for the inheritance is accompanied by a DW_AT_virtuality, so my plan would be to check for that as we read the DIE and issue a complain if something like this happens. Does that sound reasonable?

I don't understand the intent here.  You want to disallow having a
DW_AT_data_member_location with a location expression value, if the
inheritance is not virtual?  I get that location descriptions will
typically be used by DW_AT_data_member_location in the case of virtual
inheritance, but if someone wants to use a location description with
non-virtual inheritance... DWARF doesn't prevent that.  So I think it
should work.  Plus, it shouldn't make the implementation more complex,
all we need is to evaluate the data member location whatever its kind
is.

> Third, I still think this patch could be useful because if the location expression is incorrect but doesn't trigger that assertion, GDB can end up printing garbage (which would make people blame GDB, rather than looking at their dwarf for incorrect offsets). To test that, you can just change the inheritance DIE to:
> 
> DW_TAG_inheritance {
>   {DW_AT_type :$class_A_label}
>   {DW_AT_data_member_location {DW_OP_constu 9000} SPECIAL_expr}
>   {DW_AT_accessibility 1 DW_FORM_data1}

That could be another test case, indeed.

Simon

Re: [PATCH v2] Fixed core dump from incorrect location expression on bad dwarfs

[Bruno Larsen]

On 9/24/21 4:56 PM, Simon Marchi wrote:
>> Second, from your explanation, it sounds like I can solve the root cause of the core-dump/assertion by detecting if the non-trivial location expression for the inheritance is accompanied by a DW_AT_virtuality, so my plan would be to check for that as we read the DIE and issue a complain if something like this happens. Does that sound reasonable?
> 
> I don't understand the intent here.  You want to disallow having a
> DW_AT_data_member_location with a location expression value, if the
> inheritance is not virtual?  I get that location descriptions will
> typically be used by DW_AT_data_member_location in the case of virtual
> inheritance, but if someone wants to use a location description with
> non-virtual inheritance... DWARF doesn't prevent that.  So I think it
> should work.  Plus, it shouldn't make the implementation more complex,
> all we need is to evaluate the data member location whatever its kind
> is.
> 

 From what I read in the DWARF spec, if you have a constant DW_AT_data_member_location, your inheritance is non-virtual, I guess my intent was to turn that A => B into a A <=> B (making it so if you have a non-trivial expression, you must have virtual inheritance), but you're right, there's nothing on the DWARF specs that say it should be so.

This means there is no way (that I can see at least) to fix the root cause or the core dump/assert, which is getting a bogus location for a member of the class, other than adding a validation step somewhere, that checks if the pointers are actually pointing to inside the base class. This patch is the next best thing I could think of, alerting the user in a non-destructive manner.

>> Third, I still think this patch could be useful because if the location expression is incorrect but doesn't trigger that assertion, GDB can end up printing garbage (which would make people blame GDB, rather than looking at their dwarf for incorrect offsets). To test that, you can just change the inheritance DIE to:
>>
>> DW_TAG_inheritance {
>>    {DW_AT_type :$class_A_label}
>>    {DW_AT_data_member_location {DW_OP_constu 9000} SPECIAL_expr}
>>    {DW_AT_accessibility 1 DW_FORM_data1}
> 
> That could be another test case, indeed.

Great, if I get a v3 figured out, I'll add this to the patch set.

--
Cheers,
Bruno Larsen  

Re: [PATCH v2] Fixed core dump from incorrect location expression on bad dwarfs

[Simon Marchi]

On 2021-09-24 16:19, Bruno Larsen wrote:
> From what I read in the DWARF spec, if you have a constant
> DW_AT_data_member_location, your inheritance is non-virtual, I guess
> my intent was to turn that A => B into a A <=> B (making it so if you
> have a non-trivial expression, you must have virtual inheritance), but
> you're right, there's nothing on the DWARF specs that say it should be
> so.

Indeed, all I see is this non-normative text:

    For a C++ virtual base, the data member location attribute will
    usually consist of a non-trivial location description.

The virtuality of base classes is tracked separately, so there doesn't
seem to be a need to tie inheritance virtuality and the location kind.

> This means there is no way (that I can see at least) to fix the root
> cause or the core dump/assert, which is getting a bogus location for a
> member of the class, other than adding a validation step somewhere,
> that checks if the pointers are actually pointing to inside the base
> class. This patch is the next best thing I could think of, alerting
> the user in a non-destructive manner.

Well, can't we teach gnuv3_baseclass_offset to evaluate offsets given as
location descriptions even if the inheritance is non-virtual?  It seems
to be like that would be a good start.

But I think you are right, we probably need some validation / bailing
out somewhere to make sure that the offset makes sense (whether that is
in value_contents_copy_raw as you have done, or somehwere else).

>>> Third, I still think this patch could be useful because if the
>>> location expression is incorrect but doesn't trigger that assertion,
>>> GDB can end up printing garbage (which would make people blame GDB,
>>> rather than looking at their dwarf for incorrect offsets). To test
>>> that, you can just change the inheritance DIE to:
>>>
>>> DW_TAG_inheritance { {DW_AT_type :$class_A_label}
>>> {DW_AT_data_member_location {DW_OP_constu 9000} SPECIAL_expr}
>>> {DW_AT_accessibility 1 DW_FORM_data1}
>>
>> That could be another test case, indeed.
> 
> Great, if I get a v3 figured out, I'll add this to the patch set.

Thanks!

Simon

Re: [PATCH v2] Fixed core dump from incorrect location expression on bad dwarfs

[Bruno Larsen]

On 9/24/21 5:59 PM, Simon Marchi wrote:
> On 2021-09-24 16:19, Bruno Larsen wrote:
>>  From what I read in the DWARF spec, if you have a constant
>> DW_AT_data_member_location, your inheritance is non-virtual, I guess
>> my intent was to turn that A => B into a A <=> B (making it so if you
>> have a non-trivial expression, you must have virtual inheritance), but
>> you're right, there's nothing on the DWARF specs that say it should be
>> so.
> 
> Indeed, all I see is this non-normative text:
> 
>      For a C++ virtual base, the data member location attribute will
>      usually consist of a non-trivial location description.
> 
> The virtuality of base classes is tracked separately, so there doesn't
> seem to be a need to tie inheritance virtuality and the location kind.
> 
>> This means there is no way (that I can see at least) to fix the root
>> cause or the core dump/assert, which is getting a bogus location for a
>> member of the class, other than adding a validation step somewhere,
>> that checks if the pointers are actually pointing to inside the base
>> class. This patch is the next best thing I could think of, alerting
>> the user in a non-destructive manner.
> 
> Well, can't we teach gnuv3_baseclass_offset to evaluate offsets given as
> location descriptions even if the inheritance is non-virtual?  It seems
> to be like that would be a good start.

I've spent some time looking at this. This sounds like a trivial fix, because we can re-use what's already there, basically.

> 
> But I think you are right, we probably need some validation / bailing
> out somewhere to make sure that the offset makes sense (whether that is
> in value_contents_copy_raw as you have done, or somehwere else).

Another options for validation is adding a trivial validation on gnuv3_baseclass_offset (like the original one from this patch, checking if the offset is greater than the size of the class), which only work for non-virtual inheritance and returning a bitpos location. I am not familiar with virtual inheritance, much less how GDB deals with it internally, so I don't really know a good way to validate it. A quick google tells me that my validation on value_contents_copy_raw could assume that a problem has happened when we are just copying a virtual inheritance member. A better spot to put it could be cp_print_value, since we still have virtuality information at that point, and can validate differently at that point.

The trivial validation fixes the second case, but not the first, so we still need the second validation. This still doesn't sound perfect to me, but I can start work on a v3 at least. Thoughts?

--
Cheers!
Bruno Larsen

Re: [PATCH v2] Fixed core dump from incorrect location expression on bad dwarfs

[Simon Marchi]

On 2021-09-28 13:55, Bruno Larsen wrote:
> On 9/24/21 5:59 PM, Simon Marchi wrote:
>> On 2021-09-24 16:19, Bruno Larsen wrote:
>>>  From what I read in the DWARF spec, if you have a constant
>>>  DW_AT_data_member_location, your inheritance is non-virtual, I
>>>  guess my intent was to turn that A => B into a A <=> B (making it
>>>  so if you have a non-trivial expression, you must have virtual
>>>  inheritance), but you're right, there's nothing on the DWARF specs
>>>  that say it should be so.
>>
>> Indeed, all I see is this non-normative text:
>>
>>      For a C++ virtual base, the data member location attribute will
>>      usually consist of a non-trivial location description.
>>
>> The virtuality of base classes is tracked separately, so there
>> doesn't seem to be a need to tie inheritance virtuality and the
>> location kind.
>>
>>> This means there is no way (that I can see at least) to fix the root
>>> cause or the core dump/assert, which is getting a bogus location for
>>> a member of the class, other than adding a validation step
>>> somewhere, that checks if the pointers are actually pointing to
>>> inside the base class. This patch is the next best thing I could
>>> think of, alerting the user in a non-destructive manner.
>>
>> Well, can't we teach gnuv3_baseclass_offset to evaluate offsets given
>> as location descriptions even if the inheritance is non-virtual?  It
>> seems to be like that would be a good start.
> 
> I've spent some time looking at this. This sounds like a trivial fix,
> because we can re-use what's already there, basically.

Hopefully it is!

>> But I think you are right, we probably need some validation / bailing
>> out somewhere to make sure that the offset makes sense (whether that
>> is in value_contents_copy_raw as you have done, or somehwere else).

> Another options for validation is adding a trivial validation on
> gnuv3_baseclass_offset (like the original one from this patch,
> checking if the offset is greater than the size of the class), which
> only work for non-virtual inheritance and returning a bitpos location.
> I am not familiar with virtual inheritance, much less how GDB deals
> with it internally, so I don't really know a good way to validate it.
> A quick google tells me that my validation on value_contents_copy_raw
> could assume that a problem has happened when we are just copying a
> virtual inheritance member. A better spot to put it could be
> cp_print_value, since we still have virtuality information at that
> point, and can validate differently at that point.
> 
> The trivial validation fixes the second case, but not the first, so we
> still need the second validation. This still doesn't sound perfect to
> me, but I can start work on a v3 at least. Thoughts?

I would suggest adding a gdb_assert in value_contents_copy_raw, that
validate that we are not copying outside the values bounds.  I think
that we can consider any case like this to be a bug in the caller of
value_contents_copy_raw.  Doing it like this (pushing the responsibility
to the caller to check bounds if needed) will lead to more precise error
messages.  In value_contents_copy_raw, you can only give a rather
generic error message, but in the caller it can be a bit more
precise.

Let's see what you come up with in v3.  If you are not sure how to
proceed, then at least write the update test case, and I'll be able to
take a look.

Thanks,

Simon