From 9b609c0c7310dd0c01631e323027030cbd480eee Mon Sep 17 00:00:00 2001 From: "Benjamin M. Schwartz" Date: Mon, 21 Sep 2020 13:57:19 -0400 Subject: [PATCH] Don't use os.NewFile to create the TUN writer (#63) * Don't use os.NewFile to create the TUN writer os.NewFile takes ownership of the file descriptor, and adds a finalizer that closes the file descriptor when the file is garbage-collected. This violates Outline's interface contract (caller retains ownership of the TUN device), and could cause a problem if the file descriptor is still being used See https://github.com/Jigsaw-Code/Intra/issues/285 * Return to MakeTunFile, but add a call to Dup * Remove refactoring changes * Close the tunWriter and update comments --- go.mod | 2 +- intra/intra.go | 3 +++ outline/android/tun2socks.go | 4 +++- tunnel/intra.go | 2 +- tunnel/outline.go | 2 +- tunnel/tun.go | 11 ++++++++++- tunnel/tunnel.go | 1 + 7 files changed, 20 insertions(+), 5 deletions(-) diff --git a/go.mod b/go.mod index 0806bc7..f8a9d1d 100644 --- a/go.mod +++ b/go.mod @@ -17,6 +17,6 @@ require ( golang.org/x/crypto v0.0.0-20200820211705-5c72a883971a // indirect golang.org/x/mobile v0.0.0-20200329125638-4c31acba0007 // indirect golang.org/x/net v0.0.0-20200904194848-62affa334b73 - golang.org/x/sys v0.0.0-20200909081042-eff7692f9009 // indirect + golang.org/x/sys v0.0.0-20200909081042-eff7692f9009 google.golang.org/protobuf v1.25.0 // indirect ) diff --git a/intra/intra.go b/intra/intra.go index 4f906c3..ea654ff 100644 --- a/intra/intra.go +++ b/intra/intra.go @@ -34,6 +34,9 @@ func init() { // rules. Currently, this only consists of redirecting DNS packets to a specified // server; all other data flows directly to its destination. // +// `fd` is the TUN device. The IntraTunnel acquires an additional reference to it, which +// is released by IntraTunnel.Disconnect(), so the caller must close `fd` _and_ call +// Disconnect() in order to close the TUN device. // `fakedns` is the DNS server that the system believes it is using, in "host:port" style. // The port is normally 53. // `udpdns` and `tcpdns` are the location of the actual DNS server being used. For DNS diff --git a/outline/android/tun2socks.go b/outline/android/tun2socks.go index 207d34d..660dca9 100644 --- a/outline/android/tun2socks.go +++ b/outline/android/tun2socks.go @@ -38,7 +38,9 @@ type OutlineTunnel interface { // Returns an OutlineTunnel instance and does *not* take ownership of the TUN file descriptor; the // caller is responsible for closing after OutlineTunnel disconnects. // -// `fd` is the file descriptor to the VPN TUN device. Must be set to blocking mode. +// `fd` is the TUN device. The OutlineTunnel acquires an additional reference to it, which +// is released by OutlineTunnel.Disconnect(), so the caller must close `fd` _and_ call +// Disconnect() in order to close the TUN device. // `host` is IP address of the Shadowsocks proxy server. // `port` is the port of the Shadowsocks proxy server. // `password` is the password of the Shadowsocks proxy. diff --git a/tunnel/intra.go b/tunnel/intra.go index fe85647..5a4f8e6 100644 --- a/tunnel/intra.go +++ b/tunnel/intra.go @@ -69,7 +69,7 @@ type intratunnel struct { // `udpdns` and `tcpdns` are the actual location of the DNS server in use. // These will normally be localhost with a high-numbered port. // `dohdns` is the initial DOH transport. -// `tunWriter` is the downstream VPN tunnel +// `tunWriter` is the downstream VPN tunnel. IntraTunnel.Disconnect() will close `tunWriter`. // `dialer` and `config` will be used for all network activity. // `listener` will be notified at the completion of every tunneled socket. func NewIntraTunnel(fakedns string, dohdns doh.Transport, tunWriter io.WriteCloser, dialer *net.Dialer, config *net.ListenConfig, listener IntraListener) (IntraTunnel, error) { diff --git a/tunnel/outline.go b/tunnel/outline.go index 6aaa3b4..9b96478 100644 --- a/tunnel/outline.go +++ b/tunnel/outline.go @@ -53,7 +53,7 @@ type outlinetunnel struct { // `password` is the password of the Shadowsocks proxy. // `cipher` is the encryption cipher used by the Shadowsocks proxy. // `isUDPEnabled` indicates if the Shadowsocks proxy and the network support proxying UDP traffic. -// `tunWriter` is used to output packets back to the TUN device. +// `tunWriter` is used to output packets back to the TUN device. OutlineTunnel.Disconnect() will close `tunWriter`. func NewOutlineTunnel(host string, port int, password, cipher string, isUDPEnabled bool, tunWriter io.WriteCloser) (OutlineTunnel, error) { if tunWriter == nil { return nil, errors.New("Must provide a TUN writer") diff --git a/tunnel/tun.go b/tunnel/tun.go index e7e2a60..92980d2 100644 --- a/tunnel/tun.go +++ b/tunnel/tun.go @@ -6,16 +6,25 @@ import ( "github.com/eycorsican/go-tun2socks/common/log" _ "github.com/eycorsican/go-tun2socks/common/log/simple" // Import simple log for the side effect of making logs printable. + "golang.org/x/sys/unix" ) const vpnMtu = 1500 // MakeTunFile returns an os.File object from a TUN file descriptor `fd`. +// The returned os.File holds a separate reference to the underlying file, +// so the file will not be closed until both `fd` and the os.File are +// separately closed. (UNIX only.) func MakeTunFile(fd int) (*os.File, error) { if fd < 0 { return nil, errors.New("Must provide a valid TUN file descriptor") } - file := os.NewFile(uintptr(fd), "") + // Make a copy of `fd` so that os.File's finalizer doesn't close `fd`. + newfd, err := unix.Dup(fd) + if err != nil { + return nil, err + } + file := os.NewFile(uintptr(newfd), "") if file == nil { return nil, errors.New("Failed to open TUN file descriptor") } diff --git a/tunnel/tunnel.go b/tunnel/tunnel.go index 01429c9..2df9513 100644 --- a/tunnel/tunnel.go +++ b/tunnel/tunnel.go @@ -47,6 +47,7 @@ func (t *tunnel) Disconnect() { } t.isConnected = false t.lwipStack.Close() + t.tunWriter.Close() } func (t *tunnel) Write(data []byte) (int, error) {