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

[Simon Marchi <simark@simark.ca>]

On 2020-11-11 3:44 p.m., Simon Marchi via Gdb-patches wrote:
> On 2020-11-11 5:00 a.m., Alan Hayward wrote:
>>
>>
>>> On 9 Nov 2020, at 20:49, Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> 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'
>>>
>>
>> This error is because the code is technically wrong - but in reality
>> compilers will plant what we really wanted?
> 
> Well, we don't really know, different things can happen on different
> systems.  I did this program for fun:
> 
>     #include <stdlib.h>
>     #include <stdio.h>
> 
>     int main(int argc, char **argv) {
>             unsigned int foo = 0xabababab;
>             unsigned int shift = atoi(argv[1]);
>             printf("Shifting by %d\n", shift);
>             foo = foo >> shift;
>             printf("%x\n", foo);
>             return 0;
>     }
> 
> Running `./a.out 32`, I get this on AMD64:
> 
>     Shifting by 32
>     abababab
> 
> And I get this on PowerPC and ARM:
> 
>     Shifting by 32
>     0
> 
>>> 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.
>>>
>>
>> Minor nit - it took me a while to figure out that the “it” in “Since it”
>> is the following arm_analyze_prologue and not something in the previous
>> sentence.
> 
> Good point, I swapped "it" and "arm_analyze_prologue" to make sure
> "arm_analyze_prologue" comes before.
> 
>>
>>> 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.
>>
>> I both don’t like this and can’t see a better way of doing it :)
>> So, I’m ok with it
> 
> Same.  I really wanted not to add another class, but I also didn't want
> this simple fix to become a huge refactor.
> 
>>> @@ -1485,6 +1511,26 @@ arm_instruction_restores_sp (unsigned int insn)
>>>   return 0;
>>> }
>>>
>>> +/* Implement immediate value decoding, as described in section A5.2.4
>>> +   (Modified immediate constants in ARM instructions) of the ARM Architecture
>>> +   Reference Manual.  */
>>
>>
>> "of the ARM Architecture Reference Manual (ARMv7-A and ARMv7-R edition)"
>>
>> Just to make sure no one is looking for that section in the v8 doc.
> 
> Done.
> 
>>> @@ -9618,6 +9658,7 @@ vfp - VFP co-processor."),
>>>
>>> #if GDB_SELF_TEST
>>>   selftests::register_test ("arm-record", selftests::arm_record_test);
>>> +  selftests::register_test ("arm_analyze_prologue", selftests::arm_analyze_prologue_test);
>>> #endif
>>
>> Given the size of the tdep files, I wonder if it’s worth putting the tests into separate
>> files, eg arm-tdep-selftests.c
>> (Probably not for this patch though)
> 
> Possibly.  That requires exposing (making non-static) functions that are not usually exposed
> though.
> 
>>> }
>>> @@ -13254,6 +13295,63 @@ arm_record_test (void)
>>>     SELF_CHECK (arm_record.arm_regs[0] == 7);
>>>   }
>>> }
>>> +
>>> +/* Instruction reader from manually cooked instruction sequences.  */
>>> +
>>> +class test_arm_instruction_reader : public arm_instruction_reader
>>> +{
>>> +public:
>>> +  template<size_t SIZE>
>>> +  explicit test_arm_instruction_reader (const uint32_t (&insns)[SIZE])
>>> +    : m_insns (insns), m_insns_size (SIZE)
>>
>> Why the need for a template?
>> Is it just so that m_insns_size can be determined automatically?
> 
> I don't really know, I just copied the AArch64 one :).  But yeah I think
> that's it.  It could also use a gdb::array_view, that would be simpler I
> think.  I would change it for:
> 
>   /* Instruction reader from manually cooked instruction sequences.  */
> 
>   class test_arm_instruction_reader : public arm_instruction_reader
>   {
>   public:
>     explicit test_arm_instruction_reader (gdb::array_view<const uint32_t> insns)
>       : m_insns (insns)
>     {}
> 
>     uint32_t read (CORE_ADDR memaddr, enum bfd_endian byte_order) const override
>     {
>       SELF_CHECK (memaddr % 4 == 0);
>       SELF_CHECK (memaddr / 4 < m_insns.size ());
> 
>       return m_insns[memaddr / 4];
>     }
> 
>   private:
>     const gdb::array_view<const uint32_t> m_insns;
>   };
> 
> Simon
> 

I pushed the patch with the changes mentioned above.

Simon