From da8d51ddbda88ec43642ef2be03cb920dea50279 Mon Sep 17 00:00:00 2001
From: Sebastiaan van Stijn <github@gone.nl>
Date: Tue, 15 Aug 2023 18:03:33 +0200
Subject: [PATCH] libnetwork/iptables: remove firewalldRunning package-var

Make the firewalld status a property of firewalldConnection, so that all
status is captured in the `firewalld` instance.

firewalldInit() already checks if firewalld is running when initializing,
in which case the `firewalld` variable would be nil.

While refactoring, also use an `atomic.Bool`; the running-status can be
updated as part of `startSignalHandler()`, which means that there's a
potential concurrency-issue with other code accessing the firewalld
status.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
---
 libnetwork/iptables/firewalld.go      | 33 +++++++++++++++++----------
 libnetwork/iptables/firewalld_test.go | 10 ++++++++
 libnetwork/iptables/iptables.go       |  2 +-
 3 files changed, 32 insertions(+), 13 deletions(-)

diff --git a/libnetwork/iptables/firewalld.go b/libnetwork/iptables/firewalld.go
index 3eaa45ec36..e9fa0bf6b6 100644
--- a/libnetwork/iptables/firewalld.go
+++ b/libnetwork/iptables/firewalld.go
@@ -6,6 +6,7 @@ import (
 	"context"
 	"fmt"
 	"strings"
+	"sync/atomic"
 
 	"github.com/containerd/containerd/log"
 	dbus "github.com/godbus/dbus/v5"
@@ -31,6 +32,7 @@ const (
 // firewalldConnection is a connection to the firewalld dbus endpoint.
 type firewalldConnection struct {
 	conn       *dbus.Conn
+	running    atomic.Bool
 	sysObj     dbus.BusObject
 	sysConfObj dbus.BusObject
 	signal     chan *dbus.Signal
@@ -39,8 +41,7 @@ type firewalldConnection struct {
 var (
 	firewalld *firewalldConnection
 
-	firewalldRunning bool      // is Firewalld service running
-	onReloaded       []*func() // callbacks when Firewalld has been reloaded
+	onReloaded []*func() // callbacks when Firewalld has been reloaded
 )
 
 // firewalldInit initializes firewalld management code.
@@ -82,8 +83,7 @@ func newConnection() (*firewalldConnection, error) {
 		sysConfObj: conn.Object(dbusInterface, dbusConfigPath),
 	}
 
-	firewalldRunning = checkRunning(c)
-	if !firewalldRunning {
+	if !c.checkRunning() {
 		_ = c.conn.Close()
 		return nil, fmt.Errorf("firewalld is not running")
 	}
@@ -108,7 +108,7 @@ func (fwd *firewalldConnection) handleSignals() {
 			switch {
 			case strings.Contains(signal.Name, "NameOwnerChanged"):
 				// re-check if firewalld is still running.
-				checkRunning(fwd)
+				fwd.checkRunning()
 				dbusConnectionChanged(signal.Body)
 
 			case strings.Contains(signal.Name, "Reloaded"):
@@ -162,13 +162,22 @@ func OnReloaded(callback func()) {
 // checkRunning checks if firewalld is running.
 //
 // It calls some remote method to see whether the service is actually running.
-func checkRunning(conn *firewalldConnection) bool {
-	if conn == nil {
+func (fwd *firewalldConnection) checkRunning() bool {
+	var zone string
+	if err := fwd.sysObj.Call(dbusInterface+".getDefaultZone", 0).Store(&zone); err != nil {
+		fwd.running.Store(false)
+	} else {
+		fwd.running.Store(true)
+	}
+	return fwd.running.Load()
+}
+
+// isRunning returns whether firewalld is running.
+func (fwd *firewalldConnection) isRunning() bool {
+	if fwd == nil {
 		return false
 	}
-	var zone string
-	err := conn.sysObj.Call(dbusInterface+".getDefaultZone", 0).Store(&zone)
-	return err == nil
+	return fwd.running.Load()
 }
 
 // Passthrough method simply passes args through to iptables/ip6tables
@@ -262,7 +271,7 @@ func (fwd *firewalldConnection) setupDockerZone() error {
 // AddInterfaceFirewalld adds the interface to the trusted zone. It is a
 // no-op if firewalld is not running.
 func AddInterfaceFirewalld(intf string) error {
-	if !firewalldRunning {
+	if !firewalld.isRunning() {
 		return nil
 	}
 
@@ -288,7 +297,7 @@ func AddInterfaceFirewalld(intf string) error {
 // DelInterfaceFirewalld removes the interface from the trusted zone It is a
 // no-op if firewalld is not running.
 func DelInterfaceFirewalld(intf string) error {
-	if !firewalldRunning {
+	if !firewalld.isRunning() {
 		return nil
 	}
 
diff --git a/libnetwork/iptables/firewalld_test.go b/libnetwork/iptables/firewalld_test.go
index 029c874b79..d715d19f81 100644
--- a/libnetwork/iptables/firewalld_test.go
+++ b/libnetwork/iptables/firewalld_test.go
@@ -103,3 +103,13 @@ func TestPassthrough(t *testing.T) {
 		t.Fatal("rule1 does not exist")
 	}
 }
+
+// TestFirewalldUninitialized checks that calling methods, such as isRunning()
+// on an empty, uninitialized firewalldConnection doesn't panic, and returns
+// the expected status.
+func TestFirewalldUninitialized(t *testing.T) {
+	var fwd *firewalldConnection
+	if fwd.isRunning() {
+		t.Error("did not expect an uninitialized firewalldConnection to be running")
+	}
+}
diff --git a/libnetwork/iptables/iptables.go b/libnetwork/iptables/iptables.go
index 3b3d14a7aa..4483a2cf38 100644
--- a/libnetwork/iptables/iptables.go
+++ b/libnetwork/iptables/iptables.go
@@ -518,7 +518,7 @@ func filterOutput(start time.Time, output []byte, args ...string) []byte {
 
 // Raw calls 'iptables' system command, passing supplied arguments.
 func (iptable IPTable) Raw(args ...string) ([]byte, error) {
-	if firewalldRunning {
+	if firewalld.isRunning() {
 		// select correct IP version for firewalld
 		ipv := Iptables
 		if iptable.ipVersion == IPv6 {