Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

net: Conn.File uses dup #5052

Closed
gopherbot opened this issue Mar 15, 2013 · 2 comments
Closed

net: Conn.File uses dup #5052

gopherbot opened this issue Mar 15, 2013 · 2 comments

Comments

@gopherbot
Copy link

net.TCPConn embeds net.conn, which has this:

// File sets the underlying os.File to blocking mode and returns a copy.
// It is the caller's responsibility to close f when finished.
// Closing c does not affect f, and closing f does not affect c.
//
// The returned os.File's file descriptor is different from the connection's.
// Attempting to change properties of the original using this duplicate
// may or may not have the desired effect.
func (c *conn) File() (f *os.File, err error) { return c.fd.dup() }

And elsewhere:

func (fd *netFD) dup() (f *os.File, err error) {
    syscall.ForkLock.RLock()
    ns, err := syscall.Dup(fd.sysfd)
    if err != nil {
        syscall.ForkLock.RUnlock()
        return nil, &OpError{"dup", fd.net, fd.laddr, err}
    }
    syscall.CloseOnExec(ns)
    syscall.ForkLock.RUnlock()

    // We want blocking mode for the new fd, hence the double negative.
    // This also puts the old fd into blocking mode, meaning that
    // I/O will block the thread instead of letting us use the epoll server.
    // Everything will still work, just with more threads.
    if err = syscall.SetNonblock(ns, false); err != nil {
        return nil, &OpError{"setnonblock", fd.net, fd.laddr, err}
    }

    return os.NewFile(uintptr(ns), fd.name()), nil
}


However, O_NONBLOCK is a property of the open file, not the fd.
It is shared between all fds referring to the same file, *even
across processes*.

http://cr.yp.to/unix/nonblock.html

Hence, this protection does not actually work. To add insult to injury, calling .File()
*actively* screws up the epoll by disabling O_NONBLOCK.

Here's a demonstration that setting O_NONBLOCK on a dup'd socket
affects the duplicates, also at http://play.golang.org/p/w9Fwqofegp though not runnable
in the playground:

package main

import (
    "fmt"
    "syscall"
)

// copy-paste from src/pkg/syscall/zsyscall_linux_amd64.go
func fcntl(fd int, cmd int, arg int) (val int, err error) {
    r0, _, e1 := syscall.Syscall(syscall.SYS_FCNTL, uintptr(fd), uintptr(cmd), uintptr(arg))
    val = int(r0)
    if e1 != 0 {
        err = e1
    }
    return
}

func isBlocking(fd int) bool {
    val, err := fcntl(fd, syscall.F_GETFL, 0)
    if err != nil {
        panic(err)
    }
    return val&syscall.O_NONBLOCK == 0
}

func main() {
    dupped, err := syscall.Dup(0)
    if err != nil {
        panic(err)
    }
    fmt.Printf("before: stdin is blocking: %v\n", isBlocking(0))
    fmt.Printf("before: dupped is blocking: %v\n", isBlocking(dupped))
    err = syscall.SetNonblock(0, true)
    if err != nil {
        panic(err)
    }
    fmt.Printf("after: stdin is blocking: %v\n", isBlocking(0))
    fmt.Printf("after: dupped is blocking: %v\n", isBlocking(dupped))
}
@rsc
Copy link
Contributor

rsc commented Mar 15, 2013

Comment 1:

I realize that clearing O_NONBLOCK affects the original fd on many systems. File is
meant for use passing a network connection as a file when exec'ing a subprocess. Other
uses - such as for fiddling with flags - are explicitly not supported. That is what this
part of the comment is saying:
// The returned os.File's file descriptor is different from the connection's.
// Attempting to change properties of the original using this duplicate
// may or may not have the desired effect.
In the context of passing an fd to a subprocess, we made the decision that it was
important for the subprocess to start with its fd in blocking mode, so that
non-network-aware subprocesses can still interact with network fds using ordinary
(blocking) read and write calls.
The code snippet you quoted makes clear that it understands the aliasing:
    // We want blocking mode for the new fd, hence the double negative.
    // This also puts the old fd into blocking mode, meaning that
    // I/O will block the thread instead of letting us use the epoll server.
    // Everything will still work, just with more threads.
For what it's worth, the File method uses dup primarily so that the returned *os.File
can be closed independently of the original net.Conn. Not doing so would create a
situation where it is possible to close a net.Conn accidentally by closing the file, or
vice versa, and it would be the only call anywhere that returns an open *os.File that
must not be closed.
Your report does not suggest any changes, so I am closing it as working as intended.

Status changed to WorkingAsIntended.

@gopherbot
Copy link
Author

Comment 2:

Ah, ok, so then it's slightly ambiguous comments (better on a second reading) combined
with confusion on IRC. I blame Dave ;)
17:26 <davecheney> rcrowley: there is a note on that file that
                   explains why we can't make the underlying fd
                   available
17:27 <davecheney> (or *os.File)
17:27 <davecheney> because if the blocking mode is changed on that
                   file, it will screw up the networking code
That's what I started chasing down, saying "but dup won't protect you!". If this is as
intended, then it's all good with me. (Personally, I'd expect if I .File() a socket, I
shouldn't touch the original any more, so the "more threads" case shouldn't really
trigger often.)

@golang golang locked and limited conversation to collaborators Jun 24, 2016
This issue was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

2 participants