Re: Very bad advice in man 2 dup2

* Re: Very bad advice in man 2 dup2
       [not found] ` <20140529210242.GP507-C3MtFaGISjmo6RMmaWD+6Sb1p8zYI1N1@public.gmane.org>
@ 2014-05-30  9:15   ` Michael Kerrisk (man-pages)
       [not found]     ` <53884C27.7020207-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
  0 siblings, 1 reply; 6+ messages in thread
From: Michael Kerrisk (man-pages) @ 2014-05-30  9:15 UTC (permalink / raw)
  To: Rich Felker
  Cc: mtk.manpages-Re5JQEeQqe8AvxtiuMwx3w, linux-man-u79uwXL29TY76Z2rM5mHXA

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

^ permalink raw reply	[flat|nested] 6+ messages in thread