bug#7357: csplit: memory exhausted when using stdout / pipe instead of a file

[Jim Meyering]

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 <address@hidden>
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 <address@hidden>
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