Subject: bug#7213: closed (Re: bug#7213: [PATCH] sort: fix buffer overrun on 32-bit hosts when warning re obsolete keys)

From: [email-masked] (GNU bug Tracking System)
Date: Thu, 14 Oct 2010 09:34:02 +0000

Your bug report

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

which was filed against the coreutils package, has been closed.

The explanation is attached below, along with your original report.
If you require more details, please reply to [email-masked].

-- 
7213: http://debbugs.gnu.org/cgi/bugreport.cgi?bug=7213
GNU Bug Tracking System
Contact [email-masked] with problems

Subject: Re: bug#7213: [PATCH] sort: fix buffer overrun on 32-bit hosts when warning re obsolete keys
From: Jim Meyering <[email-masked]>
To: Paul Eggert <[email-masked]>
Cc: [email-masked]
Date: Thu, 14 Oct 2010 11:37:17 +0200
List-Id: <debbugs-submit.debbugs.gnu.org>

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 [email-masked] 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 [email-masked] 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

Subject: [PATCH] sort: fix buffer overrun on 32-bit hosts when warning re obsolete keys
From: Paul Eggert <[email-masked]>
To: [email-masked]
Date: Thu, 14 Oct 2010 00:12:23 -0700
List-Id: <debbugs-submit.debbugs.gnu.org>

* 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