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

[Pádraig Brady]

unarchive 13530
stop

On 05/26/2013 11:07 PM, Jim Meyering wrote:
> 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"));

Oh right it's coming back to me a bit now.
So by removing these upfront checks where possible,
it only makes sense of the program could under different
free mem available, fulfil the operation up to the specified limit.
In this case though, the program could never fulfil the request,
so it's better to fail early as is the case in the code currently?

> 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
> 

So the test would have to be adjusted to take the min(SIZE_MAX, OFF_T_MAX)?

HEAD_TAIL_LIMIT=$(printf '%s\n' $SIZE_MAX $OFF_T_MAX | sort -g | head -n1)

cheers,
Pádraig.