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

[Simon Marchi via Gdb-patches <gdb-patches@sourceware.org>]

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