Date: Wed, 14 Jun 2006 10:24:41 +0200
From: Corinna Vinschen <corinna-cygwin AT cygwin DOT com>
To: newlib AT sourceware DOT org
Cc: cygwin AT cygwin DOT com
Subject: Reading a write-only file doesn't set error condition (was Re: Cygwin fread on Write-Only File Descriptor returns undefined state)
Message-ID: <20060614082441.GA6326@calimero.vinschen.de>
Reply-To: newlib AT sourceware DOT org
Mail-Followup-To: newlib AT sourceware DOT org, cygwin AT cygwin DOT com
References: <448F9EA1 DOT 7050908 AT tlinx DOT org>
Mime-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <448F9EA1.7050908@tlinx.org>
User-Agent: Mutt/1.4.2i
Mailing-List: contact cygwin-help AT cygwin DOT com; run by ezmlm
Precedence: bulk
Sender: cygwin-owner AT cygwin DOT com
Mail-Followup-To: cygwin AT cygwin DOT com

Thanks for the testcase.  This looks like a small flaw in newlib.
I redirected to the newlib list.

On Jun 13 22:29, Linda Walsh wrote:
> I think I've run into a problem involving fread.
> I open a test file (fopen) with Write-Only access ("w"),
> immediately after that, I do an fread on the file handle
> of some number of bytes. 
> 
> I expect fread to return an error.  But it doesn't. 
> It returns 0 as the number of bytes read, and the
> error value remains unset when querying with ferror.
> 
> In addition to fread not setting the error value, a value
> of zero is returned.  Zero is to be returned, *only* on
> end-of-file or error.  However, in the test case, neither

That's not correct.  Any value less than size*nitems indicates either
EOF or an error.  The programmer is responsible to test with feof() or
ferror() to distinguish between these two cases.  See

http://www.opengroup.org/onlinepubs/009695399/functions/fread.html

> value is returned.  Theoretically, this shouldn't happen
> (i.e. it's an undefined "set" of return values).
> 
> test program follows:
> -------------------
> #include <stdio.h>
> main (int argc, char * argv[]) {
> 
>    FILE * output_handle=fopen("/tmp/tmpfile","w");
>    if (!output_handle) {
>        printf ("can't open output file /tmp/tmpfile\n");
>        exit (1);
>    }  
> 
>    char mybuff[4];
>    int retval=fread(mybuff, sizeof(char), 4, output_handle);
>    if (!retval) {
>        int eof=feof(output_handle);
>        int err=ferror(output_handle);
>        printf ("(retval,eof,err) = (%d,%d,%d)\n",retval,eof,err);
>        if (!eof && ! err) 
>            printf ("Undefined error: fread returned zero, but there is 
> no eof or error\n");
>        exit(2);
>    } else
>        printf ("Unexpected success in fread: Read %d chars\n",retval);
>   
>    exit(0);
> }

I debugged your testcase and the problem appears to be in __srefill(),
defined in newlib/libc/stdio/refill.c:

  /* if not already reading, have to be reading and writing */
  if ((fp->_flags & __SRD) == 0)
    {
      if ((fp->_flags & __SRW) == 0)
        return EOF;

So, what happens is that EOF is returned if the file is not readable.
Errno isn't set and the error condition on the file pointer isn't set
either.

Testing the same situation on Linux, errno is set to EBADF and the
error indicator is set on the file pointer, while the EOF condition
stays clear.

So, I'd like to propose the below patch.  I assume a similar patch
should be ok for __sfvwrite, too, isn't it?  There's a call to
cantwrite() which only returns EOF but which probably should also
set the error condition and errno.


	* libc/stdio/refill.c (__srefill): Return error condition when
	trying to read a file not opened for reading.

Index: libc/stdio/refill.c
===================================================================
RCS file: /cvs/src/src/newlib/libc/stdio/refill.c,v
retrieving revision 1.6
diff -u -p -r1.6 refill.c
--- libc/stdio/refill.c	8 Feb 2005 01:33:17 -0000	1.6
+++ libc/stdio/refill.c	14 Jun 2006 08:19:28 -0000
@@ -19,6 +19,7 @@
 #include <_ansi.h>
 #include <stdio.h>
 #include <stdlib.h>
+#include <sys/errno.h>
 #include "local.h"
 
 static int
@@ -55,7 +56,11 @@ _DEFUN(__srefill, (fp),
   if ((fp->_flags & __SRD) == 0)
     {
       if ((fp->_flags & __SRW) == 0)
-	return EOF;
+	{
+	  _REENT->_errno = EBADF;
+	  fp->_flags |= __SERR;
+	  return EOF;
+	}
       /* switch to reading */
       if (fp->_flags & __SWR)
 	{


Corinna

-- 
Corinna Vinschen                  Please, send mails regarding Cygwin to
Cygwin Project Co-Leader          cygwin AT cygwin DOT com
Red Hat

--
Unsubscribe info:      http://cygwin.com/ml/#unsubscribe-simple
Problem reports:       http://cygwin.com/problems.html
Documentation:         http://cygwin.com/docs.html
FAQ:                   http://cygwin.com/faq/