Another ping.... On Thu, Jun 5, 2014 at 2:54 PM, Michael Kerrisk (man-pages) <mtk.manpages@xxxxxxxxx> wrote: > Rich, Ping! > > On Fri, May 30, 2014 at 11:15 AM, Michael Kerrisk (man-pages) > <mtk.manpages@xxxxxxxxx> wrote: >> Hi Rich, >> >> For discussions like this, it really is very important to CC the list, >> so that there's an archived record of the reasons for the change. >> I've concatenated your two mails below. >> >> I agree that the page needs to be fixed; thanks for the report! >> I am mulling over what the best fix is. One proposal below. >> >> On 05/29/2014 11:02 PM, Rich Felker wrote: >>> Hi, >>> >>> The following text appears in the man page for dup2 and dup3: >>> >>> If newfd was open, any errors that would have been reported at >>> close(2) time are lost. A careful programmer will not use dup2() >>> or dup3() without closing newfd first. >>> >>> Such use of close is very bad advice, as it introduces a race >>> condition during which the file descriptor could be re-assigned and >>> subsequently clobbered by dup2/dup3. >> >> Agreed. A more fundamental problem in the man page is that >> it does not mention the atomicity of dup2() (and dup3()), >> and why that is needed to avoid the race condition. >> I'll add some words on that. >> >>> This type of bug can lead to >>> serious data leaks and/or data loss. The whole point of dup2/dup3 is >>> to _atomically_ replace a file descriptor. >>> >>> For the most part there are no meaningful errors which close can >>> return, probably only obscure NFS behavior with bad caching settings >>> which are really not handlable by applications anyway, so I feel it >>> would be best to just drop this text (or find a way to detect such >>> errors without close, perhaps using fsync, and recommend that). >> >> On 05/30/2014 07:23 AM, Rich Felker wrote: >> [...] >>> Here is a proposed alternate text that was recommended to me by a user >>> on Stack Overflow: >> >> It'd be great to add URLs when citing such discussions... With some >> effort, I found >> http://stackoverflow.com/questions/23440216/race-condition-when-using-dup >> >>> A careful programmer will first dup() the target descriptor, then >>> use dup2()/dup3() to replace the target descriptor atomically, and >>> finally close the initially duplicated target descriptor. This >>> replaces the target descriptor atomically, but also retains a >>> duplicate for closing so that close-time errors may be checked >>> for. (In Linux, close() should only be called once, as the >>> referred to descriptor is always closed, even in case of >>> errno==EINTR.) >>> >>> I'm not sure this is the best wording, since it suggests doing a lot >>> of work that's likely overkill (and useless in the case where the >>> target descriptor was read-only, for instance). I might balance such >>> text with a warning that it's an error to use dup2 or dup3 when the >>> target descriptor is not known to be open unless you know the code >>> will only be used in single-threaded programs. And I'm a little bit >>> hesitant on the parenthetical text about close() since the behavior >>> it's documenting is contrary to the requirements of the upcoming issue >>> 8 of POSIX, >> >> Again citing the Issue 8 discussion would be helpful. Could >> you tell me where it is? (It could be useful for the change log.) >> >>> and rather irrelevant since EINTR cannot happen in Linux's >>> close() except with custom device drivers anyway. >> >> So, how about something like the following (code not >> compile-tested...): >> >> If newfd was open, any errors that would have been reported at >> close(2) time are lost. If this is of concern, then—unless the >> program is single-threaded and does not allocate file descrip‐ >> tors in signal handlers—the correct approach is not to close >> newfd before calling dup2(), because of the race condition >> described above. Instead, code something like the following >> could be used: >> >> /* Obtain a duplicate of 'newfd' that can subsequently >> be used to check for close() errors; an EBADF error >> means that 'newfd' was not open. */ >> >> tmpfd = dup(newfd); >> if (tmpfd == -1 && errno != EBADF) { >> /* Handle unexpected dup() error */ >> } >> >> /* Atomically duplicate 'oldfd' on 'newfd' */ >> >> if (dup2(oldfd, newfd) == -1) { >> /* Handle dup2() error */ >> } >> >> /* Now check for close() errors on the file originally >> referred to by 'newfd' */ >> >> if (tmpfd != -1) { >> if (close(tmpfd) == -1) { >> /* Handle errors from close */ >> } >> } >> >> Cheers, >> >> Michael >> >> >> -- >> Michael Kerrisk >> Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/ >> Linux/UNIX System Programming Training: http://man7.org/training/ > > > > -- > Michael Kerrisk > Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/ > Linux/UNIX System Programming Training: http://man7.org/training/ -- Michael Kerrisk Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/ Linux/UNIX System Programming Training: http://man7.org/training/ -- To unsubscribe from this list: send the line "unsubscribe linux-man" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
