Bug 2074

Provide an example of a user of libc (not the internal functions) when this is a
problem.

I don't have a specific test case any more. This would intermittently cause
problems when running 'rpm' to install a package on a file system 
that was full. If the package being installed was just the right 
size, rpm would seg fault.

The following change in fileops.c fixes the bug. 

glibc/libio/fileops.c:

***************
*** 1371,1376 ****
--- 1371,1379 ----
        if (do_write)
          {
            count = new_do_write (f, s, do_write);
+           if (count == EOF) {
+               count = 0;
+           }
            to_do -= count;
            if (count < do_write)
              return n - to_do;

In which path does new_do_write return EOF?  It shouldn't.  Therefore the patch
isn't needed.  I'll close the bug unless you can show some data.

(In reply to comment #0)
> In glibc 2.3.4, the function new_do_write() in libio/fileops.c returns an 
> unsigned
> value (an _IO_size_t). If the underlying write system call in new_do_write()
> returns EOF (-1), new_do_write() will return 0xffffffff rather than -1.
> The function IO_new_file_xsputn() in libio/fileops.c appears to have
> 2 problems if the call to new_do_write() returns 0xffffffff. Here's
> the specific snippet of code from IO_new_file_xsputn():
>       if (do_write)
>         {
>           count = new_do_write (f, s, do_write);
>           to_do -= count;
>           if (count < do_write)
>             return n - to_do;
>         }
>       /* Now write out the remainder.  Normally, this will fit in the
> 	 buffer, but it's somewhat messier for line-buffered files,
> 	 so we let _IO_default_xsputn handle the general case. */
>       if (to_do)
> 	to_do -= INTUSE(_IO_default_xsputn) (f, s+do_write, to_do);
>     }
>   return n - to_do;
> If new_do_write() returns 0xffffffff, 'to_do' will actually
> be incremented by 1. Also, since 'count' is an unsigned quantity,
> the if statement 'if (count < do_write)' will evaluate to false,
> and the code will incorrectly fall through to the call to
> IO_default_xsputn().

(In reply to comment #3)

_IO_new_file_xsputn() calls new_do_write() which in turn calls _IO_SYSWRITE(). 
The call to _IO_SYSWRITE() may return -1 (EOF). When this happens, new_do_write
() returns this value as the 'count'.

I've highlighted this in the following piece of code in fileops.c.

static
int
new_do_write (fp, data, to_do)
     _IO_FILE *fp;
     const char *data;
     _IO_size_t to_do;
{
  _IO_size_t count;
  if (fp->_flags & _IO_IS_APPENDING)
    /* On a system without a proper O_APPEND implementation,
       you would need to sys_seek(0, SEEK_END) here, but is
       is not needed nor desirable for Unix- or Posix-like systems.
       Instead, just indicate that offset (before and after) is
       unpredictable. */
    fp->_offset = _IO_pos_BAD;
  else if (fp->_IO_read_end != fp->_IO_write_base)
    {
      _IO_off64_t new_pos
	= _IO_SYSSEEK (fp, fp->_IO_write_base - fp->_IO_read_end, 1);
      if (new_pos == _IO_pos_BAD)
	return 0;
      fp->_offset = new_pos;
    }

********** THIS CALL MAY RETURN -1 ***************
  count = _IO_SYSWRITE (fp, data, to_do);
*************************************************

  if (fp->_cur_column && count)
    fp->_cur_column = INTUSE(_IO_adjust_column) (fp->_cur_column - 1, data,
						 count) + 1;
  _IO_setg (fp, fp->_IO_buf_base, fp->_IO_buf_base, fp->_IO_buf_base);
  fp->_IO_write_base = fp->_IO_write_ptr = fp->_IO_buf_base;
  fp->_IO_write_end = (fp->_mode <= 0
		       && (fp->_flags & (_IO_LINE_BUF+_IO_UNBUFFERED))
		       ? fp->_IO_buf_base : fp->_IO_buf_end);
  return count;
}

Which function called though _IO_SYSWRITE returns -1?  None should, that's the
whole point.

Subject: RE:  _IO_new_file_xsputn() in fileops.c not checking for EOF

It's not very clear from the glibc code, but, isn't _IO_SYSWRITE
a wrapper for the system call to write?

Are you implying that the write system call should never fail? 
What do you think will happen when the file system is full?

- John Fardo

> -----Original Message-----
> From: drepper at redhat dot com 
> [mailto:sourceware-bugzilla@sourceware.org] 
> Sent: Monday, August 28, 2006 11:25 AM
> To: John Fardo
> Subject: [Bug libc/2074] _IO_new_file_xsputn() in fileops.c 
> not checking for EOF
> 
> 
> ------- Additional Comments From drepper at redhat dot com  
> 2006-08-28 15:24 ------- Which function called though 
> _IO_SYSWRITE returns -1?  None should, that's the whole point.
> 
> -- 
> 
> 
> http://sourceware.org/bugzilla/show_bug.cgi?id=2074
> 
> ------- You are receiving this mail because: ------- You 
> reported the bug, or are watching the reporter.
> 
> 

So you are just guessing what is it?  Can't you instead grep the code?
find libc -name \*.[ch] | xargs grep 'JUMP_INIT[[:blank:]]*(write'
shows the functions and none of them return EOF.

Subject: RE:  _IO_new_file_xsputn() in fileops.c not checking for EOF

_IO_cookie_write() in iofopncook.c looks like it can return -1.

Also, this problem was observed when running rpm -
rpm uses fopencookie().

Code from iofopncook.c:
static _IO_ssize_t
_IO_cookie_write (fp, buf, size)
     _IO_FILE *fp;
     const void *buf;
     _IO_ssize_t size;
{
  struct _IO_cookie_file *cfile = (struct _IO_cookie_file *) fp;

  if (cfile->__io_functions.write == NULL)
    return -1;

  return cfile->__io_functions.write (cfile->__cookie, buf, size);
}

- John Fardo 

> -----Original Message-----
> From: jakub at redhat dot com 
> [mailto:sourceware-bugzilla@sourceware.org] 
> Sent: Tuesday, August 29, 2006 11:34 AM
> To: John Fardo
> Subject: [Bug libc/2074] _IO_new_file_xsputn() in fileops.c 
> not checking for EOF
> 
> 
> ------- Additional Comments From jakub at redhat dot com  
> 2006-08-29 15:34 ------- So you are just guessing what is it? 
>  Can't you instead grep the code?
> find libc -name \*.[ch] | xargs grep 'JUMP_INIT[[:blank:]]*(write'
> shows the functions and none of them return EOF.
> 
> -- 
> 
> 
> http://sourceware.org/bugzilla/show_bug.cgi?id=2074
> 
> ------- You are receiving this mail because: ------- You 
> reported the bug, or are watching the reporter.
> 
> 

> _IO_cookie_write() in iofopncook.c looks like it can return -1.

It returns what the user provided function returns.  If the latter returns -1 it
is buggy and must be fixed.  This is not glibc's problem.

Then this is a documentation bug - 12.21.3.2 Custom Stream Hook Functions says:

   You should define the function to write data to the cookie as:

     ssize_t WRITER (void *COOKIE, const char *BUFFER, size_t SIZE)

   This is very similar to the `write' function; see *note I/O
Primitives::.  Your function should transfer up to SIZE bytes from the
buffer, and return the number of bytes written.  You can return a value
of `-1' to indicate an error.

Created attachment 4777 [details]
proposed patch

Ok to commit?

The patch to the manual and also a corresponding change to libio.h has been put into git now.

I also filed Bug 13975 - fopencookie interface surprising.

(In reply to comment #11)
> Created attachment 4777 [details]
> proposed patch
> 
> Ok to commit?

I've committed a corresponding change to the fopencookie(3) man page.

@@ -158,7 +158,8 @@ As its function result, the
 .I write
 function should return the number of bytes copied from
 .IR buf ,
-or \-1 on error.
+or 0 on error.
+(The function must not return a negative value.)
 The
 .I write
 function should update the stream offset appropriately.