Possible file stream bug

Possible file stream bug

[Paul Schutte]

[-- Attachment #1: Type: text/plain, Size: 529 bytes --]

Hi

I compiled and linked libwebserver-0.5.3 against musl.

It would just strangely break halfway through a request. After hours of
searching, I found the problem.

I can demonstrate it with the following code:

#include <unistd.h>
#include <stdio.h>

int main() {
    FILE *fstream;

    fclose(stdout);

    fstream=freopen("/dev/tty","w",stdout);

    if (fstream==NULL) {
        fprintf(stderr,"freopen failed\n");
    }

    printf("test this\n");


    return 0;
}

This snippet works fine when using glibc.

Regards
Paul

[-- Attachment #2: Type: text/html, Size: 695 bytes --]

Re: Possible file stream bug

[Paul Schutte]

[-- Attachment #1: Type: text/plain, Size: 1087 bytes --]

Hi

While investigating this, I stumbled upon another bug:

#include <unistd.h>
#include <stdio.h>

int main() {
        FILE *fstream;
        int len;

        fclose(stdout);

        len=printf("test this\n");

        fprintf(stderr,"%d\n",len);

        return 0;
}

Musl's printf gives the amount of bytes no matter whether it succeeds or
fails.
It should return a negative value in case of failure.

Regards
Paul

On Wed, Oct 24, 2012 at 9:36 PM, Paul Schutte <sjpschutte@gmail.com> wrote:

> Hi
>
> I compiled and linked libwebserver-0.5.3 against musl.
>
> It would just strangely break halfway through a request. After hours of
> searching, I found the problem.
>
> I can demonstrate it with the following code:
>
> #include <unistd.h>
> #include <stdio.h>
>
> int main() {
>     FILE *fstream;
>
>     fclose(stdout);
>
>     fstream=freopen("/dev/tty","w",stdout);
>
>     if (fstream==NULL) {
>         fprintf(stderr,"freopen failed\n");
>     }
>
>     printf("test this\n");
>
>
>     return 0;
> }
>
> This snippet works fine when using glibc.
>
> Regards
> Paul
>
>
>

[-- Attachment #2: Type: text/html, Size: 1618 bytes --]

Re: Re: Possible file stream bug

[Rich Felker]

On Wed, Oct 24, 2012 at 10:16:01PM +0200, Paul Schutte wrote:
> Hi
> 
> While investigating this, I stumbled upon another bug:
> 
> #include <unistd.h>
> #include <stdio.h>
> 
> int main() {
>         FILE *fstream;
>         int len;
> 
>         fclose(stdout);
> 
>         len=printf("test this\n");
> 
>         fprintf(stderr,"%d\n",len);
> 
>         return 0;
> }
> 
> Musl's printf gives the amount of bytes no matter whether it succeeds or
> fails.
> It should return a negative value in case of failure.

You invoked undefined behavior by accessing a FILE object after
calling fclose on it. Thus anything can happen. Even if you just
closed the underlying file descriptor #1 without closing the FILE,
printf would be under no obligation to return -1 since the write error
need not be detected until the buffer gets flushed.

Rich

Re: Possible file stream bug

[Rich Felker]

On Wed, Oct 24, 2012 at 09:36:28PM +0200, Paul Schutte wrote:
> Hi
> 
> I compiled and linked libwebserver-0.5.3 against musl.
> 
> It would just strangely break halfway through a request. After hours of
> searching, I found the problem.
> 
> I can demonstrate it with the following code:
> 
> #include <unistd.h>
> #include <stdio.h>
> 
> int main() {
>     FILE *fstream;
> 
>     fclose(stdout);
> 
>     fstream=freopen("/dev/tty","w",stdout);
> 
>     if (fstream==NULL) {
>         fprintf(stderr,"freopen failed\n");
>     }
> 
>     printf("test this\n");
> 
> 
>     return 0;
> }
> 
> This snippet works fine when using glibc.

This code is invalid; you have invoked undefined behavior by accessing
stdout (passing it to freopen) after it was closed with fclose. Remove
the fclose and it works correctly. This code would certainly cause
memory corruption and/or crash if it were used on any FILE other than
one of the 3 builtin ones (since the memory would have been returned
to the heap when fclose was called) so there is no sense in trying to
support this invalid usage. You should file a bug report with the
application.

Rich

Re: Possible file stream bug

[Paul Schutte]

[-- Attachment #1: Type: text/plain, Size: 2491 bytes --]

Hi,

It is not my code, but I can not see why it is invalid.

Here is a strace when linked agains glibc:

close(1)                                = 0
open("/dev/tty", O_WRONLY|O_CREAT|O_TRUNC, 0666) = 1
fstat(1, {st_mode=S_IFCHR|0666, st_rdev=makedev(5, 0), ...}) = 0
ioctl(1, SNDCTL_TMR_TIMEBASE or TCGETS, {B38400 opost isig icanon echo
...}) = 0
mmap(NULL, 4096, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0) =
0x7f6eb782e000
write(1, "test this\n", 10)             = 10
exit_group(0)                           = ?

Here is an strace aganst musl:

close(1)                                = 0
open("/dev/tty", O_WRONLY|O_CREAT|O_TRUNC, 0666) = 1
ioctl(1, SNDCTL_TMR_TIMEBASE or TCGETS, {B38400 opost isig icanon echo
...}) = 0
dup2(1, 1)                              = 1
close(1)                                = 0
writev(1, [{"test this", 9}, {"\n", 1}], 2) = -1 EBADF (Bad file descriptor)
exit_group(0)                           = ?

Musl is doing a dup2 and then closes the old descriptor. In this case, it
is the same and therefore the stream is closed.
One could possibly test for the case where newfd==oldfd and not close oldfd
in that case. Or am I missing something ?

Regards
Paul

On Wed, Oct 24, 2012 at 10:59 PM, Rich Felker <dalias@aerifal.cx> wrote:

> On Wed, Oct 24, 2012 at 09:36:28PM +0200, Paul Schutte wrote:
> > Hi
> >
> > I compiled and linked libwebserver-0.5.3 against musl.
> >
> > It would just strangely break halfway through a request. After hours of
> > searching, I found the problem.
> >
> > I can demonstrate it with the following code:
> >
> > #include <unistd.h>
> > #include <stdio.h>
> >
> > int main() {
> >     FILE *fstream;
> >
> >     fclose(stdout);
> >
> >     fstream=freopen("/dev/tty","w",stdout);
> >
> >     if (fstream==NULL) {
> >         fprintf(stderr,"freopen failed\n");
> >     }
> >
> >     printf("test this\n");
> >
> >
> >     return 0;
> > }
> >
> > This snippet works fine when using glibc.
>
> This code is invalid; you have invoked undefined behavior by accessing
> stdout (passing it to freopen) after it was closed with fclose. Remove
> the fclose and it works correctly. This code would certainly cause
> memory corruption and/or crash if it were used on any FILE other than
> one of the 3 builtin ones (since the memory would have been returned
> to the heap when fclose was called) so there is no sense in trying to
> support this invalid usage. You should file a bug report with the
> application.
>
> Rich
>

[-- Attachment #2: Type: text/html, Size: 3206 bytes --]

Re: Possible file stream bug

[Rich Felker]

On Wed, Oct 24, 2012 at 11:22:13PM +0200, Paul Schutte wrote:
> Hi,
> 
> It is not my code, but I can not see why it is invalid.

    C99 7.19.3 Files

    ¶4. A file may be disassociated from a controlling stream by
    closing the file. Output streams are flushed (any unwritten buffer
    contents are transmitted to the host environment) before the
    stream is disassociated from the file. The value of a pointer to a
    FILE object is indeterminate after the associated file is closed
    (including the standard text streams). Whether a file of zero
    length (on which no characters have been written by an output
    stream) actually exists is implementation-defined.

Since the value of the pointer-to-FILE is indeterminate after fclose,
any use of it results in undefined behavior.

The error you've encountered is analogous to the error of calling
free() on memory obtained by malloc(), then later calling realloc() on
the already-freed pointer to try to get it back. This is not the
purpose of realloc (or freopen), and fundamentally cannot work in
general, since the pointed-to memory could already have been reclaimed
for another use. In both cases (realloc and reopen), the function must
be used on an object that is still valid, not one which has already
been closed/freed.

The fact that it happens to work on glibc is purely luck (good or bad
luck, depending on your perspective). This is the nature of undefined
behavior - it can lead to your program doing what you wanted it to,
even though the program is wrong.

Rich

Re: Possible file stream bug

[Paul Schutte]

[-- Attachment #1: Type: text/plain, Size: 1903 bytes --]

Thanks Rich !

I was thinking in terms of file descriptors which is only integers and not
whole data structures.

Sorry for wasting your time.

BTW
Musl is really awesome !


On Wed, Oct 24, 2012 at 11:25 PM, Rich Felker <dalias@aerifal.cx> wrote:

> On Wed, Oct 24, 2012 at 11:22:13PM +0200, Paul Schutte wrote:
> > Hi,
> >
> > It is not my code, but I can not see why it is invalid.
>
>     C99 7.19.3 Files
>
>     ¶4. A file may be disassociated from a controlling stream by
>     closing the file. Output streams are flushed (any unwritten buffer
>     contents are transmitted to the host environment) before the
>     stream is disassociated from the file. The value of a pointer to a
>     FILE object is indeterminate after the associated file is closed
>     (including the standard text streams). Whether a file of zero
>     length (on which no characters have been written by an output
>     stream) actually exists is implementation-defined.
>
> Since the value of the pointer-to-FILE is indeterminate after fclose,
> any use of it results in undefined behavior.
>
> The error you've encountered is analogous to the error of calling
> free() on memory obtained by malloc(), then later calling realloc() on
> the already-freed pointer to try to get it back. This is not the
> purpose of realloc (or freopen), and fundamentally cannot work in
> general, since the pointed-to memory could already have been reclaimed
> for another use. In both cases (realloc and reopen), the function must
> be used on an object that is still valid, not one which has already
> been closed/freed.
>
> The fact that it happens to work on glibc is purely luck (good or bad
> luck, depending on your perspective). This is the nature of undefined
> behavior - it can lead to your program doing what you wanted it to,
> even though the program is wrong.
>
> Rich
>

[-- Attachment #2: Type: text/html, Size: 2303 bytes --]

Re: Possible file stream bug

[Rich Felker]

On Wed, Oct 24, 2012 at 11:54:17PM +0200, Paul Schutte wrote:
> Thanks Rich !
> 
> I was thinking in terms of file descriptors which is only integers and not
> whole data structures.
> 
> Sorry for wasting your time.

No problem, it wasn't a waste of time -- and this may actually be a
bug in cases where just the file descriptor and not the FILE was
closed. I'm looking into it.

Rich

Re: Possible file stream bug

[Rich Felker]

On Wed, Oct 24, 2012 at 11:54:17PM +0200, Paul Schutte wrote:
> Thanks Rich !
> 
> I was thinking in terms of file descriptors which is only integers and not
> whole data structures.
> 
> Sorry for wasting your time.

I committed some changes that weren't strictly needed, but which make
the behavior "better" in programs which have called close(fileno(f))
prior to freopen(f, ...). These changes should also have the effect
that the code in your example "works", but since it's using a FILE
stream after calling fclose on it, relying on it continuing to work is
not something I would recommend. It would be better to fix the buggy
code.

Rich

Re: Possible file stream bug

[Szabolcs Nagy]

* Paul Schutte <sjpschutte@gmail.com> [2012-10-24 23:22:13 +0200]:
> It is not my code, but I can not see why it is invalid.

beacuse the standard says so

http://port70.net/~nsz/c/c99/n1256.html#7.19.3p4

and in annex j.2 in the undefined behaviour list there is:
"- The value of a pointer to a FILE object is used after the associated file is closed"


also note that the freopen spec says that it closes the underlying
file so there is no reason for a separate fclose (or fflush) anyway

http://port70.net/~nsz/c/c99/n1256.html#7.19.5.4p4

> Here is a strace when linked agains glibc:
> 
> close(1)                                = 0
> open("/dev/tty", O_WRONLY|O_CREAT|O_TRUNC, 0666) = 1
> fstat(1, {st_mode=S_IFCHR|0666, st_rdev=makedev(5, 0), ...}) = 0
> ioctl(1, SNDCTL_TMR_TIMEBASE or TCGETS, {B38400 opost isig icanon echo
> ...}) = 0

glibc clearly does not do close in freopen

so it detects closed file streams and handles it in some way
masking the error in the program

Re: Possible file stream bug

[Rich Felker]

On Wed, Oct 24, 2012 at 11:41:43PM +0200, Szabolcs Nagy wrote:
> * Paul Schutte <sjpschutte@gmail.com> [2012-10-24 23:22:13 +0200]:
> > It is not my code, but I can not see why it is invalid.
> 
> beacuse the standard says so
> 
> http://port70.net/~nsz/c/c99/n1256.html#7.19.3p4
> 
> and in annex j.2 in the undefined behaviour list there is:
> "- The value of a pointer to a FILE object is used after the associated file is closed"
> 
> 
> also note that the freopen spec says that it closes the underlying
> file so there is no reason for a separate fclose (or fflush) anyway
> 
> http://port70.net/~nsz/c/c99/n1256.html#7.19.5.4p4
> 
> > Here is a strace when linked agains glibc:
> > 
> > close(1)                                = 0
> > open("/dev/tty", O_WRONLY|O_CREAT|O_TRUNC, 0666) = 1
> > fstat(1, {st_mode=S_IFCHR|0666, st_rdev=makedev(5, 0), ...}) = 0
> > ioctl(1, SNDCTL_TMR_TIMEBASE or TCGETS, {B38400 opost isig icanon echo
> > ...}) = 0
> 
> glibc clearly does not do close in freopen
> 
> so it detects closed file streams and handles it in some way
> masking the error in the program

The issue is that musl does a dup2 followed by close to ensure that
freopen keeps the original file descriptor in a thread-safe way, and
it assumes the two file descriptor numbers passed to dup2 will not be
the same. This could also bite us in the case where fclose(f) had not
been called, but close(fileno(fd)) had been; such usage definitely
isn't valid in multi-threaded programs, and it's questionable to begin
with, but it does have some semi-legitimate uses in single-threaded
programs and might be worth supporting.

In any case, fixing that will not make the usage in libwebserver as
_supported_ usage (i.e. it may be subject to breakage at any time,
since it's invoking UB), but it might happen to make it work for now.

Rich