Re: Very bad advice in man 2 dup2

["Michael Kerrisk (man-pages)" <mtk.manpages-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]

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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html