GNU bug report logs - #11631
Head command does not position file pointer correctly for negative line count

Previous Next

Full log

Message #49 received at 11631 <at> debbugs.gnu.org (full text, mbox):

From: Jim Meyering <jim <at> meyering.net>
To: Eric Blake <eblake <at> redhat.com>
Cc: Anoop Sharma <sendtoanoop <at> gmail.com>, 11631 <at> debbugs.gnu.org
Subject: Re: bug#11631: closed (Re: bug#11631: Head command does not position
	file pointer correctly for negative line count)
Date: Wed, 06 Jun 2012 15:09:42 +0200

Eric Blake wrote:
> On 06/06/2012 02:02 AM, Jim Meyering wrote:
>
>> +++ b/src/head.c
>> @@ -663,10 +663,9 @@ elide_tail_lines_seekable (const char *pretty_filename, int fd,
>>                  }
>>
>>                /* Output the initial portion of the buffer
>> -                 in which we found the desired newline byte.
>> -                 Don't bother testing for failure for such a small amount.
>> -                 Any failure will be detected upon close.  */
>> -              fwrite (buffer, 1, n + 1, stdout);
>> +                 in which we found the desired newline byte.  */
>> +              if (fwrite (buffer, 1, n + 1, stdout) < n + 1)
>> +                error (EXIT_FAILURE, errno, _("write error"));
>
> Is testing for fwrite() sufficient?  Shouldn't you actually be testing
> for fflush() errors, since fwrite() buffers the output and might not

Hi Eric,

There is no need (or desire) for explicit fflush here.
Relying on our atexit-invoked close_stdout is enough.

> actually encounter an error until it flushes?  Or even on NFS, where
> fflush() may succeed but fclose() fails?  In other words, our atexit()
> handler for detecting fclose() failure will already catch things; we may
> still be in a situation where fwrite() succeeds, we then do lseek(), but
> fclose() fails, in spite of our efforts.  I don't see how this patch
> improves anything, other than earlier error reporting.

Adding the fwrite test would be important solely if we were required
to diagnose-with-errno a failure that occurs when this fwrite actually
happens to perform a write syscall.  Without the new test, the fwrite
can fail, leading the eventual close_stdout call to emit the dumbed-down
diagnostic (no strerror part) that it must emit when ferror indicates
a previous error yet fclose does not fail.

It's a hard call.  This fwrite is not in a loop (it's printing only
a fraction of a read buffer), so the cost of the test is negligible,
but similarly, there's a relatively small risk that, assuming we'll get
a write failure, it will happen while printing this relatively small
amount of data.

Thanks for making me think more about it.
There are many other unchecked uses of fwrite in head.c,
and I cannot justify adding tests for all of them.

As you probably saw, I have retracted the proposed change.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.