GNU bug report logs - #7213
[PATCH] sort: fix buffer overrun on 32-bit hosts when warning re obsolete keys

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 7213 in the body.
You can then email your comments to 7213 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: Paul Eggert <eggert <at> cs.ucla.edu>
To: bug-coreutils <at> gnu.org
Subject: [PATCH] sort: fix buffer overrun on 32-bit hosts when warning re
	obsolete keys
Date: Thu, 14 Oct 2010 00:12:23 -0700

* src/sort.c (key_warnings): Local buffer should be of size
INT_BUFSIZE_BOUND (uintmax_t), not INT_BUFSIZE_BOUND (sword).
This bug was discovered by running 'make check' on a 32-bit
Solaris 8 sparc host, using Sun cc.  I saw several other instances
of invoking umaxtostr on a buffer declared to be of size
INT_BUFSIZE_BOUND (VAR), and these instances should at some point
be replaced by INT_BUFSIZE_BOUND (uintmax_t) too, as that's a
less error-prone style.
---
 src/sort.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/src/sort.c b/src/sort.c
index c155eda..7e25f6a 100644
--- a/src/sort.c
+++ b/src/sort.c
@@ -2320,7 +2320,7 @@ key_warnings (struct keyfield const *gkey, bool gkey_only)
         {
           size_t sword = key->sword;
           size_t eword = key->eword;
-          char tmp[INT_BUFSIZE_BOUND (sword)];
+          char tmp[INT_BUFSIZE_BOUND (uintmax_t)];
           /* obsolescent syntax +A.x -B.y is equivalent to:
                -k A+1.x+1,B.y   (when y = 0)
                -k A+1.x+1,B+1.y (when y > 0)  */
-- 
1.7.2

Message #10 received at 7213-done <at> debbugs.gnu.org (full text, mbox):

From: Jim Meyering <jim <at> meyering.net>
To: Paul Eggert <eggert <at> cs.ucla.edu>
Cc: 7213-done <at> debbugs.gnu.org
Subject: Re: bug#7213: [PATCH] sort: fix buffer overrun on 32-bit hosts when
	warning re obsolete keys
Date: Thu, 14 Oct 2010 11:37:17 +0200

Paul Eggert wrote:
> * src/sort.c (key_warnings): Local buffer should be of size
> INT_BUFSIZE_BOUND (uintmax_t), not INT_BUFSIZE_BOUND (sword).
> This bug was discovered by running 'make check' on a 32-bit
> Solaris 8 sparc host, using Sun cc.  I saw several other instances
> of invoking umaxtostr on a buffer declared to be of size
> INT_BUFSIZE_BOUND (VAR), and these instances should at some point
> be replaced by INT_BUFSIZE_BOUND (uintmax_t) too, as that's a
> less error-prone style.
> ---
>  src/sort.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/src/sort.c b/src/sort.c
> index c155eda..7e25f6a 100644
> --- a/src/sort.c
> +++ b/src/sort.c
> @@ -2320,7 +2320,7 @@ key_warnings (struct keyfield const *gkey, bool gkey_only)
>          {
>            size_t sword = key->sword;
>            size_t eword = key->eword;
> -          char tmp[INT_BUFSIZE_BOUND (sword)];
> +          char tmp[INT_BUFSIZE_BOUND (uintmax_t)];
>            /* obsolescent syntax +A.x -B.y is equivalent to:
>                 -k A+1.x+1,B.y   (when y = 0)
>                 -k A+1.x+1,B+1.y (when y > 0)  */

Thanks!  Pushed.
BTW, for fyi-style patches like this,
please use coreutils <at> gnu.org rather than bug-...
Using the latter creates a new bug report in the database
and typically requires manual closing, even if only by
my appending "-done" to the number part of the Cc'd address above.

While I do see how hard-coding the maximum of uintmax_t would make
maintenance a little less error-prone, I prefer to use the variable name
when possible, since that makes the bound tight, and it is guaranteed
to grow if ever the variable's type size increases.  However, inttostr.h
does not provide a sizetostr (or size_?ttostr) function, so currently
we cannot make the bound tight for a variable of type size_t.

To that end, I realized that we can automatically detect some classes
of abuse like what you fixed above, perhaps even "most" of it, since
most users of inttostr.h-declared functions also use a 2nd argument
that is declared on the stack, as above.  See below:

PS. for fyi-style patches like this, please use coreutils <at> gnu.org rather
than bug-coreutils.  Using the latter creates a new bug report in the
database and typically requires manual closing, even if only by my
appending "-done" to the number part of the Cc'd address above.

With the following patch, compilation now fails on x86-based systems:

sort.c: In function 'key_warnings':
sort.c:2335: error: negative width in bit-field 'verify_error_if_negative_size__'
sort.c:2335: error: negative width in bit-field 'verify_error_if_negative_size__'
sort.c:2336: error: negative width in bit-field 'verify_error_if_negative_size__'
sort.c:2336: error: negative width in bit-field 'verify_error_if_negative_size__'
sort.c:2339: error: negative width in bit-field 'verify_error_if_negative_size__'
sort.c:2339: error: negative width in bit-field 'verify_error_if_negative_size__'
sort.c:2340: error: negative width in bit-field 'verify_error_if_negative_size__'
sort.c:2340: error: negative width in bit-field 'verify_error_if_negative_size__'

I'll add comments.

diff --git a/lib/inttostr.h b/lib/inttostr.h
index 4f74968..757ac30 100644
--- a/lib/inttostr.h
+++ b/lib/inttostr.h
@@ -21,6 +21,7 @@
 #include <sys/types.h>

 #include "intprops.h"
+#include "verify.h"

 #ifndef __GNUC_PREREQ
 # if defined __GNUC__ && defined __GNUC_MINOR__
@@ -44,3 +45,30 @@ char *inttostr (int, char *) __attribute_warn_unused_result__;
 char *offtostr (off_t, char *) __attribute_warn_unused_result__;
 char *uinttostr (unsigned int, char *) __attribute_warn_unused_result__;
 char *umaxtostr (uintmax_t, char *) __attribute_warn_unused_result__;
+
+#ifndef inttype
+# define imaxtostr(n,s) \
+   ((void) verify_true (sizeof (s) == sizeof (char *) \
+                        || INT_BUFSIZE_BOUND (intmax_t) <= sizeof (s)), \
+    (imaxtostr) (n, s))
+
+# define inttostr(n,s) \
+   ((void) verify_true (sizeof (s) == sizeof (char *) \
+                        || INT_BUFSIZE_BOUND (int) <= sizeof (s)), \
+    (inttostr) (n, s))
+
+# define offtostr(n,s) \
+   ((void) verify_true (sizeof (s) == sizeof (char *) \
+                        || INT_BUFSIZE_BOUND (off_t) <= sizeof (s)), \
+    (offtostr) (n, s))
+
+# define uinttostr(n,s) \
+   ((void) verify_true (sizeof (s) == sizeof (char *) \
+                        || INT_BUFSIZE_BOUND (unsigned int) <= sizeof (s)), \
+    (uinttostr) (n, s))
+
+# define umaxtostr(n,s) \
+   ((void) verify_true (sizeof (s) == sizeof (char *) \
+                        || INT_BUFSIZE_BOUND (uintmax_t) <= sizeof (s)), \
+    (umaxtostr) (n, s))
+#endif

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

From: Pádraig Brady <P <at> draigBrady.com>
To: Paul Eggert <eggert <at> cs.ucla.edu>
Cc: 7213 <at> debbugs.gnu.org
Subject: Re: bug#7213: [PATCH] sort: fix buffer overrun on 32-bit hosts when
	warning re obsolete keys
Date: Thu, 14 Oct 2010 11:27:24 +0100

Ah I wasn't aware anytostr put the numbers at the end of the buffer.
That's confirmed by replacing the tmp buffer with one on the heap
and running:

$ valgrind ./src/sort --debug  +0 -1 /dev/null
==25943== Memcheck, a memory error detector.
==25943== Invalid write of size 1
==25943==    at 0x8051F25: umaxtostr (anytostr.c:34)
==25943==    by 0x8050D95: main (sort.c:2336)
==25943==  Address 0x4026f64 is 9 bytes after a block of size 11 alloc'd

On 14/10/10 08:12, Paul Eggert wrote:
> * src/sort.c (key_warnings): Local buffer should be of size
> INT_BUFSIZE_BOUND (uintmax_t), not INT_BUFSIZE_BOUND (sword).
> This bug was discovered by running 'make check' on a 32-bit
> Solaris 8 sparc host, using Sun cc.

So the test failed due to buffer overrun side effects?

thanks!
Pádraig.

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

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: Pádraig Brady <P <at> draigBrady.com>
Cc: 7213 <at> debbugs.gnu.org
Subject: Re: bug#7213: [PATCH] sort: fix buffer overrun on 32-bit hosts when
	warning re obsolete keys
Date: Thu, 14 Oct 2010 09:37:08 -0700

On 10/14/10 03:27, Pádraig Brady wrote:
> So the test failed due to buffer overrun side effects?

I think so, yes, though I didn't investigate the details.

On 10/14/10 02:37, Jim Meyering wrote:
> With the following patch, compilation now fails on x86-based systems:
> 
> sort.c: In function 'key_warnings':
> sort.c:2335: error: negative width in bit-field 'verify_error_if_negative_size__'
> sort.c:2335: error: negative width in bit-field 'verify_error_if_negative_size__'
...

I assume this is against the unpatched sort.c.  It's nice that
it generates a diagnostic, but why is it generating duplicate
diagnostics for each error?

> BTW, for fyi-style patches like this,
> please use coreutils <at> gnu.org rather than bug-...

Sorry about posting to bug-coreutils; I forgot that I was
supposed to send it to coreutils.  But even if I had remembered,
I thought I was supposed to send patches to coreutils only if I
had applied them, under the theory that the bug had already been
fixed.  So the real rule is: send patches to coreutils, and
bug reports without patches to bug-coreutils?

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

From: Jim Meyering <jim <at> meyering.net>
To: Paul Eggert <eggert <at> cs.ucla.edu>
Cc: 7213 <at> debbugs.gnu.org, Pádraig Brady <P <at> draigBrady.com>
Subject: Re: bug#7213: [PATCH] sort: fix buffer overrun on 32-bit hosts when
	warning re obsolete keys
Date: Thu, 14 Oct 2010 21:48:54 +0200

Paul Eggert wrote:
> On 10/14/10 03:27, Pádraig Brady wrote:
>> So the test failed due to buffer overrun side effects?
>
> I think so, yes, though I didn't investigate the details.
>
> On 10/14/10 02:37, Jim Meyering wrote:
>> With the following patch, compilation now fails on x86-based systems:
>>
>> sort.c: In function 'key_warnings':
>> sort.c:2335: error: negative width in bit-field 'verify_error_if_negative_size__'
>> sort.c:2335: error: negative width in bit-field 'verify_error_if_negative_size__'
> ...
>
> I assume this is against the unpatched sort.c.  It's nice that

Right.

> it generates a diagnostic, but why is it generating duplicate
> diagnostics for each error?

That surprised me, too.
umaxtostr is used there as an argument to stpcpy,

          po = stpcpy (stpcpy (po, "+"), umaxtostr (sword, tmp));

and stpcpy happens to be a macro on glibc-based systems.

>> BTW, for fyi-style patches like this,
>> please use coreutils <at> gnu.org rather than bug-...
>
> Sorry about posting to bug-coreutils; I forgot that I was
> supposed to send it to coreutils.  But even if I had remembered,
> I thought I was supposed to send patches to coreutils only if I
> had applied them, under the theory that the bug had already been
> fixed.  So the real rule is: send patches to coreutils, and
> bug reports without patches to bug-coreutils?

Actually, you're welcome to send them to either,
but if you send them to bug-coreutils, please close
the ticket once your patch has been pushed.

Previous Next

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