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

[Joel Brobecker <brobecker@adacore.com>]

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?

> > +{
> > +  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.

> 
> > +	   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.

> > +    (mpz_export (NULL, &word_countp, -1 /* order */, buf.size () /* size */,
> > +		 endian, nails, exported_val.val));
> > +
> > +  gdb_assert (word_countp == 1);
> > +
> >    /* Start by clearing the buffer, as mpz_export only writes as many
> > -     bytes as it needs (including none, if the value to export is zero.  */
> > +     bytes as it needs.  */
> >    memset (buf.data (), 0, buf.size ());
> > -  mpz_export (buf.data (), NULL /* count */, -1 /* order */,
> > -	      buf.size () /* size */,
> > -	      byte_order == BFD_ENDIAN_BIG ? 1 : -1 /* endian */,
> > -	      0 /* nails */, exported_val.val);
> > +  memcpy (buf.data (), exported.get (), buf.size ());
> 
> The memset just before the memcpy of the same size is useless, as all
> the bytes will get overwritten by the memcpy.

Indeed. The above comes from my belief that mpz_export only writes
the necessary number of *bytes*, but re-reading the documentation,
I belive it's the necessary number of *words*. I don't know how things
could possibly be working in the case where mpz_export does the memory
allocation otherwise.

Thanks for catching that. I'll remove the extra memset.

> > @@ -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.

-- 
Joel