Jim Meyering wrote: > Paul Eggert wrote: >>> + unsigned int max_digit_string_len >>> + = (suffix >>> + ? max_out (suffix) >>> + : MAX (INT_STRLEN_BOUND (unsigned int), digits)); >> >> That should be size_t, not unsigned int, since max_out >> returns a size_t, and it could return a value greater than > > Good catch. > > In addition, it should be using snprintf, not sprintf, on principle. > >> UINT_MAX. For example, the user might run "csplit -b %4294967296d" >> and on a 64-bit host max_out will return UINTMAX + 1. >> >> While we're on the subject of undefined printf behavior, perhaps >> we should be rejecting any attempt to use a printf format like >> %4294967296d that uses a width or precision greater than INT_MAX? >> POSIX seems to say that such a format should work, but I'll bet >> nobody does it right (glibc doesn't). For safety we might want >> to be truncating large widths or precisions to INT_MAX, or >> rejecting them. > > Here are two patches. First follows your suggestions.
The test included there was an old, useless version. This one provokes a buffer overrun without the fix. >From d1f178a7222f5ec4f77c1f307f42d40e7552e569 Mon Sep 17 00:00:00 2001 From: Jim Meyering <[email protected]> Date: Wed, 10 Nov 2010 21:30:26 +0100 Subject: [PATCH] csplit: don't misbehave given a format like %4294967296d * src/csplit.c (main): Use size_t for length, not unsigned int. Also, reject options that would require a file name buffer longer than INT_MAX. * tests/misc/csplit-1000: Test for this. Suggested by Paul Eggert. --- src/csplit.c | 8 ++++++-- tests/misc/csplit-1000 | 7 +++++++ 2 files changed, 13 insertions(+), 2 deletions(-) diff --git a/src/csplit.c b/src/csplit.c index 57543f0..e510cb5 100644 --- a/src/csplit.c +++ b/src/csplit.c @@ -1372,11 +1372,15 @@ main (int argc, char **argv) usage (EXIT_FAILURE); } - unsigned int max_digit_string_len + size_t max_digit_string_len = (suffix ? max_out (suffix) : MAX (INT_STRLEN_BOUND (unsigned int), digits)); - filename_space = xmalloc (strlen (prefix) + max_digit_string_len + 1); + + size_t buf_len = strlen (prefix) + max_digit_string_len + 1; + if (INT_MAX < buf_len || INT_MAX < max_digit_string_len) + error (EXIT_FAILURE, 0, _("invalid options")); + filename_space = xmalloc (buf_len); set_input_file (argv[optind++]); diff --git a/tests/misc/csplit-1000 b/tests/misc/csplit-1000 index 259d514..a343c81 100755 --- a/tests/misc/csplit-1000 +++ b/tests/misc/csplit-1000 @@ -26,4 +26,11 @@ seq 1000 | csplit - '/./' '{*}' || fail=1 test -f xx1000 || fail=1 test -f xx1001 && fail=1 +# This would exercise another buffer overrun, +# when 4294967296 is mistakenly wrapped to 0, leaving +# an undersized filename_space buffer. +seq 1000 | csplit csplit - -b %4294967296d '/./' '{*}' 2> err && fail=1 +echo csplit: invalid options > exp || framework_failure_ +compare exp err || fail=1 + Exit $fail -- 1.7.3.2.4.g60aa9
