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

Doug Evans
2013-12-03T23:01:36Z

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.