This is the mail archive of the
newlib@sourceware.org
mailing list for the newlib project.
Re: [PATCH/RFA] _fflush_r check seek result fix
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.
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);
}
--
Corinna Vinschen
Cygwin Project Co-Leader
Red Hat