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

Simon Marchi via Gdb-patches
2020-12-24T14:46:26Z

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