FreeChainXenon/binutils-gdb
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

gmp-utils: protect gdb_mpz exports against out-of-range values

Browse source 
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.
This commit is contained in:
Joel Brobecker 2020-12-05 23:56:59 -05:00
parent 6b1dce3a3d
commit 63c457b911
4 changed files with 174 additions and 29 deletions
Download patch file Download diff file Expand all files Collapse all files

13
gdb/ChangeLog
View file

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

79
gdb/gmp-utils.c
View file

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

40
gdb/gmp-utils.h
View file

@ -121,6 +121,24 @@ private:
/* 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;
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_byte *) &result, sizeof (result)},
0 /* endian (0 = native) */,
!std::is_signed<T>::value /* unsigned_p */);
mpz_ui_pow_ui (neg_offset.val, 2, sizeof (T) * HOST_CHAR_BIT);
mpz_add (exported_val.val, exported_val.val, neg_offset.val);
}
mpz_export (&result, NULL /* count */, -1 /* order */,
sizeof (T) /* size */, 0 /* endian (0 = native) */,
0 /* nails */, exported_val.val);
return result;
}

71
gdb/unittests/gmp-utils-selftests.c
View file

@ -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",