From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from rock.gnat.com (rock.gnat.com [IPv6:2620:20:4000:0:a9e:1ff:fe9b:1d1]) by sourceware.org (Postfix) with ESMTP id BD5D23858024 for ; Mon, 23 Nov 2020 04:27:17 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org BD5D23858024 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=adacore.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=brobecker@adacore.com Received: from localhost (localhost.localdomain [127.0.0.1]) by filtered-rock.gnat.com (Postfix) with ESMTP id 87B7C116829; Sun, 22 Nov 2020 23:27:17 -0500 (EST) X-Virus-Scanned: Debian amavisd-new at gnat.com Received: from rock.gnat.com ([127.0.0.1]) by localhost (rock.gnat.com [127.0.0.1]) (amavisd-new, port 10024) with LMTP id nKco05dPa8m5; Sun, 22 Nov 2020 23:27:17 -0500 (EST) Received: from float.home (localhost.localdomain [127.0.0.1]) (using TLSv1.2 with cipher ADH-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by rock.gnat.com (Postfix) with ESMTPS id 28E23116828; Sun, 22 Nov 2020 23:27:17 -0500 (EST) Received: by float.home (Postfix, from userid 1000) id BB987A6B97; Mon, 23 Nov 2020 08:27:11 +0400 (+04) Date: Mon, 23 Nov 2020 08:27:11 +0400 From: Joel Brobecker To: Simon Marchi Cc: gdb-patches@sourceware.org Subject: Re: pushed: Add support for DWARF-based fixed point types Message-ID: <20201123042711.GA967337@adacore.com> References: <1604817017-25807-1-git-send-email-brobecker@adacore.com> <1605429345-78384-1-git-send-email-brobecker@adacore.com> <20201118034727.GF617116@adacore.com> <20201122140036.GA604842@adacore.com> <5e62ef60-93d7-ad52-4f9d-b23266ae4fc8@simark.ca> MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="BOKacYhQ+x31HxR3" Content-Disposition: inline In-Reply-To: <5e62ef60-93d7-ad52-4f9d-b23266ae4fc8@simark.ca> X-Spam-Status: No, score=-9.4 required=5.0 tests=BAYES_00, GIT_PATCH_0, JMQ_SPF_NEUTRAL, KAM_DMARC_STATUS, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 23 Nov 2020 04:27:19 -0000 --BOKacYhQ+x31HxR3 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline > 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 --BOKacYhQ+x31HxR3 Content-Type: text/x-diff; charset=us-ascii Content-Disposition: attachment; filename="0001-Fix-stack-smashing-error-during-gdb_mpq_write_fixed_.patch" >From 1cb4f9e076dbd87808783e6f4f54b4b22c45d0e5 Mon Sep 17 00:00:00 2001 From: Joel Brobecker 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 --BOKacYhQ+x31HxR3--