Re: bug#13530: head: memory exhausted when printing all from stdin but last P/E bytes

[Jim Meyering]

Jim Meyering wrote:

> Pádraig Brady wrote:
>> On 05/26/2013 06:07 PM, Jim Meyering wrote:
>>> Pádraig Brady wrote:
>>> ...
>>>> I expect to push soon, the attached more complete fix to realloc the array.
>>>
>>> That new test would still fail on 32-bit systems, and on any system
>>> with SIZE_MAX < 1E.  I expect to push the additional fix below.
>>
>> The change looks good thanks.
>> Ideally the test should be system independent,
>> which could be done by also including this in your change:
>>
>> diff --git a/tests/misc/head-c.sh b/tests/misc/head-c.sh
>> index 37a86ce..8b4df5c 100755
>> --- a/tests/misc/head-c.sh
>> +++ b/tests/misc/head-c.sh
>> @@ -19,6 +19,7 @@
>>  . "${srcdir=.}/tests/init.sh"; path_prepend_ ./src
>>  print_ver_ head
>>  require_ulimit_v_
>> +getlimits_
>>
>>  # exercise the fix of 2001-08-18, based on test case from Ian Bruce
>>  echo abc > in || framework_failure_
>> @@ -31,6 +32,6 @@ esac
>>  # Only allocate memory as needed.
>>  # Coreutils <= 8.21 would allocate memory up front
>>  # based on the value passed to -c
>> -(ulimit -v 20000; head --bytes=-E < /dev/null) || fail=1
>> +(ulimit -v 20000; head --bytes=-$OFF_T_MAX < /dev/null) || fail=1
>
> Good idea.
> When I test with that on a system with size_t smaller than uintmax_t,
> I get a segfault, because of this uintmax_t-to-size_t truncation:
>
>     static bool
>     elide_tail_bytes_pipe (const char *filename, int fd, uintmax_t n_elide_0)
>     {
>       size_t n_elide = n_elide_0;
>
> That function really did require that N be no larger than SIZE_MAX.
> So my patch needs further work.
> Thanks!

Here's a better patch.
It includes your test improvement, as well as code to address
the elide-HUGE-from-unseekable-input problem it exposed.

To test it, I made tail --bytes=-E read from a fifo into which I
wrote just over 2^32 bytes on a 32-bit system.  While it didn't
evoke the new "memory exhausted while reading %s" diagnostic,
(not unsurprisingly, in retrospect) it evoked this:

  src/head: memory exhausted

That system had only 3GB of RAM.


>From f561299eb875cec29a263ad963e7bbddfbe034b7 Mon Sep 17 00:00:00 2001
From: Jim Meyering <address@hidden>
Date: Sun, 26 May 2013 09:49:22 -0700
Subject: [PATCH] head: with --bytes=-N don't fail early for large N on 32-bit
 systems
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

* src/head.c (elide_tail_bytes_pipe): This command would fail
unnecessarily on 32-bit systems, where SIZE_MAX < 1E:
  head --bytes=-E < /dev/null
Remove the test that would make us fail for N larger than SIZE_MAX.
Instead, when that happens, pretend we've been given an N slightly
smaller than SIZE_MAX, and if we actually manage to read/allocate
that much data, fail only at the end.
Thanks to Pádraig Brady for a test improvement.
---
 src/head.c           | 24 +++++++++++++++---------
 tests/misc/head-c.sh |  3 ++-
 2 files changed, 17 insertions(+), 10 deletions(-)

diff --git a/src/head.c b/src/head.c
index 00e1be1..b85a5b0 100644
--- a/src/head.c
+++ b/src/head.c
@@ -202,13 +202,18 @@ copy_fd (int src_fd, FILE *o_stream, uintmax_t n_bytes)
 static bool
 elide_tail_bytes_pipe (const char *filename, int fd, uintmax_t n_elide_0)
 {
-  size_t n_elide = n_elide_0;
-
 #ifndef HEAD_TAIL_PIPE_READ_BUFSIZE
 # define HEAD_TAIL_PIPE_READ_BUFSIZE BUFSIZ
 #endif
 #define READ_BUFSIZE HEAD_TAIL_PIPE_READ_BUFSIZE

+  /* If n_elide_0 is too large, then we cannot always allocate enough memory
+     to do the requested job.  In that case, set n_elide to a value near
+     SIZE_MAX but small enough so that below, when we round it up, it does
+     not overflow.  */
+  size_t n_elide = MIN (n_elide_0, (SIZE_MAX - READ_BUFSIZE
+                                    - SIZE_MAX % READ_BUFSIZE));
+
   /* If we're eliding no more than this many bytes, then it's ok to allocate
      more memory in order to use a more time-efficient algorithm.
      FIXME: use a fraction of available memory instead, as in sort.
@@ -221,13 +226,6 @@ elide_tail_bytes_pipe (const char *filename, int fd, 
uintmax_t n_elide_0)
   "HEAD_TAIL_PIPE_BYTECOUNT_THRESHOLD must be at least 2 * READ_BUFSIZE"
 #endif

-  if (SIZE_MAX < n_elide_0 + READ_BUFSIZE)
-    {
-      char umax_buf[INT_BUFSIZE_BOUND (n_elide_0)];
-      error (EXIT_FAILURE, 0, _("%s: number of bytes is too large"),
-             umaxtostr (n_elide_0, umax_buf));
-    }
-
   /* Two cases to consider...
      1) n_elide is small enough that we can afford to double-buffer:
         allocate 2 * (READ_BUFSIZE + n_elide) bytes
@@ -358,6 +356,14 @@ elide_tail_bytes_pipe (const char *filename, int fd, 
uintmax_t n_elide_0)

           if (buffered_enough)
             {
+              if (n_elide_0 != n_elide)
+                {
+                  error (0, 0, _("memory exhausted while reading %s"),
+                         quote (filename));
+                  ok = false;
+                  goto free_mem;
+                }
+
               if (fwrite (b[i_next], 1, n_read, stdout) < n_read)
                 {
                   error (0, errno, _("write error"));
diff --git a/tests/misc/head-c.sh b/tests/misc/head-c.sh
index 37a86ce..8b4df5c 100755
--- a/tests/misc/head-c.sh
+++ b/tests/misc/head-c.sh
@@ -19,6 +19,7 @@
 . "${srcdir=.}/tests/init.sh"; path_prepend_ ./src
 print_ver_ head
 require_ulimit_v_
+getlimits_

 # exercise the fix of 2001-08-18, based on test case from Ian Bruce
 echo abc > in || framework_failure_
@@ -31,6 +32,6 @@ esac
 # Only allocate memory as needed.
 # Coreutils <= 8.21 would allocate memory up front
 # based on the value passed to -c
-(ulimit -v 20000; head --bytes=-E < /dev/null) || fail=1
+(ulimit -v 20000; head --bytes=-$OFF_T_MAX < /dev/null) || fail=1

 Exit $fail
--
1.8.3