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

[Joel Brobecker <brobecker@adacore.com>]

[-- Attachment #1: Type: text/plain, Size: 1396 bytes --]

> On 2020-12-05 3:10 a.m., Joel Brobecker wrote:
> > @@ -81,13 +133,26 @@ 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
> > +    (mpz_export (NULL, &word_countp, -1 /* order */, buf.size () /* size */,
> > +		 endian, 0 /* 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.  */
> 
> That comment is now stale.

Indeed. Thanks for catching that.

> Otherwise, that LGTM.

Thanks Simon. The patch is now in, with the correction above (a copy
of the patch is attached, for the record).

-- 
Joel

[-- Attachment #2: 0001-gmp-utils-protect-gdb_mpz-exports-against-out-of-ran.patch --]
[-- Type: text/x-diff, Size: 12521 bytes --]

From 63c457b911043aa6ebf3558e2d2050ee53d28a8e Mon Sep 17 00:00:00 2001
From: Joel Brobecker <brobecker@adacore.com>
Date: Sat, 5 Dec 2020 23:56:59 -0500
Subject: [PATCH] gmp-utils: protect gdb_mpz exports against out-of-range
 values

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/ChangeLog                       | 13 ++++++
 gdb/gmp-utils.c                     | 79 +++++++++++++++++++++++++++++++++----
 gdb/gmp-utils.h                     | 40 ++++++++++---------
 gdb/unittests/gmp-utils-selftests.c | 71 +++++++++++++++++++++++++++++++--
 4 files changed, 174 insertions(+), 29 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index fd95992..d8b67c6 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,16 @@
+2020-12-06  Joel Brobecker  <brobecker@adacore.com>
+
+	* 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.
+
 2020-12-05  Joel Brobecker  <brobecker@adacore.com>
 
 	* gmp-utils.c (gdb_mpz::read): Use HOST_CHAR_BIT instead of
diff --git a/gdb/gmp-utils.c b/gdb/gmp-utils.c
index e3a3333..60c915c 100644
--- a/gdb/gmp-utils.c
+++ b/gdb/gmp-utils.c
@@ -68,9 +68,61 @@ 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 */, unsigned_p);
+}
+
+/* See gmp-utils.h.  */
+
+void
+gdb_mpz::safe_export (gdb::array_view<gdb_byte> buf,
+		      int endian, bool unsigned_p) const
+{
+  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;
+  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 %zu-bits %s integer"
+	     " (must be between %s and %s)"),
+	   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 +133,24 @@ 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);
     }
 
-  /* 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.  */
-  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);
+  /* 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
+    (mpz_export (NULL, &word_countp, -1 /* order */, buf.size () /* size */,
+		 endian, 0 /* nails */, exported_val.val));
+
+  gdb_assert (word_countp == 1);
+
+  memcpy (buf.data (), exported.get (), buf.size ());
 }
 
 /* See gmp-utils.h.  */
diff --git a/gdb/gmp-utils.h b/gdb/gmp-utils.h
index 12e4f8e..dae62cf 100644
--- a/gdb/gmp-utils.h
+++ b/gdb/gmp-utils.h
@@ -121,6 +121,24 @@ struct gdb_mpz
 
   /* Helper template for constructor and operator=.  */
   template<typename T> void set (T src);
+
+  /* Low-level function to export VAL into BUF as a number whose byte size
+     is the size of BUF.
+
+     If UNSIGNED_P is true, then export VAL into BUF as an unsigned value.
+     Otherwise, export it as a signed value.
+
+     The API is inspired from GMP's mpz_export, hence the naming and types
+     of the following parameter:
+       - ENDIAN should be:
+           . 1 for most significant byte first; or
+	   . -1 for least significant byte first; or
+	   . 0 for native endianness.
+
+    An error is raised if BUF is not large enough to contain the value
+    being exported.  */
+  void safe_export (gdb::array_view<gdb_byte> buf,
+		    int endian, bool unsigned_p) const;
 };
 
 /* A class to make it easier to use GMP's mpq_t values within GDB.  */
@@ -258,26 +276,12 @@ 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;
-
-  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;
+  T result;
 
-      mpz_ui_pow_ui (neg_offset.val, 2, sizeof (T) * HOST_CHAR_BIT);
-      mpz_add (exported_val.val, exported_val.val, neg_offset.val);
-    }
+  this->safe_export ({(gdb_byte *) &result, sizeof (result)},
+		     0 /* endian (0 = native) */,
+		     !std::is_signed<T>::value /* unsigned_p */);
 
