Re: extract_unsigned_integer API (Re: [PATCH] Remove MAX_REGISTER_SIZE from frame.c)

[Pedro Alves <palves@redhat.com>]

On 03/28/2017 05:13 PM, Yao Qi wrote:
> Pedro Alves <palves@redhat.com> writes:
> 
>> class extractor
>> {
>> public:
>>    extractor () = default;
>>
>>    // Get buffer.  Could take a "size" parameter too,
>>    // for pre-validation instead of passing "size" to "extract".
>>    // Or make that a separate size() method.   Or add a "size" parameter
>>    // to the ctor and validate there.  Whatever.  The lambda-based
>>    // solution isn't validating upfront either.
> 
> My lambda-based solution does validate the boundary before reading
> contents to buffer,
> 
> +ULONGEST
> +extract_unsigned_integer (gdb::function_view<void (gdb_byte *, size_t size)> content_provider,
> +                         int len, enum bfd_endian byte_order)
> +{
> +  if (len > (int) sizeof (ULONGEST))
> +    error (_("\
> +That operation is not available on integers of more than %d bytes."),
> +          (int) sizeof (ULONGEST));
> +
> +  gdb_byte buf[sizeof (ULONGEST)];
> +
> +  content_provider (buf, len);
> +  return extract_unsigned_integer_1 (buf, len, byte_order);
> +}
> 

The upfront check doesn't protect entirely.  It still up to the lambda,
to ultimately check for overflow.  Here:

  return extract_unsigned_integer ([&] (gdb_byte *buf, size_t size)
				   {
				     frame_unwind_register (frame, regnum, buf);
				   }, size, byte_order);

... frame_unwind_register can overflow the buffer, since it ignores "size".
And if we require adding some check inside the lambda, then we've really
not gained that much by doing the upfront check.

>>
>>  extractor extr;
>>  frame_unwind_register (frame, regnum, ext.buffer ());
> 
> We may overflow ext.buffer (), because the boundary checking is done in
> .extract below,
> 
>>  return extr.extract (size, byte_order);

Yes, and that can sorted by e.g., passing the size to the buffer()
method, as I mentioned in the comment.  Like:

  extractor extr;
  frame_unwind_register (frame, regnum, ext.buffer (size));
  return extr.extract (size, byte_order);

extractor::buffer(size_t) would throw error on overflow.

Or pass it to the ctor (which would likewise throw error on overflow):

  extractor extr (size);
  frame_unwind_register (frame, regnum, ext.buffer ());
  return extr.extract (size, byte_order);

Could even store the size and byte order inside the extractor
object, and avoid writing the size more than once:

  extractor extr (size, byte_order);
  frame_unwind_register (frame, regnum, ext.buffer ());
  return extr.extract ();

Or make "extrator::buffer" remember the last size, so extractors
can be reused.  Or even support both, ctor with and without size,
buffer() with and without size.  extractror::extract would always
used the last remembered size.

So I still don't see any advantage in a callback-based interface.

Thanks,
Pedro Alves