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 second cleans up my just-added test. >From e51561440446cd23c8c60b96d4712263423fd39c Mon Sep 17 00:00:00 2001 From: Jim Meyering <[email protected]> Date: Wed, 10 Nov 2010 21:30:26 +0100 Subject: [PATCH 1/2] 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-abuse: Test for this. * tests/Makefile.am (TESTS): Add misc/csplit-abuse. Suggested by Paul Eggert. --- src/csplit.c | 8 ++++++-- tests/Makefile.am | 1 + tests/misc/csplit-abuse | 28 ++++++++++++++++++++++++++++ 3 files changed, 35 insertions(+), 2 deletions(-) create mode 100755 tests/misc/csplit-abuse 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/Makefile.am b/tests/Makefile.am index a3a30b6..ceb8de1 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -173,6 +173,7 @@ TESTS = \ misc/comm \ misc/csplit \ misc/csplit-1000 \ + misc/csplit-abuse \ misc/date-sec \ misc/dircolors \ misc/df \ diff --git a/tests/misc/csplit-abuse b/tests/misc/csplit-abuse new file mode 100755 index 0000000..3448b9a --- /dev/null +++ b/tests/misc/csplit-abuse @@ -0,0 +1,28 @@ +#!/bin/sh +# ensure that an outrageous format evokes an error + +# Copyright (C) 2010 Free Software Foundation, Inc. + +# This program is free software: you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation, either version 3 of the License, or +# (at your option) any later version. + +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. + +# You should have received a copy of the GNU General Public License +# along with this program. If not, see <http://www.gnu.org/licenses/>. + +. "${srcdir=.}/init.sh"; path_prepend_ ../src +test "$VERBOSE" = yes && csplit --version + +echo csplit: invalid options > exp || framework_failure_ + +# Require failure. +csplit - -b %4294967295d x 1 2> err && fail=1 +compare exp err || fail=1 + +Exit $fail -- 1.7.3.2.4.g60aa9 >From d2c441bb125a18ad949f344db2c192469e2972bb Mon Sep 17 00:00:00 2001 From: Jim Meyering <[email protected]> Date: Wed, 10 Nov 2010 21:54:57 +0100 Subject: [PATCH 2/2] tests: fix comments and --version invocation in new test * tests/misc/csplit-1000: Fix comments and --version invocation. --- tests/misc/csplit-1000 | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/misc/csplit-1000 b/tests/misc/csplit-1000 index accbe46..259d514 100755 --- a/tests/misc/csplit-1000 +++ b/tests/misc/csplit-1000 @@ -1,5 +1,5 @@ #!/bin/sh -# various csplit tests +# cause a 1-byte heap buffer overrun # Copyright (C) 2010 Free Software Foundation, Inc. @@ -17,7 +17,7 @@ # along with this program. If not, see <http://www.gnu.org/licenses/>. . "${srcdir=.}/init.sh"; path_prepend_ ../src -test "$VERBOSE" = yes && FIXME --version +test "$VERBOSE" = yes && csplit --version # Before coreutils-8.7, this would overrun the 6-byte filename_space buffer. # It's hard to detect that without using valgrind, so here, we simply -- 1.7.3.2.4.g60aa9
