From fc2414f988ed9bb5373c096e20cc7c2d9d0dc112 Mon Sep 17 00:00:00 2001
From: Max Timchenko <maxvt@pagerduty.com>
Date: Wed, 8 Mar 2017 20:37:13 +0200
Subject: [PATCH] Ensure iptables initialization only happens once

I saw a rare race during the first few calls to iptables module
where some of them would reenter initCheck() after the first call
to it already changed iptablesPath, but before the rest of the function
completed (in particular the long execs into testing for availability
of --wait flag and determining iptables version), resulting in
failure of one or more of iptables calls that did not use --wait and
were concurrent.

To fix the problem, this change gathers all one-time initialization into a
single function under a sync.Once instead of using a global variable
as a "done initializing" flag before initialization is done. sync.Once
guarantees all concurrent calls will block until the first one completes.

In addition, it turns out that GetVersion(), called from initCheck(), used
Raw() which called back into initCheck() via raw(), which did not cause a
problem in the earlier implementation but deadlocked when initialization became
strict.  This was changed to use a direct call, similar to initialization of
supportsXlock.

Signed-off-by: Max Timchenko <max@maxvt.com>
---
 libnetwork/iptables/iptables.go | 51 +++++++++++++++++++++------------
 1 file changed, 32 insertions(+), 19 deletions(-)

diff --git a/libnetwork/iptables/iptables.go b/libnetwork/iptables/iptables.go
index 9d06eb2f18..34f7dee09d 100644
--- a/libnetwork/iptables/iptables.go
+++ b/libnetwork/iptables/iptables.go
@@ -50,8 +50,7 @@ var (
 	bestEffortLock sync.Mutex
 	// ErrIptablesNotFound is returned when the rule is not found.
 	ErrIptablesNotFound = errors.New("Iptables not found")
-	probeOnce           sync.Once
-	firewalldOnce       sync.Once
+	initOnce            sync.Once
 )
 
 // ChainInfo defines the iptables chain.
@@ -86,22 +85,32 @@ func initFirewalld() {
 	}
 }
 
+func detectIptables() {
+	path, err := exec.LookPath("iptables")
+	if err != nil {
+		return
+	}
+	iptablesPath = path
+	supportsXlock = exec.Command(iptablesPath, "--wait", "-L", "-n").Run() == nil
+	mj, mn, mc, err := GetVersion()
+	if err != nil {
+		logrus.Warnf("Failed to read iptables version: %v", err)
+		return
+	}
+	supportsCOpt = supportsCOption(mj, mn, mc)
+}
+
+func initIptables() {
+	probe()
+	initFirewalld()
+	detectIptables()
+}
+
 func initCheck() error {
+	initOnce.Do(initIptables)
+
 	if iptablesPath == "" {
-		probeOnce.Do(probe)
-		firewalldOnce.Do(initFirewalld)
-		path, err := exec.LookPath("iptables")
-		if err != nil {
-			return ErrIptablesNotFound
-		}
-		iptablesPath = path
-		supportsXlock = exec.Command(iptablesPath, "--wait", "-L", "-n").Run() == nil
-		mj, mn, mc, err := GetVersion()
-		if err != nil {
-			logrus.Warnf("Failed to read iptables version: %v", err)
-			return nil
-		}
-		supportsCOpt = supportsCOption(mj, mn, mc)
+		return ErrIptablesNotFound
 	}
 	return nil
 }
@@ -373,7 +382,11 @@ func exists(native bool, table Table, chain string, rule ...string) bool {
 		table = Filter
 	}
 
-	initCheck()
+	if err := initCheck(); err != nil {
+		// The exists() signature does not allow us to return an error, but at least
+		// we can skip the (likely invalid) exec invocation.
+		return false
+	}
 
 	if supportsCOpt {
 		// if exit status is 0 then return true, the rule exists
@@ -456,9 +469,9 @@ func ExistChain(chain string, table Table) bool {
 	return false
 }
 
-// GetVersion reads the iptables version numbers
+// GetVersion reads the iptables version numbers during initialization
 func GetVersion() (major, minor, micro int, err error) {
-	out, err := Raw("--version")
+	out, err := exec.Command(iptablesPath, "--version").CombinedOutput()
 	if err == nil {
 		major, minor, micro = parseVersionNumbers(string(out))
 	}