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

[Simon Marchi <simark@simark.ca>]

On 2020-11-29 10:45 a.m., Joel Brobecker wrote:
> The gdb_mpz class currently provides a couple of methods which
> essentially export an mpz_t value into either a buffer, or an integral
> type. The export is based on using the mpz_export function which
> we discovered can be a bit treacherous if used without caution.
>
> In particular, the initial motivation for this patch was to catch
> situations where the mpz_t value was so large that it would not fit
> in the destination area. mpz_export does not know the size of
> the buffer, and therefore can happily write past the end of our buffer.
>
> While designing a solution to the above problem, I also discovered
> that we also needed to be careful when exporting signed numbers.
> In particular, numbers which are larger than the maximum value
> for a given signed type size, but no so large as to fit in the
> *unsigned* version with the same size, would end up being exported
> incorrectly. This is related to the fact that mpz_export ignores
> the sign of the value being exportd, and assumes an unsigned export.
> Thus, for such large values, the appears as if mpz_export is able
> to fit our value into our buffer, but in fact, it does not.
>
> Also, I noticed that gdb_mpz::write wasn't taking its unsigned_p
> parameter, which was a hole.
>
> For all these reasons, a new low-level private method called
> "safe_export" has been added to class gdb_mpz, whose goal is
> to perform all necessary checks and manipulations for a safe
> and correct export. As a bonus, this method allows us to factorize
> the handling of negative value exports.
>
> The gdb_mpz::as_integer and gdb_mpz::write methods are then simplified
> to take advantage of this new safe_export method.
>
> gdb/ChangeLog:
>
>         * gmp-utils.h (gdb_mpz::safe_export): New private method.
>         (gdb_mpz::as_integer): Reimplement using gdb_mpz::safe_export.
>         * gmp-utils.c (gdb_mpz::write): Rewrite using gdb_mpz::safe_export.
>         (gdb_mpz::safe_export): New method.
>         * unittests/gmp-utils-selftests .c (gdb_mpz_as_integer):
>         Update function description.
>         (check_as_integer_raises_out_of_range_error): New function.
>         (gdb_mpz_as_integer_out_of_range): New function.
>         (_initialize_gmp_utils_selftests): Register
>         gdb_mpz_as_integer_out_of_range as a selftest.
> ---
>  gdb/gmp-utils.c                     | 79 ++++++++++++++++++++++++++++++++++---
>  gdb/gmp-utils.h                     | 44 ++++++++++++---------
>  gdb/unittests/gmp-utils-selftests.c | 71 +++++++++++++++++++++++++++++++--
>  3 files changed, 167 insertions(+), 27 deletions(-)
>
> diff --git a/gdb/gmp-utils.c b/gdb/gmp-utils.c
> index e3a3333..f94bdc5 100644
> --- a/gdb/gmp-utils.c
> +++ b/gdb/gmp-utils.c
> @@ -68,9 +68,62 @@ void
>  gdb_mpz::write (gdb::array_view<gdb_byte> buf, enum bfd_endian byte_order,
>  		bool unsigned_p) const
>  {
> +  this->safe_export
> +    (buf, byte_order == BFD_ENDIAN_BIG ? 1 : -1 /* endian */, 0 /* nails */,
> +     unsigned_p);
> +}
> +
> +/* See gmp-utils.h.  */
> +
> +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.

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

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

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

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

Simon