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 Jigsaw-Code/Intra#285

* Return to MakeTunFile, but add a call to Dup

* Remove refactoring changes

* Close the tunWriter and update comments

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
 )

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

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.

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) {

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")

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")
 	}

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) {