Merge pull request #5464 from tianon/close-leftover-fds

daemon/execdriver/lxc/driver.go

@@ -5,6 +5,7 @@ import (
 	"github.com/dotcloud/docker/daemon/execdriver"
 	"github.com/dotcloud/docker/pkg/cgroups"
 	"github.com/dotcloud/docker/pkg/label"
+	"github.com/dotcloud/docker/pkg/system"
 	"github.com/dotcloud/docker/utils"
 	"io/ioutil"
 	"log"
@@ -42,6 +43,10 @@ func init() {
 			return err
 		}
 
+		if err := system.CloseFdsFrom(3); err != nil {
+			return err
+		}
+
 		if err := changeUser(args); err != nil {
 			return err
 		}

hack/dind

@@ -70,22 +70,6 @@ grep -q :devices: /proc/1/cgroup ||
 grep -qw devices /proc/1/cgroup ||
 	echo "WARNING: it looks like the 'devices' cgroup is not mounted."
 
-# Now, close extraneous file descriptors.
-pushd /proc/self/fd >/dev/null
-for FD in *
-do
-	case "$FD" in
-	# Keep stdin/stdout/stderr
-	[012])
-		;;
-	# Nuke everything else
-	*)
-		eval exec "$FD>&-"
-		;;
-	esac
-done
-popd >/dev/null
-
 # Mount /tmp
 mount -t tmpfs none /tmp
 

hack/make/test-integration-cli

@@ -19,6 +19,9 @@ bundle_test_integration_cli() {
 		false
 	fi
 
+	# intentionally open a couple bogus file descriptors to help test that they get scrubbed in containers
+	exec 41>&1 42>&2
+
 	( set -x; exec \
 		docker --daemon --debug \
 		--storage-driver "$DOCKER_GRAPHDRIVER" \

integration-cli/docker_cli_run_test.go

@@ -92,6 +92,22 @@ func TestDockerRunEchoNamedContainer(t *testing.T) {
 	logDone("run - echo with named container")
 }
 
+// docker run should not leak file descriptors
+func TestDockerRunLeakyFileDescriptors(t *testing.T) {
+	runCmd := exec.Command(dockerBinary, "run", "busybox", "ls", "-C", "/proc/self/fd")
+	out, _, _, err := runCommandWithStdoutStderr(runCmd)
+	errorOut(err, t, out)
+
+	// normally, we should only get 0, 1, and 2, but 3 gets created by "ls" when it does "opendir" on the "fd" directory
+	if out != "0  1  2  3\n" {
+		t.Errorf("container should've printed '0  1  2  3', not: %s", out)
+	}
+
+	deleteAllContainers()
+
+	logDone("run - check file descriptor leakage")
+}
+
 // it should be possible to ping Google DNS resolver
 // this will fail when Internet access is unavailable
 func TestDockerRunPingGoogle(t *testing.T) {

pkg/libcontainer/nsinit/init.go

@@ -117,12 +117,16 @@ func setupNetwork(container *libcontainer.Container, context libcontainer.Contex
 	return nil
 }
 
-// finalizeNamespace drops the caps and sets the correct user
-// and working dir before execing the command inside the namespace
+// finalizeNamespace drops the caps, sets the correct user
+// and working dir, and closes any leaky file descriptors
+// before execing the command inside the namespace
 func finalizeNamespace(container *libcontainer.Container) error {
 	if err := capabilities.DropCapabilities(container); err != nil {
 		return fmt.Errorf("drop capabilities %s", err)
 	}
+	if err := system.CloseFdsFrom(3); err != nil {
+		return fmt.Errorf("close open file descriptors %s", err)
+	}
 	if err := setupUser(container); err != nil {
 		return fmt.Errorf("setup user %s", err)
 	}

pkg/system/fds_linux.go

@@ -0,0 +1,38 @@
+package system
+
+import (
+	"io/ioutil"
+	"strconv"
+	"syscall"
+)
+
+// Works similarly to OpenBSD's "closefrom(2)":
+//   The closefrom() call deletes all descriptors numbered fd and higher from
+//   the per-process file descriptor table.  It is effectively the same as
+//   calling close(2) on each descriptor.
+// http://www.openbsd.org/cgi-bin/man.cgi?query=closefrom&sektion=2
+//
+// See also http://stackoverflow.com/a/918469/433558
+func CloseFdsFrom(minFd int) error {
+	fdList, err := ioutil.ReadDir("/proc/self/fd")
+	if err != nil {
+		return err
+	}
+	for _, fi := range fdList {
+		fd, err := strconv.Atoi(fi.Name())
+		if err != nil {
+			// ignore non-numeric file names
+			continue
+		}
+
+		if fd < minFd {
+			// ignore descriptors lower than our specified minimum
+			continue
+		}
+
+		// intentionally ignore errors from syscall.Close
+		syscall.Close(fd)
+		// the cases where this might fail are basically file descriptors that have already been closed (including and especially the one that was created when ioutil.ReadDir did the "opendir" syscall)
+	}
+	return nil
+}

pkg/system/fds_unsupported.go

@@ -0,0 +1,12 @@
+// +build !linux
+
+package system
+
+import (
+	"fmt"
+	"runtime"
+)
+
+func CloseFdsFrom(minFd int) error {
+	return fmt.Errorf("CloseFdsFrom is unsupported on this platform (%s/%s)", runtime.GOOS, runtime.GOARCH)
+}