bug in fwrite/__towrite

bug in fwrite/__towrite

[hombre]

Hello,

I think there is a bug in fwrite/__towrite.

This is my unittest that fails:
static void test_write_read2(const char *fname)
{
     char wbuf[3];
     char c;

     FILE *file = fopen(fname, "wb");
     assert(file != NULL);
     wbuf[0] = 'a';
     wbuf[1] = 'b';
     wbuf[2] = 'c';
     size_t written = fwrite(wbuf, 1, 3, file);
     assert(written == 3);
     fclose(file);

     file = fopen(fname, "rb+");
     size_t nread = fread(&c, 1, 1, file);
     assert(nread == 1);
     assert(c == 'a');
     c = 'B';
     written = fwrite(&c, 1, 1, file);
     assert(written == 1);
     nread = fread(&c, 1, 1, file);
     assert(nread == 1); /* <================== nread is 0 here ! */
     assert(c == 'c');
     fclose(file);
}

Please note that I have not tested this with the original musl-libc in 
linux. I found this bug while I was trying to port parts of musl to a 
small embedded os. But I think the bug is not in my port. Here is what I 
think is wrong:

- fwrite calls __towrite when the write buffer is not active
     if (!f->wend && __towrite(f)) return 0;
- __towrite clears the read buffer, but the underlying filepointer is 
not adjusted. I think that the filepointer should be adjusted, when the 
read buffer is not empty.
int __towrite(FILE *f)
{
     ...
     /* Clear read buffer (easier than summoning nasal demons) */
     f->rpos = f->rend = 0;
     ...

Here is my fix:
int __towrite(FILE *f)
{
     ...
     /* Clear read buffer (easier than summoning nasal demons) */
     if (f->rpos) {
         /* Adjust underlying filepointer for unread data in buffer. */
         if (f->seek(f, -(f->rend - f->rpos), SEEK_CUR) < 0)
             return -1;
         f->rpos = f->rend = 0;
     }
     ...

Regards,
Erwin

Re: bug in fwrite/__towrite

[Rich Felker]

On Fri, Feb 05, 2016 at 04:32:58PM +0100, hombre wrote:
> Hello,
> 
> I think there is a bug in fwrite/__towrite.
> 
> This is my unittest that fails:
> static void test_write_read2(const char *fname)
> {
>     char wbuf[3];
>     char c;
> 
>     FILE *file = fopen(fname, "wb");
>     assert(file != NULL);
>     wbuf[0] = 'a';
>     wbuf[1] = 'b';
>     wbuf[2] = 'c';
>     size_t written = fwrite(wbuf, 1, 3, file);
>     assert(written == 3);
>     fclose(file);
> 
>     file = fopen(fname, "rb+");
>     size_t nread = fread(&c, 1, 1, file);
>     assert(nread == 1);
>     assert(c == 'a');
>     c = 'B';
>     written = fwrite(&c, 1, 1, file);

This line caused undefined behavior. You cannot switch between writing
and reading on a stream without an intervening successful fseek or
fflush (the latter only in the write->read direction). See 7.21.5.3
The fopen function, paragraph 7:

"When a file is opened with update mode ('+' as the second or third
character in the above list of mode argument values), both input and
output may be performed on the associated stream. However, output
shall not be directly followed by input without an intervening call to
the fflush function or to a file positioning function (fseek, fsetpos,
or rewind), and input shall not be directly followed by output without
an intervening call to a file positioning function, unless the input
operation encounters end- of-file. Opening (or creating) a text file
with update mode may instead open (or create) a binary stream in some
implementations."

>     assert(written == 1);
>     nread = fread(&c, 1, 1, file);

And again.

>     assert(nread == 1); /* <================== nread is 0 here ! */
>     assert(c == 'c');
>     fclose(file);
> }

Rich