| Message ID | 550C2BE2.4000108@redhat.com |
|---|---|
| State | New |
| Headers | show |
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
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.
> Sadly, glibc does not seem to provide an ARRAY_SIZE macro for global use.
If you want one, put it in libc-internal.h.
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.
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
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.
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