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/ -- 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
