Re: pushed: Add support for DWARF-based fixed point types

[Joel Brobecker <brobecker@adacore.com>]

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

> We pass mpz_export a buffer of 8 bytes (statically allocated in
> write_fp_test), but GMP decides it needs to write 16 bytes, hence the
> overflow.
> 
> I tried to read the GMP doc, but I am familiar with its concepts, so I
> don't really understand if we are using the API correctly or not.

I found the source of the problem, which was in a way subtle-enough
that you really have to pay attention to these details (which,
luckily, are handled automatically thanks to our minor C++-ification
of GMP in gmp-utils), and yet so obvious once you find it.
Attached is the patch that I will push later today (need to run RSN,
and don't want to make a mistake because I'm rushing).

I think this error might be highlighting a weakness, though. I need
to investigate more, but I'm thinking it might be wise to add some
checks during export that the buffer size is large enough to fit
the value. In other words, I'm thinking of having our own
safe_mpz_export which double-checks the size of the buffer according
to the formula given by the documentation, and raises an error if
too small.

The fact that GMP happily goes beyond the end of the buffer is
a bit unexpected, still. Maybe something to report to the GMP team.

Later!

-- 
Joel

[-- Attachment #2: 0001-Fix-stack-smashing-error-during-gdb_mpq_write_fixed_.patch --]
[-- Type: text/x-diff, Size: 2944 bytes --]

From 1cb4f9e076dbd87808783e6f4f54b4b22c45d0e5 Mon Sep 17 00:00:00 2001
From: Joel Brobecker <brobecker@adacore.com>
Date: Mon, 23 Nov 2020 08:02:10 +0400
Subject: [PATCH] Fix stack smashing error during gdb_mpq_write_fixed_point
 selftest

When building GDB using Ubuntu 20.04's system libgmp and compiler,
running the "maintenance selftest" command triggers the following error:

    | Running selftest gdb_mpq_write_fixed_point.
    | *** stack smashing detected ***: terminated
    | [1]    1092790 abort (core dumped)  ./gdb gdb

This happens while trying to construct an mpq_t object (a rational)
from two integers representing the numerator and denominator.
In our test, the numerator is -8, and the denominator is 1.
The problem was that the rational was constructed using the wrong
function. This is what we were doing prior to this patch:

    mpq_set_ui (v.val, numerator, denominator);

The 'u' in "ui" stands for *unsigned*, which is wrong because
numerator and denominator's type is "int".

As a result of the above, instead of getting a rational value of -8,
we get a rational with a very large positive value (gmp_printf
says "18446744073709551608").

From there, the test performs an operation which is expected to
write this value into a buffer which was not dimensioned to fit
such a number, thus leading GMP into a buffer overflow.
This was verified by applying the formula that GMP's documentation
gives for the required memory buffer size needed during export:

    | When an application is allocating space itself the required size can
    | be determined with a calculation like the following. Since
    | mpz_sizeinbase always returns at least 1, count here will be at
    | least one, which avoids any portability problems with malloc(0),
    | though if z is zero no space at all is actually needed (or written).
    |
    |     numb = 8*size - nail;
    |     count = (mpz_sizeinbase (z, 2) + numb-1) / numb;
    |     p = malloc (count * size);

With the very large number, mpz_sizeinbase returns 66 and thus
the malloc size becomes 16 bytes instead of the 8 we allocated.

This patch fixes the issue by using the correct "set" function.

gdb/ChangeLog:

        * unittests/gmp-utils-selftests.c (write_fp_test): Use mpq_set_si
        instead of mpq_set_ui to initialize our GMP rational.
---
 gdb/unittests/gmp-utils-selftests.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gdb/unittests/gmp-utils-selftests.c b/gdb/unittests/gmp-utils-selftests.c
index af5bc65d2f9..175ab3f9827 100644
--- a/gdb/unittests/gmp-utils-selftests.c
+++ b/gdb/unittests/gmp-utils-selftests.c
@@ -400,7 +400,7 @@ write_fp_test (int numerator, unsigned int denominator,
   memset (buf, 0, len);
 
   gdb_mpq v;
-  mpq_set_ui (v.val, numerator, denominator);
+  mpq_set_si (v.val, numerator, denominator);
   mpq_canonicalize (v.val);
   v.write_fixed_point (buf, len, byte_order, 0, scaling_factor);
 
-- 
2.25.1