Re: [PATCH] gdb/arm: avoid undefined behavior shift when decoding immediate value

[Luis Machado via Gdb-patches <gdb-patches@sourceware.org>]

On 11/9/20 5:49 PM, Simon Marchi via Gdb-patches wrote:
> From: Simon Marchi <simon.marchi@polymtl.ca>
> 
> When loading the code file provided in PR 26828 and GDB is build with
> UBSan, we get:
> 
>      Core was generated by `./Foo'.
>      Program terminated with signal SIGABRT, Aborted.
>      #0  0xb6c3809c in pthread_cond_wait () from /home/simark/build/binutils-gdb/gdb/repo/lib/libpthread.so.0
>      [Current thread is 1 (LWP 29367)]
>      (gdb) bt
>      /home/simark/src/binutils-gdb/gdb/arm-tdep.c:1551:30: runtime error: shift exponent 32 is too large for 32-bit type 'unsigned int'
> 
> The sequence of instructions at pthread_cond_wait, in the
> libpthread.so.0 library, contains this instruction with an immediate
> constant with a "rotate amount" of 0:
> 
>      e24dd044        sub     sp, sp, #68     ; 0x44
> 
> Since it shifts by "32 - rotate amount", arm_analyze_prologue does a 32
> bit shift of a 32 bit type, which is caught by UBSan.
> 
> Fix it by factoring out the decoding of immediates in a new function,
> arm_expand_immediate.
> 
> I added a selftest for arm_analyze_prologue that replicates the
> instruction sequence.  Without the fix, it crashes GDB if it is build
> with --enable-ubsan.
> 
> I initially wanted to re-use the abstract_memory_reader class already in
> arm-tdep.c, used to make arm_process_record testable.  However,
> arm_process_record and arm_analyze_prologue don't use the same kind of
> memory reading functions.  arm_process_record uses a function that
> returns an error status on failure while arm_analyze_prologue uses one
> that throws an exception.  Since i didn't want to introduce any other
> behavior change, I decided to just introduce a separate interface
> (arm_instruction_reader).  It is derived from
> abstract_instruction_reader in aarch64-tdep.c.
> 
> gdb/ChangeLog:
> 
> 	PR gdb/26835
> 	* arm-tdep.c (class arm_instruction_reader): New.
> 	(target_arm_instruction_reader): New.
> 	(arm_analyze_prologue): Add instruction reader parameter and use
> 	it.  Use arm_expand_immediate.
> 	(class target_arm_instruction_reader): Adjust.
> 	(arm_skip_prologue): Adjust.
> 	(arm_expand_immediate): New.
> 	(arm_scan_prologue): Adjust.
> 	(arm_analyze_prologue_test): New.
> 	(class test_arm_instruction_reader): New.
> 
> Change-Id: Ieb1c1799bd66f8c7421384f44f5c2777b578ff8d
> ---
>   gdb/arm-tdep.c | 144 +++++++++++++++++++++++++++++++++++++++++--------
>   1 file changed, 121 insertions(+), 23 deletions(-)
> 
> diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
> index 82e8ec4df49c..7f47654233dd 100644
> --- a/gdb/arm-tdep.c
> +++ b/gdb/arm-tdep.c
> @@ -285,10 +285,34 @@ struct arm_prologue_cache
>     struct trad_frame_saved_reg *saved_regs;
>   };
>   
> -static CORE_ADDR arm_analyze_prologue (struct gdbarch *gdbarch,
> -				       CORE_ADDR prologue_start,
> -				       CORE_ADDR prologue_end,
> -				       struct arm_prologue_cache *cache);
> +namespace {
> +
> +/* Abstract class to read ARM instructions from memory.  */
> +
> +class arm_instruction_reader
> +{
> +public:
> +  /* Read a 4 bytes instruction bytes from memory using the BYTE_ORDER

The comment above reads a bit funny.