-  mpz_export (&result, NULL /* count */, -1 /* order */,
-	      sizeof (T) /* size */, 0 /* endian (0 = native) */,
-	      0 /* nails */, exported_val.val);
   return result;
 }
 
diff --git a/gdb/unittests/gmp-utils-selftests.c b/gdb/unittests/gmp-utils-selftests.c
index 1365905..30c1902 100644
--- a/gdb/unittests/gmp-utils-selftests.c
+++ b/gdb/unittests/gmp-utils-selftests.c
@@ -26,9 +26,10 @@ namespace selftests {
 
 /* Perform a series of general tests of gdb_mpz's as_integer method.
 
-   This function tries to be reasonably exhaustive, by testing the edges,
-   as well as a resonable set of values including negative ones, zero,
-   and positive values.  */
+   This function limits itself to values which are in range (out-of-range
+   values will be tested separately).  In doing so, it tries to be reasonably
+   exhaustive, by testing the edges, as well as a resonable set of values
+   including negative ones, zero, and positive values.  */
 
 static void
 gdb_mpz_as_integer ()
@@ -80,6 +81,68 @@ gdb_mpz_as_integer ()
   SELF_CHECK (v.as_integer<ULONGEST> () == ul_expected);
 }
 
+/* A helper function which calls the given gdb_mpz object's as_integer
+   method with the given type T, and verifies that this triggers
+   an error due to VAL's value being out of range for type T.  */
+
+template<typename T, typename = gdb::Requires<std::is_integral<T>>>
+static void
+check_as_integer_raises_out_of_range_error (const gdb_mpz &val)
+{
+  try
+    {
+      val.as_integer<T> ();
+    }
+  catch (const gdb_exception_error &ex)
+    {
+      SELF_CHECK (ex.reason == RETURN_ERROR);
+      SELF_CHECK (ex.error == GENERIC_ERROR);
+      SELF_CHECK (strstr (ex.what (), "Cannot export value") != nullptr);
+      return;
+    }
+  /* The expected exception did not get raised.  */
+  SELF_CHECK (false);
+}
+
+/* Perform out-of-range tests of gdb_mpz's as_integer method.
+
+   The goal of this function is to verify that gdb_mpz::as_integer
+   handles out-of-range values correctly.  */
+
+static void
+gdb_mpz_as_integer_out_of_range ()
+{
+  gdb_mpz v;
+
+  /* Try LONGEST_MIN minus 1.  */
+  mpz_ui_pow_ui (v.val, 2, sizeof (LONGEST) * 8 - 1);
+  mpz_neg (v.val, v.val);
+  mpz_sub_ui (v.val, v.val, 1);
+
+  check_as_integer_raises_out_of_range_error<ULONGEST> (v);
+  check_as_integer_raises_out_of_range_error<LONGEST> (v);
+
+  /* Try negative one (-1). */
+  v = -1;
+
+  check_as_integer_raises_out_of_range_error<ULONGEST> (v);
+  SELF_CHECK (v.as_integer<LONGEST> () == (LONGEST) -1);
+
+  /* Try LONGEST_MAX plus 1.  */
+  v = LONGEST_MAX;
+  mpz_add_ui (v.val, v.val, 1);
+
+  SELF_CHECK (v.as_integer<ULONGEST> () == (ULONGEST) LONGEST_MAX + 1);
+  check_as_integer_raises_out_of_range_error<LONGEST> (v);
+
+  /* Try ULONGEST_MAX plus 1.  */
+  v = ULONGEST_MAX;
+  mpz_add_ui (v.val, v.val, 1);
+
+  check_as_integer_raises_out_of_range_error<ULONGEST> (v);
+  check_as_integer_raises_out_of_range_error<LONGEST> (v);
+}
+
 /* A helper function to store the given integer value into a buffer,
    before reading it back into a gdb_mpz.  Sets ACTUAL to the value
    read back, while at the same time setting EXPECTED as the value
@@ -445,6 +508,8 @@ _initialize_gmp_utils_selftests ()
 {
   selftests::register_test ("gdb_mpz_as_integer",
 			    selftests::gdb_mpz_as_integer);
+  selftests::register_test ("gdb_mpz_as_integer_out_of_range",
+			    selftests::gdb_mpz_as_integer_out_of_range);
   selftests::register_test ("gdb_mpz_read_all_from_small",
 			    selftests::gdb_mpz_read_all_from_small);
   selftests::register_test ("gdb_mpz_read_min_max",
-- 
2.1.4