diff mbox

Avoid SIGFPE in wordexp [BZ #18100]

Message ID 550C2BE2.4000108@redhat.com
State Committed
Headers

Commit Message

Florian Weimer March 20, 2015, 2:17 p.m. UTC 
  This patch avoids SIGFPE in wordexp on some architectures.  Both
division by zero and the LONG_MIN / (-1) overflow are covered.

Comments

Mike Frysinger March 20, 2015, 3:37 p.m. UTC | #1 
On 20 Mar 2015 15:17, Florian Weimer wrote:

change looks fine ... two minor questions below

> --- a/posix/wordexp-test.c
> +++ b/posix/wordexp-test.c
> +  /* Integer overflow in division.  */
> +  {
> +    static const char *const numbers[] = {
> +      "0",
> +      "1",
> +      "65536",
> +      "2147483648",
> +      "4294967296"
> +      "9223372036854775808",
> +      "18446744073709551616",
> +      "170141183460469231731687303715884105728",
> +      "340282366920938463463374607431768211456",
> +      NULL
> +    };

should there be tests for negative numeric limits ?

> +    for (const char *const *num = numbers; *num; ++num)

could use ARRAY_SIZE(numbers) rather than a NULL sentinel
-mike
Florian Weimer March 20, 2015, 3:41 p.m. UTC | #2 
On 03/20/2015 04:37 PM, Mike Frysinger wrote:
> On 20 Mar 2015 15:17, Florian Weimer wrote:
> 
> change looks fine ... two minor questions below

Thanks.

>> --- a/posix/wordexp-test.c +++ b/posix/wordexp-test.c +  /*
>> Integer overflow in division.  */ +  { +    static const char
>> *const numbers[] = { +      "0", +      "1", +      "65536", +
>> "2147483648", +      "4294967296" +      "9223372036854775808", +
>> "18446744073709551616", +
>> "170141183460469231731687303715884105728", +
>> "340282366920938463463374607431768211456", +      NULL +    };
> 
> should there be tests for negative numeric limits ?

These tests are for then negative limit, the minus sign is patched
into the pattern.  It will be gone on success, that's why it's not
included in the reference array.

>> +    for (const char *const *num = numbers; *num; ++num)

(“*num” should be “*num != NULL”.)

> could use ARRAY_SIZE(numbers) rather than a NULL sentinel

Sadly, glibc does not seem to provide an ARRAY_SIZE macro for global use.
Roland McGrath March 20, 2015, 4:57 p.m. UTC | #3 
> Sadly, glibc does not seem to provide an ARRAY_SIZE macro for global use.

If you want one, put it in libc-internal.h.
Paul Eggert March 20, 2015, 5:21 p.m. UTC | #4 
Roland McGrath wrote:
>> Sadly, glibc does not seem to provide an ARRAY_SIZE macro for global use.
>
> If you want one, put it in libc-internal.h.
>

I suggest avoiding the name 'ARRAY_SIZE', as a size is typically a byte count, 
e.g., what 'sizeof' returns.  To help avoid this confusion GNU Emacs uses the 
name 'ARRAYELTS' internally, in src/lisp.h:

#define ARRAYELTS(arr) (sizeof (arr) / sizeof (arr)[0])

and perhaps glibc should do the same.
Mike Frysinger March 20, 2015, 6:13 p.m. UTC | #5 
On 20 Mar 2015 10:21, Paul Eggert wrote:
> Roland McGrath wrote:
> >> Sadly, glibc does not seem to provide an ARRAY_SIZE macro for global use.
> >
> > If you want one, put it in libc-internal.h.
> 
> I suggest avoiding the name 'ARRAY_SIZE', as a size is typically a byte count, 
> e.g., what 'sizeof' returns.  To help avoid this confusion GNU Emacs uses the 
> name 'ARRAYELTS' internally, in src/lisp.h:
> 
> #define ARRAYELTS(arr) (sizeof (arr) / sizeof (arr)[0])
> 
> and perhaps glibc should do the same.

i can't say i've ever seen that confusion.  considering glibc tends to have a 
lot of kernel hackers, and libiberty itself uses ARRAY_SIZE, i think it'd be
more natural for us to use that.  i'm not sure i've ever seen someone refer to
emacs as the way to do things before ;).
-mike
Florian Weimer March 23, 2015, 3:21 p.m. UTC | #6 
On 03/20/2015 04:37 PM, Mike Frysinger wrote:
> On 20 Mar 2015 15:17, Florian Weimer wrote:
> 
> change looks fine ... two minor questions below

Thanks, I interpreted this as approval and committed the change.
diff mbox

Patch

From 0122b8ef489a7488b469bbb80a04b843c307765c Mon Sep 17 00:00:00 2001
From: Florian Weimer <fweimer@redhat.com>
Date: Fri, 20 Mar 2015 15:13:13 +0100
Subject: [PATCH] Avoid SIGFPE in wordexp [BZ #18100]

Check for a zero divisor and integer overflow before performing
division in arithmetic expansion.

2015-03-20  Florian Weimer  <fweimer@redhat.com>

	[BZ #18100]
	* posix/wordexp.c (eval_expr_multdiv): Check for division by zero
	and integer overflow.
	* posix/wordexp-test.c (test_case): Add divide-by-zero test.
	(main): Add integer overflow tests.
	* manual/pattern.texi (Calling Wordexp): Document additional use
	for WRDE_SYNTAX.

diff --git a/NEWS b/NEWS
index 4c18bf4..63075a8 100644
--- a/NEWS
+++ b/NEWS
@@ -14,8 +14,8 @@  Version 2.22
   17621, 17628, 17631, 17711, 17776, 17779, 17792, 17836, 17912, 17916,
   17932, 17944, 17949, 17964, 17965, 17967, 17969, 17978, 17987, 17991,
   17996, 17998, 17999, 18019, 18020, 18029, 18030, 18032, 18036, 18038,
-  18039, 18042, 18043, 18046, 18047, 18068, 18080, 18093, 18104, 18110,
-  18111, 18128, 18138.
+  18039, 18042, 18043, 18046, 18047, 18068, 18080, 18093, 18100, 18104,
+  18110, 18111, 18128, 18138.
 
 * Character encoding and ctype tables were updated to Unicode 7.0.0, using
   new generator scripts contributed by Pravin Satpute and Mike FABIAN (Red
diff --git a/manual/pattern.texi b/manual/pattern.texi
index da848c3..d1b9275 100644
--- a/manual/pattern.texi
+++ b/manual/pattern.texi
@@ -2006,7 +2006,8 @@  allocate room for.
 @comment POSIX.2
 @item WRDE_SYNTAX
 There was a syntax error in the input string.  For example, an unmatched
-quoting character is a syntax error.
+quoting character is a syntax error.  This error code is also used to
+signal division by zero and overflow in arithmetic expansion.
 @end table
 @end deftypefun
 
diff --git a/posix/wordexp-test.c b/posix/wordexp-test.c
index 0a353a4..73f1b7d 100644
--- a/posix/wordexp-test.c
+++ b/posix/wordexp-test.c
@@ -237,6 +237,7 @@  struct test_case_struct
     { WRDE_SYNTAX, NULL, "`\\", 0, 0, { NULL, }, IFS },     /* BZ 18042  */
     { WRDE_SYNTAX, NULL, "${", 0, 0, { NULL, }, IFS },      /* BZ 18043  */
     { WRDE_SYNTAX, NULL, "L${a:", 0, 0, { NULL, }, IFS },   /* BZ 18043#c4  */
+    { WRDE_SYNTAX, NULL, "$[1/0]", WRDE_NOCMD, 0, {NULL, }, IFS }, /* BZ 18100 */
 
     { -1, NULL, NULL, 0, 0, { NULL, }, IFS },
   };
@@ -362,6 +363,45 @@  main (int argc, char *argv[])
 	++fail;
     }
 
+  /* Integer overflow in division.  */
+  {
+    static const char *const numbers[] = {
+      "0",
+      "1",
+      "65536",
+      "2147483648",
+      "4294967296"
+      "9223372036854775808",
+      "18446744073709551616",
+      "170141183460469231731687303715884105728",
+      "340282366920938463463374607431768211456",
+      NULL
+    };
+
+    for (const char *const *num = numbers; *num; ++num)
+      {
+	wordexp_t p;
+	char pattern[256];
+	snprintf (pattern, sizeof (pattern), "$[(-%s)/(-1)]", *num);
+	int ret = wordexp (pattern, &p, WRDE_NOCMD);
+	if (ret == 0)
+	  {
+	    if (p.we_wordc != 1 || strcmp (p.we_wordv[0], *num) != 0)
+	      {
+		printf ("Integer overflow for \"%s\" failed", pattern);
+		++fail;
+	      }
+	    wordfree (&p);
+	  }
+	else if (ret != WRDE_SYNTAX)
+	  {
+	    printf ("Integer overflow for \"%s\" failed with %d",
+		    pattern, ret);
+	    ++fail;
+	  }
+      }
+  }
+
   puts ("tests completed, now cleaning up");
 
   /* Clean up */
diff --git a/posix/wordexp.c b/posix/wordexp.c
index f6062d5..e711d43 100644
--- a/posix/wordexp.c
+++ b/posix/wordexp.c
@@ -617,6 +617,10 @@  eval_expr_multdiv (char **expr, long int *result)
 	  if (eval_expr_val (expr, &arg) != 0)
 	    return WRDE_SYNTAX;
 
+	  /* Division by zero or integer overflow.  */
+	  if (arg == 0 || (arg == -1 && *result == LONG_MIN))
+	    return WRDE_SYNTAX;
+
 	  *result /= arg;
 	}
       else break;
-- 
2.1.0