Re: [PATCH] gdb: fix out-of-bounds read when printing a pointer with a format

[Simon Marchi <simon.marchi@polymtl.ca>]

On 10/15/26 6:42 AM, Andrew Burgess wrote:
> simon.marchi@polymtl.ca writes:
> 
>> From: Simon Marchi <simon.marchi@polymtl.ca>
>>
>> I stumbled on this while investigating bug 34638 ("Multi-register-reading
>> oddity / anachronism"), but this patch is not otherwise related to that
>> bug.
>>
>> When the value being printed is a pointer, print_scalar_formatted
>> overrides the number of bytes it reads from the value buffer:
>>
>>   if (type->code () == TYPE_CODE_PTR)
>>     len = gdbarch_addr_bit (gdbarch) / TARGET_CHAR_BIT;
>>
>> VALADDR only holds type->length() bytes though, and for a pointer type
>> built by GDB that is `gdbarch_ptr_bit / TARGET_CHAR_BIT` bytes.  On targets
>> where gdbarch_addr_bit is larger than gdbarch_ptr_bit, we therefore read
>> past the end of the buffer.
>>
>> AVR is such a target: gdbarch_ptr_bit is 16 while gdbarch_addr_bit is 32,
>> so printing any pointer with a format reads 4 bytes out of a 2-byte buffer.
>> With a GDB built with AddressSanitizer:
>>
>>     (gdb) p/x $sp
>>     =================================================================
>>     ==433709==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x7c11d1239e33 at pc 0x55eb8e8438f0 bp 0x7ffc17c69050 sp 0x7ffc17c69040
>>
>> It can be reproduced without running a program, simply with a GDB that has
>> AVR support, by forcing the architecture to be AVR:
>>
>>     (gdb) set architecture avr
>>     The target architecture is set to "avr".
>>     (gdb) p/x (void *) 0x1234
>>     =================================================================
>>     ==434356==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x7bbca0e21293 at pc 0x55f5d74d08f0 bp 0x7ffe74cba960 sp 0x7ffe74cba950
>>
>> "info registers SP" on an AVR target goes through the same path, so hits
>> the same crash:
>>
>>     (gdb) c
>>     Continuing.
>>
>>     Breakpoint 1, main () at test.c:16
>>     16        int local = 42;
>>     (gdb) info registers SP
>>     =================================================================
>>     ==432390==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x7b764403b413 at pc 0x55e8ef8628f0 bp 0x7fff13994110 sp 0x7fff13994100
>>
>> The override comes with this comment:
>>
>>   /* If the value is a pointer, and pointers and addresses are not the
>>      same, then at this point, the value's length (in target bytes) is
>>      gdbarch_addr_bit/TARGET_CHAR_BIT, not type->length ().  */
>>
>> This was true when the override was introduced, in commit ef166cf46a1b
>> (2001).  At the time, print_scalar_formatted started by converting the
>> value to a LONGEST:
>>
>>   val_long = unpack_long (type, valaddr);
>>
>> For pointers, unpack_long goes through extract_typed_address, then
>> gdbarch_pointer_to_address, which converts the pointer to a CORE_ADDR
>> gdbarch_addr_bit wide.  On AVR, for instance, data pointer 0x1234 (2 bytes
>> long) becomes address 0x801234 (4 bytes long).  "The value" in the comment
>> refers to this VAL_LONG, not to the bytes at VALADDR.  LEN was then only
>> used to mask VAL_LONG, and the override made sure that the mask did not
>> truncate the converted address to gdbarch_ptr_bit bits.  VALADDR (2 bytes)
>> was never read using LEN (4 bytes), so there was no out-of-bounds read.
>>
>> Commit d9109c8080ac ("Simplify print_scalar_formatted", 2017) changed the
>> function to print the value using the print_*_chars functions, which
>> operate directly on the bytes at VALADDR, and removed the conversion to
>> LONGEST.  From then on, the value printed became the raw pointer
>> representation, type->length() bytes long (2 bytes), not the converted
>> address (4 bytes).  The length override was left in place, and this is
>> when the out-of-bounds read was introduced.
>>
>> Note that commit d9109c8080ac therefore changed how `print (void *) 0x1234`
>> is printed for architectures like AVR, it went from
>>
>>     $1 = 0x801234
>>
>> to just
>>
>>     $1 = 0x1234
>>
>> I don't think it's a mistake in itself, but I am not sure it was fully
>> understood at the time.
>>
>> Since the LEN override no longer has a reason to be, remove it.
>>
>> With LEN now always equal to type->length(), the following block becomes
>> dead code, so remove it.  The comment above it is also a relic of the
>> VAL_LONG time.
>>
>> I started by adding a very simple test to
>> gdb.base/all-architectures.exp.tcl to try to print a formatted pointer with
>> each arch.  I then thought it would be useful to test printing a pointer
>> without formatting, and then the various kinds of pointers (data vs code).
>> The result is the somewhat complicated procedure in this patch.  It did not
>> find additional bugs, but I think it's still useful to have in the
>> testsuite, as it captures what GDB does today.  If a change in the pointer
>> printing code ever changes (notably in the address <-> pointer conversion),
>> we will notice it.
> 
> LGTM.
> 
> Approved-By: Andrew Burgess <aburgess@redhat.com>

Thanks, pushed.

Simon