[PATCH/RFA] _fflush_r check seek result fix

[Jeff Johnston]

On 27/10/09 03:49 PM, Corinna Vinschen wrote:
> On Oct 27 15:14, Jeff Johnston wrote:
>> On 23/10/09 09:08 AM, Corinna Vinschen wrote:
>>> Hi,
>>>
>>> there was a short thread on the Cygwin list, in which it turned out that
>>> fclose on certain Cygwin devices opened for readin could return an
>>> error: http://cygwin.com/ml/cygwin/2009-10/msg00562.html
>>>
>>> I tracked it down to the _fflush_r function.  _fflush_r calls fp->seek,
>>> basically like this:
>>>
>>>    curoff = fp->_seek (0, SEEK_CUR);
>>>    curoff = -= fp->_r; // Take buffer position into account
>>>    tmp = (fp->_seek (curoff, SEEK_SET) == curoff)
>>>    if (tmp)
>>>      // Success
>>>    else
>>>      // Failure
>>>
>>> This code ignores the possibility that the offset returned by seek does
>>> not exactly match the desired position.  This result is no error
>>> condition, especially when taking devices into account.  It's especially
>>> no error if lseek returns a negative offset on character special
>>> devices.
>>> Please note that lseek on the Cygwin devices mentioned in the above
>>> thread behave exactly like their Linux counterparts.  Thus, the same
>>> _fseek_r would also treat the Linux behaviour as error.
>>> [...]
>>
>> So for these devices, you're saying seek is supported, but they
>> cannot return their position (correct)?  Otherwise, if seek isn't
>> supported, why don't they set ESPIPE?
>
> Per POSIX, ESPIPE has to be returned if the descriptor references
> a pipe, a fifo, or a socket.  POSIX does not require this for block
> or character special devices.  Quote:
>
>    The behavior of lseek() on devices which are incapable of seeking is
>    implementation-defined. The value of the file offset associated with
>    such a device is undefined.
>

Glibc adds to this under info lseek:

     `ESPIPE'
           The FILEDES corresponds to an object that cannot be
           positioned, such as a pipe, FIFO or terminal device.
           (POSIX.1 specifies this error only for pipes and FIFOs, but
           in the GNU system, you always get `ESPIPE' if the object is
           not seekable.)

I interpret this that Linux always return ESPIPE for unseekable devices.

> Please note that Cygwin tries to emulate Linux behaviour in the first
> place.  So the Cygwin devices behave just like the same Linux devices.
>
> Try this code on Linux...
>
>    #include<stdio.h>
>    #include<sys/fcntl.h>
>    #include<string.h>
>    #include<errno.h>
>
>    int
>    main (int argc, char **argv)
>    {
>      int fd = open (argv[1], O_RDONLY);
>      if (fd>= 0)
>        {
> 	char buf[65536];
> 	off_t pos = lseek (fd, 0, SEEK_CUR);
> 	printf ("pos(0,CUR): %lld\n", (long long) pos);
> 	pos = read (fd, buf, 65536);
> 	printf ("pos(read): %lld\n", (long long) pos);
> 	pos = lseek (fd, 0, SEEK_CUR);
> 	printf ("pos(0, CUR): %lld\n", (long long) pos);
> 	pos = lseek (fd, -65528, SEEK_CUR);
> 	printf ("pos(-65528,CUR): %lld\n", (long long) pos);
> 	pos = lseek (fd, 0, SEEK_CUR);
> 	printf ("pos(0, CUR): %lld\n", (long long) pos);
> 	close (fd);
>        }
>      else
>        printf ("open: %d<%s>\n", errno, strerror (errno));
>      return 0;
>    }
>
> ...with the following devices:
>
>    $ gcc -o seektest seektest.c
>
>    $ ./seektest /dev/urandom
>    pos(0,CUR): 0
>    pos(read): 65536
>    pos(0, CUR): 0
>    pos(-65528,CUR): -65528
>    pos(0, CUR): -65528
>
>    $ ./seektest /dev/zero
>    pos(0,CUR): 0
>    pos(read): 65536
>    pos(0, CUR): 0
>    pos(-65528,CUR): 0
>    pos(0, CUR): 0
>
>    $ ./seektest /dev/full
>    pos(0,CUR): 0
>    pos(read): 65536
>    pos(0, CUR): 0
>    pos(-65528,CUR): 0
>    pos(0, CUR): 0
>
> As you can see, lseek() on these devices does not return an error.
> However, the position returned to the calling function doesn't match the
> position you'd expect if you made the same operation on a file.
>

I have no issue with removing the check for position equality.  I get a 
slightly different result on Fedora Linux with /dev/urandom.  I get back 
-1 and errno set to 22 (Invalid argument) for the seek of -65528.

>> Above the code you are changing is a check that either takes the
>> file offset that has been tracked or tries to calculate it.  If it
>> tries to calculate and -1 is returned, it returns 0 if ESPIPE,
>> otherwise, it returns failure (it fails to reset errno in the case
>> of the ignored ESPIPE).
>>
>> Should that logic be changed to match yours or should your new logic
>> do the same? (i.e. treat ESPIPE as ok and otherwise -1 is failure).
>
> I'm not sure I can follow.  How are you going to change that code if
> the curoff value doesn't reflect an error condition at all?!?
>

Your change allows -1 to be returned if no errno is set.  None of the 
above tests exhibit this, but you may know differently.  If -1 and 
errno-not-set is possible, then the earlier check in fflush where -1 
returned from seek causes failure unless ESPIPE is set, might require 
similar checking to what you are proposing.

There might be an issue if any libgloss implementation does not properly 
set errno on an lseek failure, but I know none off-hand and have only 
checked a handful of platforms.

If an unseekable device returns -1 and sets ESPIPE, shouldn't you ignore 
this situation as is done above in the same code? (i.e. there is no 
requirement to do a seek once the read buffer is cleared).

So one possible solution would be to change the added check to be (rc != 
-1 || ptr->errno == ESPIPE || ptr->errno == 0), with the last check for 
errno being removed if it is not feasible that -1 will be returned with 
errno 0.

-- Jeff J.