Re: [PATCH] PR 16286: Reading python value as string beyond declared size

[Doug Evans <dje@google.com>]

On Tue, Dec 3, 2013 at 12:29 PM, Pedro Alves <palves@redhat.com> wrote:
> On 12/02/2013 11:14 PM, Doug Evans wrote:
>> +      if (*length > 0)
>> +     fetchlimit = UINT_MAX;
>
> Shouldn't this be:
>
>       if (*length > 0)
>         fetchlimit = *length;
>
> ?  That is, if the caller specified a limit, why do we do over it?

read_string will take min (len, fetchlimit), and I saw no value in
passing fetchlimit = *length.

> Couldn't this new check be merge above where we compute
> fetchlimit to begin with?  With the comment there adjusted to
> something like:
>
> +      /* If have an explicit requested length, use that as fetchlimit.
> +        Otherwise, if we know the size of the array, we can use it as
> +        a limit on the number of characters to be fetched.  */

I want to keep fetchlimit and *length separate at this point.
They have different meanings/purposes.

> BTW, it looks like the not_lval/lval_internalvar path can
> blindly read beyond the value's contents buffer, if *length
> is bigger than the value's contents buffer size:
>
>   /* If the string lives in GDB's memory instead of the inferior's,
>      then we just need to copy it to BUFFER.  Also, since such strings
>      are arrays with known size, FETCHLIMIT will hold the size of the
>      array.  */
>   if ((VALUE_LVAL (value) == not_lval
>        || VALUE_LVAL (value) == lval_internalvar)
>       && fetchlimit != UINT_MAX)
>     {
>       int i;
>       const gdb_byte *contents = value_contents (value);
>
>       /* If a length is specified, use that.  */
>       if (*length >= 0)
>         i  = *length;
>         ^^^^^^^^^^^^^
>       else
>         /* Otherwise, look for a null character.  */
>         for (i = 0; i < fetchlimit; i++)
>           if (extract_unsigned_integer (contents + i * width,
>                                         width, byte_order) == 0)
>             break;
>
>       /* I is now either a user-defined length, the number of non-null
>          characters, or FETCHLIMIT.  */
>       *length = i * width;
>       *buffer = xmalloc (*length);
>       memcpy (*buffer, contents, *length);
>       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

It didn't look right to me either, but I was leaving digging deeper
for another pass.