On 06/29/2014 05:05 AM, Rich Felker wrote: > On Thu, Jun 12, 2014 at 08:53:38PM +0200, Michael Kerrisk (man-pages) wrote: >> Another ping.... > > Sorry for not getting back to you on this! > >>>>> 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.) > > See the issue that added all the new atomic close-on-exec operations: > http://austingroupbugs.net/view.php?id=411 > > The resolution of 0000149 documented that using close( ) on > arbitrary file descriptors is unsafe, and that applications should > instead atomically create file descriptors with FD_CLOEXEC already > set. [...] > > And the issue referenced there: > http://austingroupbugs.net/view.php?id=149 > >>>>> 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 */ >>>> } >>>> } > > This code looks like it works, but it's a lot to be recommending > without a really good reason to do so. Well, I think the para that precedes the code goes some way toward explaining why you might want to do this. > I seem to remember someone > claiming that the whole "need" to check for error on close is outdated > and not applicable to modern Linux (even with NFS?) but I can't All the world is not Linux. And in the future, even Linux behavior might change. And I do not recall the details, but as far as I know some NFS errors can be delivered on close(). So, it seems to me that robust applications should check the status from close. > remember where. Anyway I think such code should be accompanied by a > discussion of why one might care about checking for close errors so > programmers can make an informed decision about whether it's worth the > trouble rather than cargo-culting it. There is some text on this in the close(2) page. So, for the moment, the text as I gave it, is in. It's certainly an improvement over what was there before. If you feel strongly that further a change is needed, please send me a patch (or carefully worded free text that I can drop into the page). 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
