Re: [PATCH] Fix buffer overflow in ada-lang.c:move_bits

[Pedro Alves <palves@redhat.com>]

On 11/14/2018 05:11 PM, Joel Brobecker wrote:
> Hello,
> 
>>>> I was going to suggest that this would benefit from unit tests in
>>>> the style of dwarf2read.c:copy_bitwise's, but, actually, isn't this
>>>> exactly the same as copy_bitwise?  Can we get rid of ada-lang.c:move_bits?
>>>> (And maybe move copy_bitwise elsewhere?)
>>> I meant to say dwarf2loc.c instead of dwarf2read.c.
>> It does look exactly the same, doesn't it? I'll see if we can just
>> re-use dwarf2loc's copy_bitwise. Thanks for the suggestion!
> How about the attached? I ran it through AdaCore's testsuite on
> all the platforms we support as well as the official testsuite on
> x86_64-linux. No regression.
> 
> gdb/ChangeLog:
> 
>         * ada-lang.c (move_bits): Delete. Update all callers to use
>         copy_bitwise instead.
>         * dwarf2loc.c (copy_bitwise, bits_to_str::bits_to_str)
>         (selftests::check_copy_bitwise, selftests::copy_bitwise_tests):
>         Move from here to utils.c.
>         (_initialize_dwarf2loc): Remove call to register copy_bitwise
>         selftests.
>         * utils.h (copy_bitwise): Add declaration.
>         * utils.c (copy_bitwise, bits_to_str::bits_to_str)
>         (selftests::check_copy_bitwise, selftests::copy_bitwise_tests):
>         Moved here from dwarf2loc.c.
>         (_initialize_utils): Register copy_bitwise selftests.
> 
> Thank you!
> -- Joel
> 
> 

Great, thanks!

Nit, since the function is now public, I'd consider moving the unit
tests to under gdb/unittests/ instead, like, to a new
copy_bitwise-selftests.c file.  (I'm mildly thinking that'd be a better
filename than utils-selftest.c because the function may well
move again in the future.  Notice how gdb_realpath's unit tests
were left behind in gdb/utils.c even though gdb_realpath moved to 
common/pathstuff.c.)

If you do that, you can drop the
'#if GDB_SELF_TEST' around the tests, since files in that
directory are not compiled if unit tests are disabled.

Regardless, LGTM.

Thanks,
Pedro Alves