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

[Jim Meyering]

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