Re: [RFA 2/2] gmp-utils: protect gdb_mpz exports against out-of-range values

[Simon Marchi <simark@simark.ca>]

On 2020-11-30 10:37 p.m., Joel Brobecker wrote:
> Hi Simon,
>
> Thanks for the review! My preliminary answers below..
>
>>> +void
>>> +gdb_mpz::safe_export (gdb::array_view<gdb_byte> buf,
>>> +		      int endian, size_t nails, bool unsigned_p) const
>>
>> Just wondering if you'll ever need to pass a non-zero value for nails.
>> If not, I think you could simplify the code by just removing it.
>
> I don't know for sure. What about a scenario where we want to export
> to a variable which is held in an area that's not a multiple number
> of bytes?
>
> The code isn't dramatically complexified because of it, so perhaps
> just keep it by default, and remove it later, if we find that
> we never used it?

Well, I find it more logical to do the other way around, add the
complexity when you need it.  Because I'm pretty sure you won't go set
an alarm clock for one year from now, to remember to come back and check
whether or not we are using that feature :).  Also, I'm usually not keen
on checking in things that aren't used, because we don't even know if
they work / do what they are intended to do.

If you know things are coming that are going to use it, I'm fine with
leaving it in, but otherwise I just don't see the point.  I'll let you
decide.

>>> +{
>>> +  gdb_assert (buf.size () > 0);
>>> +
>>> +  if (mpz_sgn (val) == 0)
>>> +    {
>>> +      /* Our value is zero, so no need to call mpz_export to do the work,
>>> +	 especially since mpz_export's documentation explicitly says
>>> +	 that the function is a noop in this case.  Just write zero to
>>> +	 BUF ourselves.  */
>>> +      memset (buf.data (), 0, buf.size ());
>>> +      return;
>>> +    }
>>> +
>>> +  /* Determine the maximum range of values that our buffer can hold,
>>> +     and verify that VAL is within that range.  */
>>> +
>>> +  gdb_mpz lo, hi;
>>> +  const size_t max_usable_bits = buf.size () * HOST_CHAR_BIT - nails;
>>> +  if (unsigned_p)
>>> +    {
>>> +      lo = 0;
>>> +
>>> +      mpz_ui_pow_ui (hi.val, 2, max_usable_bits);
>>> +      mpz_sub_ui (hi.val, hi.val, 1);
>>> +    }
>>> +  else
>>> +    {
>>> +      mpz_ui_pow_ui (lo.val, 2, max_usable_bits - 1);
>>> +      mpz_neg (lo.val, lo.val);
>>> +
>>> +      mpz_ui_pow_ui (hi.val, 2, max_usable_bits - 1);
>>> +      mpz_sub_ui (hi.val, hi.val, 1);
>>> +    }
>>> +
>>> +  if (mpz_cmp (val, lo.val) < 0 || mpz_cmp (val, hi.val) > 0)
>>> +    error (_("Cannot export value %s as %zubits %s integer"
>>> +	     " (must be between %s and %s)"),
>>
>> You might be missing a space before "bits", or maybe it's on purpose.
>
> It was on purpose, as I tend to write "32bit object" or "64bit object".
> But if there is a standard way to write these things. A quick google
> search indeed seems to indicate that there is large preference for
> having a dash (as in "32-bit computing").
>
> So, if people agree, I will change the above to add a dash.

I googled a bit and it turns out that this is a kind of compound
adjective, which requires a hyphen:

  https://www.gingersoftware.com/content/grammar-rules/adjectives/compound-adjectives/

Also, the name (bit) should be singular.  So I think we should
standardize on "%zu-bit".

>
>>
>>> +	   this->str ().c_str (),
>>> +	   max_usable_bits,
>>> +	   unsigned_p ? _("unsigned") : _("signed"),
>>> +	   lo.str ().c_str (),
>>> +	   hi.str ().c_str ());
>>> +
>>>    gdb_mpz exported_val (val);
>>>
>>> -  if (mpz_cmp_ui (val, 0) < 0)
>>> +  if (mpz_cmp_ui (exported_val.val, 0) < 0)
>>>      {
>>>        /* mpz_export does not handle signed values, so create a positive
>>>  	 value whose bit representation as an unsigned of the same length
>>> @@ -81,13 +134,27 @@ gdb_mpz::write (gdb::array_view<gdb_byte> buf, enum bfd_endian byte_order,
>>>        mpz_add (exported_val.val, exported_val.val, neg_offset.val);
>>>      }
>>>
>>> +  /* Do the export into a buffer allocated by GMP itself; that way,
>>> +     we can detect cases where BUF is not large enough to export
>>> +     our value, and thus avoid a buffer overlow.  Normally, this should
>>> +     never happen, since we verified earlier that the buffer is large
>>> +     enough to accomodate our value, but doing this allows us to be
>>> +     extra safe with the export.
>>> +
>>> +     After verification that the export behaved as expected, we will
>>> +     copy the data over to BUF.  */
>>> +
>>> +  size_t word_countp;
>>> +  gdb::unique_xmalloc_ptr<void> exported
>>
>> I'd prefer if we didn't use heap allocation unnecessarily.  If we get
>> things right, that isn't necessary.  And if we can still assert after
>> the call that we did indeed get it right, then we'll catch any case
>> where we didn't.
>
> The problem with what you suggest is that, if we didn't do things right,
> by the time we realize it, we'll have already gone through a buffer
> overflow, with the associated random and uncontrolled damage. On Ubuntu,
> we already know that the overflow causes an abort, so we wouldn't even
> get to the point where we'd double-check. For me, this risk is bad enough
> that it's well worth this (small) extra allocation.  I don't think
> this function is going to be called very frequently.

The way I see it is that if we check after the fact, it would be a
gdb_assert.  So if we get it wrong, it results in either in a crash or a
failed assertion.  Both equally result in a bug report.

But you're right, using the heap allocation makes it so we can't get it
wrong, so that's an advantage.  I just thought that we'd get it right
for good now :).

>>> @@ -258,26 +278,14 @@ template<typename T>
>>>  T
>>>  gdb_mpz::as_integer () const
>>>  {
>>> -  /* Initialize RESULT, because mpz_export only write the minimum
>>> -     number of bytes, including none if our value is zero!  */
>>> -  T result = 0;
>>> +  T result;
>>>
>>> -  gdb_mpz exported_val (val);
>>> -  if (std::is_signed<T>::value && mpz_cmp_ui (val, 0) < 0)
>>> -    {
>>> -      /* We want to use mpz_export to set the return value, but
>>> -	 this function does not handle the sign. So give exported_val
>>> -	 a value which is at the same time positive, and has the same
>>> -	 bit representation as our negative value.  */
>>> -      gdb_mpz neg_offset;
>>> +  this->safe_export (gdb::make_array_view ((gdb_byte *) &result,
>>> +					   sizeof (result)),
>>
>> I'd suggest using
>>
>>   {(gdb_byte *) &result, sizeof (result)}
>>
>> to make the array view, as suggested by Pedro earlier.
>
> I thought I had tried that and got an error, but I will try again.

I tried it before making that suggestion and it worked, so if you have
troubles let me know.

Simon