--- Begin Message ---
|
Subject: |
[PATCH] sort: fix buffer overrun on 32-bit hosts when warning re obsolete keys |
|
Date: |
Thu, 14 Oct 2010 00:12:23 -0700 |
|
User-agent: |
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1.12) Gecko/20100915 Thunderbird/3.0.8 |
* 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
--- End Message ---
--- Begin Message ---
|
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 address@hidden 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 address@hidden 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
--- End Message ---