GNU bug report logs - #41850
[PATCH] maint: Avoid signed integer overflows

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 41850 in the body.
You can then email your comments to 41850 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Tobias Stoeckmann <tobias <at> stoeckmann.org>
To: bug-coreutils <at> gnu.org
Subject: [PATCH] maint: Avoid signed integer overflows
Date: Sun, 14 Jun 2020 14:47:11 +0200

Since -LONG_MIN results in LONG_MIN again, the operation itself is
a signed integer overflow.

This can be observed with the following calls (best if compiled
with -ftrapv or -fsanitize=undefined):

$ numfmt --padding=-9223372036854775808
$ seq 1e-9223372036854775808

Technically, the change in seq "reduces" the precision, but a double
or long double that small would be represented as 0 anyway.
---
 src/numfmt.c | 2 +-
 src/seq.c    | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/numfmt.c b/src/numfmt.c
index 8869791b0..8871a8c01 100644
--- a/src/numfmt.c
+++ b/src/numfmt.c
@@ -1496,7 +1496,7 @@ main (int argc, char **argv)
 
         case PADDING_OPTION:
           if (xstrtol (optarg, NULL, 10, &padding_width, "") != LONGINT_OK
-              || padding_width == 0)
+              || padding_width == 0 || padding_width < -LONG_MAX)
             die (EXIT_FAILURE, 0, _("invalid padding value %s"),
                  quote (optarg));
           if (padding_width < 0)
diff --git a/src/seq.c b/src/seq.c
index ddb63b642..2ca1e4f7b 100644
--- a/src/seq.c
+++ b/src/seq.c
@@ -197,7 +197,7 @@ scan_arg (const char *arg)
         e = strchr (arg, 'E');
       if (e)
         {
-          long exponent = strtol (e + 1, NULL, 10);
+          long exponent = MAX (strtol (e + 1, NULL, 10), -LONG_MAX);
           ret.precision += exponent < 0 ? -exponent
                                         : - MIN (ret.precision, exponent);
           /* Don't account for e.... in the width since this is not output.  */
-- 
2.27.0

Message #8 received at 41850 <at> debbugs.gnu.org (full text, mbox):

From: Pádraig Brady <P <at> draigBrady.com>
To: Tobias Stoeckmann <tobias <at> stoeckmann.org>, 41850 <at> debbugs.gnu.org
Subject: Re: bug#41850: [PATCH] maint: Avoid signed integer overflows
Date: Mon, 15 Jun 2020 23:11:57 +0100

[Message part 1 (text/plain, inline)]

On 14/06/2020 13:47, Tobias Stoeckmann wrote:
> Since -LONG_MIN results in LONG_MIN again, the operation itself is
> a signed integer overflow.
> 
> This can be observed with the following calls (best if compiled
> with -ftrapv or -fsanitize=undefined):
> 
> $ numfmt --padding=-9223372036854775808
> $ seq 1e-9223372036854775808
> 
> Technically, the change in seq "reduces" the precision, but a double
> or long double that small would be represented as 0 anyway.

Thanks for fixing those -fsanitize=undefined issues.

I can confirm with GCC 10 -fsanitize=undefined was giving:

  src/numfmt.c:1505:31: runtime error:
  negation of -9223372036854775808 cannot be represented in type 'long int'

How did you notice BTW. This wasn't exposed in existing tests.
I've updated your patch (attached) to add tests for this.

cheers,
Pádraig

[numfmt-seq-ubsan.patch (text/x-patch, attachment)]

Message #11 received at 41850 <at> debbugs.gnu.org (full text, mbox):

From: Bernhard Voelker <mail <at> bernhard-voelker.de>
To: Pádraig Brady <P <at> draigBrady.com>,
 Tobias Stoeckmann <tobias <at> stoeckmann.org>, 41850 <at> debbugs.gnu.org
Subject: Re: bug#41850: [PATCH] maint: Avoid signed integer overflows
Date: Tue, 16 Jun 2020 08:32:02 +0200

On 2020-06-16 00:11, Pádraig Brady wrote:
> I've updated your patch (attached) to add tests for this.

nice, thanks.

Have a nice day,
Berny

Message #14 received at 41850 <at> debbugs.gnu.org (full text, mbox):

From: Tobias Stoeckmann <tobias <at> stoeckmann.org>
To: Pádraig Brady <P <at> draigBrady.com>
Cc: 41850 <at> debbugs.gnu.org
Subject: Re: bug#41850: [PATCH] maint: Avoid signed integer overflows
Date: Wed, 17 Jun 2020 20:15:16 +0200

Hi,

On Mon, Jun 15, 2020 at 11:11:57PM +0100, Pádraig Brady wrote:
> How did you notice BTW. This wasn't exposed in existing tests.

it was a manual code review.

I like coreutils for having such a high quality and its goal of being as
portable as possible. The former means that it's challenging and
educational to find bugs or standard violations and the latter that
fixes have a high change of being accepted. Last but not least, small
programs make it easy to trigger issues with a simple call for easy
reproducability.

> I've updated your patch (attached) to add tests for this.

Thanks, the test suite looks nice.


Tobias

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.