The freopen() function (1) open the new file, (2) close the old file (3) dup2() the newly opened file to the file descriptor (wantfd) previously held by the old file. (As comments says, "Various C library routines assume stderr is always fd STDERR_FILENO"). So, between step (2) and step (3) the wantfd file descriptor is closed, and if another thread open a file it make take the wantfd descriptor. Then, after the dup2() call in step (3) above, a mess will happen. Fix: Don't close the old file descriptor, the dup2() call will do the job.
Indeed, this a bug, but the patch you provided breaks the samentic the FILE structure was designed for, here you conditionally call fp->_close(), this is incorrect, because the hook may be an external function, it should always be called to notify external code. I think the right fix is to fix those code which is still using STDERR_FILENO, or don't do following hack in freopen.c: if (wantfd >= 0 && f != wantfd) { if (_dup2(f, wantfd) >= 0) { (void)_close(f); f = wantfd; } } This is not required by standard. David Xu
David Xu wrote: > Indeed, this a bug, but the patch you provided breaks the samentic the > FILE structure was designed for, here you conditionally call > fp->_close(), this is incorrect, because the hook may be an external > function, it should always be called to notify external code. I only assume that 1) _file and _close fields are internal to stdio, i.e. only stdio code manipulate with them directly 2) If _file != -1, then the FILE is associated with the file descriptor, fp->_close == __sclose (because the only code that can set fp_close to something different is funopen, and it set _file to -1) and __sclose just close the _fp->_file If so, we know that dup2() will close the descriptor too, dup2() is required to do it. > I think the right fix is to fix those code which is still using > STDERR_FILENO, or don't do following hack in freopen.c: > if (wantfd >= 0 && f != wantfd) { > if (_dup2(f, wantfd) >= 0) { > (void)_close(f); > f = wantfd; > } > } > This is not required by standard. Well, I tried to keep existing behaviour, and I think that the hack is indeed a good idea even though it is not required. > > > David Xu >
Dmitrij Tejblum wrote: > > David Xu wrote: > >> Indeed, this a bug, but the patch you provided breaks the samentic the >> FILE structure was designed for, here you conditionally call >> fp->_close(), this is incorrect, because the hook may be an external >> function, it should always be called to notify external code. > > > I only assume that > 1) _file and _close fields are internal to stdio, i.e. only stdio code > manipulate with them directly > 2) If _file != -1, then the FILE is associated with the file descriptor, > fp->_close == __sclose (because the only code that can set fp_close to > something different is funopen, and it set _file to -1) and __sclose > just close the _fp->_file > If so, we know that dup2() will close the descriptor too, dup2() is > required to do it. > I think we allow _close and others to be changed by user code unless someone can clarify that this is not allowed now, otherwise your assumption is false. >> I think the right fix is to fix those code which is still using >> STDERR_FILENO, or don't do following hack in freopen.c: >> if (wantfd >= 0 && f != wantfd) { >> if (_dup2(f, wantfd) >= 0) { >> (void)_close(f); >> f = wantfd; >> } >> } >> This is not required by standard. > > > Well, I tried to keep existing behaviour, and I think that the hack is > indeed a good idea even though it is not required.
David Xu wrote: > Dmitrij Tejblum wrote: > >> >> David Xu wrote: >> >>> Indeed, this a bug, but the patch you provided breaks the samentic the >>> FILE structure was designed for, here you conditionally call >>> fp->_close(), this is incorrect, because the hook may be an external >>> function, it should always be called to notify external code. >> >> >> >> I only assume that >> 1) _file and _close fields are internal to stdio, i.e. only stdio >> code manipulate with them directly >> 2) If _file != -1, then the FILE is associated with the file >> descriptor, fp->_close == __sclose (because the only code that can >> set fp_close to something different is funopen, and it set _file to >> -1) and __sclose just close the _fp->_file >> If so, we know that dup2() will close the descriptor too, dup2() is >> required to do it. >> > I think we allow _close and others to be changed by user code unless > someone can clarify that this is not allowed now, otherwise your > assumption is false. Well, this is C, not C++, so there cannot be strict difference between allowed and disallowed. But the _close field is not required by any standarts (funopen() too) and is not documented by manpages (I checked manpages with grep). And there does exist documented interface for setting _close: funopen(). Thus _close is internal.
Dmitrij Tejblum wrote: > > Well, this is C, not C++, so there cannot be strict difference between > allowed and disallowed. But the _close field is not required by any > standarts (funopen() too) and is not documented by manpages (I checked > manpages with grep). And there does exist documented interface for > setting _close: funopen(). Thus _close is internal. > > > Then why use the following code to close a file descriptor, if (isopen) (void) (*fp->_close)(fp->_cookie); Why don't use _close(fp->_file)? this should be faster. This implies that someone can replace fp->_close with another function pointer,so that function must be called unconditionally. I think the libc should have a document to clarify this problem. Also should we move the PR to freebsd-standard@ list ?
Hi, This bug is biting MySQL badly (data corruption due to writing to the wrong file). See http://bugs.mysql.com/51023 Bug#51023 Mysql server crashes on SIGHUP and destroys InnoDB files Attaching a simple prog to reproduce this (tested on 8.1-STABLE/amd64). -- Vasil Dimov gro.DSBeerF@dv % The difference between life and the movies is that a script has to make sense, and life doesn't. -- Joseph L. Mankiewicz
Attached is a prog which demonstrates what happens when another thread calls open() when freopen() is executing. The output is: open(): /tmp/f1: 3 trx1: freopen() begins, will associate fd=3 with /tmp/f2 trx1: freopen(): open(): /tmp/f2: 4 trx1: freopen(): close(): 3: 0 trx2: open(): /tmp/f_other: 3 trx1: freopen(): dup2(): 4,3: 3 trx1: freopen(): end, now fd=3 is associated with /tmp/f2 trx1: write to fd=3 (supposed to be /tmp/f2) trx2: write to fd=3 (supposed to be /tmp/f_other) At the end /tmp/f2 contains "ABCXYZ", /tmp/f_other is empty. -- Vasil Dimov gro.DSBeerF@dv % The difference between life and the movies is that a script has to make sense, and life doesn't. -- Joseph L. Mankiewicz
David, I think the submitter's analysis is correct that the only place that can set the close function pointer is funopen() and that for that case (and any other "fake" files), the file descriptor will be -1. If the fd is >= 0, then it must be a file-descriptor-backed FILE, and relying on dup2() to close the fd is ok. As the manpage notes, the most common usage is to redirect stderr or stdout by doing 'freopen("/dev/null", "w", stderr)'. The bug allows some other random code that is calling open() in another thread to have that open() return 2 during the window where fd '2' is closed during freopen(). That other file descriptor then gets trounced by the dup2() call in freopen() to point to something else. The code likely uses _close() rather than close() directly to be cleaner. Given that this is stdio, I don't think we are really worried about the performance impact of one extra wrapper function. I think the original patch is most likely correct. -- John Baldwin
John Baldwin wrote: > David, > > I think the submitter's analysis is correct that the only place that can set > the close function pointer is funopen() and that for that case (and any other > "fake" files), the file descriptor will be -1. If the fd is >= 0, then it > must be a file-descriptor-backed FILE, and relying on dup2() to close the fd > is ok. > > As the manpage notes, the most common usage is to redirect stderr or stdout by > doing 'freopen("/dev/null", "w", stderr)'. The bug allows some other random > code that is calling open() in another thread to have that open() return 2 > during the window where fd '2' is closed during freopen(). That other file > descriptor then gets trounced by the dup2() call in freopen() to point to > something else. > > The code likely uses _close() rather than close() directly to be cleaner. > Given that this is stdio, I don't think we are really worried about the > performance impact of one extra wrapper function. > > I think the original patch is most likely correct. > The patch works, I just don't like the design of the (*fp->_close)(fp->_cookie) it seems the patch make freopen bypass it. I think the patch can be committed, but I am busy and have no time to do it by myself.
On Tuesday, December 07, 2010 9:43:35 pm David Xu wrote: > John Baldwin wrote: > > David, > > > > I think the submitter's analysis is correct that the only place that can set > > the close function pointer is funopen() and that for that case (and any other > > "fake" files), the file descriptor will be -1. If the fd is >= 0, then it > > must be a file-descriptor-backed FILE, and relying on dup2() to close the fd > > is ok. > > > > As the manpage notes, the most common usage is to redirect stderr or stdout by > > doing 'freopen("/dev/null", "w", stderr)'. The bug allows some other random > > code that is calling open() in another thread to have that open() return 2 > > during the window where fd '2' is closed during freopen(). That other file > > descriptor then gets trounced by the dup2() call in freopen() to point to > > something else. > > > > The code likely uses _close() rather than close() directly to be cleaner. > > Given that this is stdio, I don't think we are really worried about the > > performance impact of one extra wrapper function. > > > > I think the original patch is most likely correct. > > > > The patch works, I just don't like the design of the > (*fp->_close)(fp->_cookie) > it seems the patch make freopen bypass it. > I think the patch can be committed, but I am busy and have > no time to do it by myself. Actually, the freopen() code honors custom _close() routines earlier when it checks for _file being < 0. I do really think this is ok. _close() is not public, it is only allowed to be set via funopen(). We also need the dup2() change to effectively implement this function's rationale, which is a way to redirect stdin, stdout, and stderr. I will take care of committing this today, with an extra bit of comment. -- John Baldwin
Author: jhb Date: Thu Dec 9 20:28:30 2010 New Revision: 216334 URL: http://svn.freebsd.org/changeset/base/216334 Log: When reopening a stream backed by an open file descriptor, do not close the existing file descriptor. Instead, let dup2() atomically close the old file descriptor when assigning the newly opened file to the same descriptor. This closes a race in a multithreaded application where a concurrent open() could allocate the existing file descriptor in between the calls to close() and dup2(). PR: threads/79887 Submitted by: Dmitrij Tejblum tejblum of yandex-team.ru Reviewed by: davidxu MFC after: 1 week Modified: head/lib/libc/stdio/freopen.c Modified: head/lib/libc/stdio/freopen.c ============================================================================== --- head/lib/libc/stdio/freopen.c Thu Dec 9 20:16:00 2010 (r216333) +++ head/lib/libc/stdio/freopen.c Thu Dec 9 20:28:30 2010 (r216334) @@ -150,14 +150,6 @@ freopen(file, mode, fp) /* Get a new descriptor to refer to the new file. */ f = _open(file, oflags, DEFFILEMODE); - if (f < 0 && isopen) { - /* If out of fd's close the old one and try again. */ - if (errno == ENFILE || errno == EMFILE) { - (void) (*fp->_close)(fp->_cookie); - isopen = 0; - f = _open(file, oflags, DEFFILEMODE); - } - } sverrno = errno; finish: @@ -165,9 +157,11 @@ finish: * Finish closing fp. Even if the open succeeded above, we cannot * keep fp->_base: it may be the wrong size. This loses the effect * of any setbuffer calls, but stdio has always done this before. + * + * Leave the existing file descriptor open until dup2() is called + * below to avoid races where a concurrent open() in another thread + * could claim the existing descriptor. */ - if (isopen) - (void) (*fp->_close)(fp->_cookie); if (fp->_flags & __SMBF) free((char *)fp->_bf._base); fp->_w = 0; @@ -186,6 +180,8 @@ finish: memset(&fp->_mbstate, 0, sizeof(mbstate_t)); if (f < 0) { /* did not get it after all */ + if (isopen) + (void) (*fp->_close)(fp->_cookie); fp->_flags = 0; /* set it free */ FUNLOCKFILE(fp); errno = sverrno; /* restore in case _close clobbered */ @@ -197,11 +193,12 @@ finish: * to maintain the descriptor. Various C library routines (perror) * assume stderr is always fd STDERR_FILENO, even if being freopen'd. */ - if (wantfd >= 0 && f != wantfd) { + if (wantfd >= 0) { if (_dup2(f, wantfd) >= 0) { (void)_close(f); f = wantfd; - } + } else + (void)_close(fp->_file); } /* _______________________________________________ svn-src-all@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscribe@freebsd.org"
State Changed From-To: open->patched Fix committed to HEAD, will attempt to merge it for 7.4 and 8.2.
Responsible Changed From-To: freebsd-threads->jhb Fix committed to HEAD, will attempt to merge it for 7.4 and 8.2.
State Changed From-To: patched->closed Fix merged to 7 and 8.