[PATCH/RFA] _fflush_r check seek result fix

[Jeff Johnston]

On 28/10/09 06:47 AM, Corinna Vinschen wrote:
> On Oct 28 11:06, Corinna Vinschen wrote:
>> 	* libc/stdio/fflush.c (_fflush_r): Store old errno to check for
>> 	low-level seek error condition.  Restore old errno in case of
>> 	success.  Don't use new position after seek as error condition,
>> 	rather check for return value of -1 and errno.  Only seek to
>> 	positive offsets, explain why.  Only set fp->_offset if errno
>> 	is not ESPIPE.
>
> There's a still a problem with this patch.  Assuming the offset returned
> by the first seek on a regular file would result in a negative offset
> after fp->_r and fp->_ur have been subtracted, then the new position
> stored in fp->_offset will be incorrect.  Therefore, the code must check
> for this condition before changing curoff.  The below patch does this,
> so fp->_offset will not be set to an incorrect value.
>
> Alternatively, I'm wondering if it isn't easier to ignore the EINVAL
> condition in the second invocation of seek just like ESPIPE.
>

I think you need to ignore the EINVAL regardless as it doesn't indicate 
the seek had a failure as much as ESPIPE doesn't.

-- Jeff J.

>
> Corinna
>
>
> Index: libc/stdio/fflush.c
> ===================================================================
> RCS file: /cvs/src/src/newlib/libc/stdio/fflush.c,v
> retrieving revision 1.14
> diff -u -p -r1.14 fflush.c
> --- libc/stdio/fflush.c	22 Jul 2009 02:17:12 -0000	1.14
> +++ libc/stdio/fflush.c	28 Oct 2009 10:44:48 -0000
> @@ -115,13 +115,18 @@ _DEFUN(_fflush_r, (ptr, fp),
>            to miss a code scenario.  */
>         if ((fp->_r>  0 || fp->_ur>  0)&&  fp->_seek != NULL)
>   	{
> -	  int tmp;
> +	  int tmp_errno;
>   #ifdef __LARGE64_FILES
>   	  _fpos64_t curoff;
>   #else
>   	  _fpos_t curoff;
>   #endif
>
> +	  /* Save last errno and set errno to 0, so we can check if a device
> +	     returns with a valid position -1.  We restore the last errno if
> +	     no other error condition has been encountered. */
> +	  tmp_errno = ptr->_errno;
> +	  ptr->_errno = 0;
>   	  /* Get the physical position we are at in the file.  */
>   	  if (fp->_flags&  __SOFF)
>   	    curoff = fp->_offset;
> @@ -135,11 +140,14 @@ _DEFUN(_fflush_r, (ptr, fp),
>   	      else
>   #endif
>   		curoff = fp->_seek (ptr, fp->_cookie, 0, SEEK_CUR);
> -	      if (curoff == -1L)
> +	      if (curoff == -1L&&  ptr->_errno != 0)
>   		{
>   		  int result = EOF;
>   		  if (ptr->_errno == ESPIPE)
> -		    result = 0;
> +		    {
> +		      result = 0;
> +		      ptr->_errno = tmp_errno;
> +		    }
>   		  else
>   		    fp->_flags |= __SERR;
>   		  _funlockfile (fp);
> @@ -148,27 +156,44 @@ _DEFUN(_fflush_r, (ptr, fp),
>               }
>             if (fp->_flags&  __SRD)
>               {
> +#ifdef __LARGE64_FILES
> +	      _fpos64_t newoff = curoff;
> +#else
> +	      _fpos_t newoff = curoff;
> +#endif
>                 /* Current offset is at end of buffer.  Compensate for
>                    characters not yet read.  */
> -              curoff -= fp->_r;
> +              newoff -= fp->_r;
>                 if (HASUB (fp))
> -                curoff -= fp->_ur;
> +                newoff -= fp->_ur;
> +	      /* Make sure that curoff still reflects the current file
> +	         position if we don't seek.  */
> +	      if (newoff>= 0)
> +		curoff = newoff;
>               }
> -	  /* Now physically seek to after byte last read.  */
> +	  /* Now physically seek to after byte last read.
> +	     If curoff is<  0, fp is either pointing to a non-seekable device,
> +	     or the application already called lseek and invalidated the
> +	     information in fp.  Seeking doesn't make sense in this case.  */
> +	  if (curoff>= 0)
> +	    {
>   #ifdef __LARGE64_FILES
> -	  if (fp->_flags&  __SL64)
> -	    tmp = (fp->_seek64 (ptr, fp->_cookie, curoff, SEEK_SET) == curoff);
> -	  else
> +	      if (fp->_flags&  __SL64)
> +		curoff = fp->_seek64 (ptr, fp->_cookie, curoff, SEEK_SET);
> +	      else
>   #endif
> -	    tmp = (fp->_seek (ptr, fp->_cookie, curoff, SEEK_SET) == curoff);
> -	  if (tmp)
> +		curoff = fp->_seek (ptr, fp->_cookie, curoff, SEEK_SET);
> +	    }
> +	  if (curoff != -1 || ptr->_errno == 0 || ptr->_errno == ESPIPE)
>   	    {
>   	      /* Seek successful.  We can clear read buffer now.  */
>   	      fp->_flags&= ~__SNPT;
>   	      fp->_r = 0;
>   	      fp->_p = fp->_bf._base;
> -	      if (fp->_flags&  __SOFF)
> +	      if ((fp->_flags&  __SOFF)
> +		&&  (curoff != -1 || ptr->_errno != ESPIPE))
>   		fp->_offset = curoff;
> +	      ptr->_errno = tmp_errno;
>   	      if (HASUB (fp))
>   		FREEUB (ptr, fp);
>   	    }
>