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

[Joel Brobecker <brobecker@adacore.com>]

Hi Tom,

> -fsanitize=address showed that ada-lang.c:move_bits can run off the
> end of the source buffer.  I believe this patch fixes the problem, by
> arranging not to read from the source buffer once there are sufficient
> bits in the accumulator.
> 
> gdb/ChangeLog
> 2018-10-23  Tom Tromey  <tom@tromey.com>
> 
> 	* ada-lang.c (move_bits): Don't run off the end of the source
> 	buffer.

Thanks for the patch!

This is a part of the code that always forces me to think twice
(or ten times), each time I try to touch it. I should really start
adding comments to this code that detail what we are trying to do
as we do it.

I tested your change through our testsuite on the various baremetal
targets we have, and noticed that it causes regressions on ppc and arm
targets. It's hopefully something small, but just being back from
a holiday, I'm a bit tied up at work; I'll put that issue on my TODO
list to look at further.

> ---
>  gdb/ChangeLog  |  5 +++++
>  gdb/ada-lang.c | 18 ++++++++++++------
>  2 files changed, 17 insertions(+), 6 deletions(-)
> 
> diff --git a/gdb/ada-lang.c b/gdb/ada-lang.c
> index 1462271a71..7288d65df6 100644
> --- a/gdb/ada-lang.c
> +++ b/gdb/ada-lang.c
> @@ -2682,9 +2682,12 @@ move_bits (gdb_byte *target, int targ_offset, const gdb_byte *source,
>          {
>            int unused_right;
>  
> -          accum = (accum << HOST_CHAR_BIT) + (unsigned char) *source;
> -          accum_bits += HOST_CHAR_BIT;
> -          source += 1;
> +	  if (n > accum_bits)
> +	    {
> +	      accum = (accum << HOST_CHAR_BIT) + (unsigned char) *source;
> +	      accum_bits += HOST_CHAR_BIT;
> +	      source += 1;
> +	    }
>            chunk_size = HOST_CHAR_BIT - targ_offset;
>            if (chunk_size > n)
>              chunk_size = n;
> @@ -2707,9 +2710,12 @@ move_bits (gdb_byte *target, int targ_offset, const gdb_byte *source,
>  
>        while (n > 0)
>          {
> -          accum = accum + ((unsigned char) *source << accum_bits);
> -          accum_bits += HOST_CHAR_BIT;
> -          source += 1;
> +	  if (n > accum_bits)
> +	    {
> +	      accum = accum + ((unsigned char) *source << accum_bits);
> +	      accum_bits += HOST_CHAR_BIT;
> +	      source += 1;
> +	    }
>            chunk_size = HOST_CHAR_BIT - targ_offset;
>            if (chunk_size > n)
>              chunk_size = n;
> -- 
> 2.17.1

-- 
Joel