[lxc-devel] [lxd/master] Operations: Fixes race condition and unsafe Operations() function

[tomponline on Github]

A non-text attachment was scrubbed...
Name: not available
Type: text/x-mailbox
Size: 999 bytes
Desc: not available
URL: <http://lists.linuxcontainers.org/pipermail/lxc-devel/attachments/20200720/01faac17/attachment.bin>
-------------- next part --------------
From 71ea44fd8eef1a84f550eafec54adbe64ca68cd2 Mon Sep 17 00:00:00 2001
From: Thomas Parrott <thomas.parrott at canonical.com>
Date: Mon, 20 Jul 2020 14:36:21 +0100
Subject: [PATCH 1/3] lxd/operations/operations: Renames Operations to Clone

And updates to return a copy of the map rather than an non-current safe reference to the map.

Signed-off-by: Thomas Parrott <thomas.parrott at canonical.com>
---
 lxd/operations/operations.go | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/lxd/operations/operations.go b/lxd/operations/operations.go
index ffba288981..6b0f3c50b6 100644
--- a/lxd/operations/operations.go
+++ b/lxd/operations/operations.go
@@ -59,9 +59,17 @@ func Unlock() {
 	operationsLock.Unlock()
 }
 
-// Operations returns a map of operations.
-func Operations() map[string]*Operation {
-	return operations
+// Clone returns a clone of the internal operations map containing references to the actual operations.
+func Clone() map[string]*Operation {
+	operationsLock.Lock()
+	defer operationsLock.Unlock()
+
+	localOperations := make(map[string]*Operation, len(operations))
+	for k, v := range operations {
+		localOperations[k] = v
+	}
+
+	return localOperations
 }
 
 // OperationGetInternal returns the operation with the given id. It returns an

From f6ab2e407794f95d06b992f47a559e0442080fa4 Mon Sep 17 00:00:00 2001
From: Thomas Parrott <thomas.parrott at canonical.com>
Date: Mon, 20 Jul 2020 14:37:18 +0100
Subject: [PATCH 2/3] lxd-agent/operations: operations.Clone() usage

Signed-off-by: Thomas Parrott <thomas.parrott at canonical.com>
---
 lxd-agent/operations.go | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/lxd-agent/operations.go b/lxd-agent/operations.go
index 004bb82a28..3c8f033f13 100644
--- a/lxd-agent/operations.go
+++ b/lxd-agent/operations.go
@@ -74,9 +74,7 @@ func operationsGet(d *Daemon, r *http.Request) response.Response {
 
 	localOperationURLs := func() (shared.Jmap, error) {
 		// Get all the operations
-		operations.Lock()
-		ops := operations.Operations()
-		operations.Unlock()
+		ops := operations.Clone()
 
 		// Build a list of URLs
 		body := shared.Jmap{}
@@ -96,9 +94,7 @@ func operationsGet(d *Daemon, r *http.Request) response.Response {
 
 	localOperations := func() (shared.Jmap, error) {
 		// Get all the operations
-		operations.Lock()
-		ops := operations.Operations()
-		operations.Unlock()
+		ops := operations.Clone()
 
 		// Build a list of operations
 		body := shared.Jmap{}

From 11fe9d6debc027ca19f78e60301f4cb536c5e406 Mon Sep 17 00:00:00 2001
From: Thomas Parrott <thomas.parrott at canonical.com>
Date: Mon, 20 Jul 2020 14:37:41 +0100
Subject: [PATCH 3/3] lxd: operations.Clone() usage

Signed-off-by: Thomas Parrott <thomas.parrott at canonical.com>
---
 lxd/images.go     |  2 +-
 lxd/operations.go | 12 +++---------
 2 files changed, 4 insertions(+), 10 deletions(-)

diff --git a/lxd/images.go b/lxd/images.go
index a5927a0000..39c7df8d75 100644
--- a/lxd/images.go
+++ b/lxd/images.go
@@ -1635,7 +1635,7 @@ func doImageGet(db *db.Cluster, project, fingerprint string, public bool) (*api.
 }
 
 func imageValidSecret(fingerprint string, secret string) (*operations.Operation, bool) {
-	for _, op := range operations.Operations() {
+	for _, op := range operations.Clone() {
 		if op.Resources() == nil {
 			continue
 		}
diff --git a/lxd/operations.go b/lxd/operations.go
index 854351ab10..e1815a02ef 100644
--- a/lxd/operations.go
+++ b/lxd/operations.go
@@ -59,9 +59,7 @@ func waitForOperations(s *state.State, chCancel chan struct{}) {
 		<-tick
 
 		// Get all the operations
-		operations.Lock()
-		ops := operations.Operations()
-		operations.Unlock()
+		ops := operations.Clone()
 
 		runningOps := 0
 
@@ -209,9 +207,7 @@ func operationsGet(d *Daemon, r *http.Request) response.Response {
 
 	localOperationURLs := func() (shared.Jmap, error) {
 		// Get all the operations
-		operations.Lock()
-		localOps := operations.Operations()
-		operations.Unlock()
+		localOps := operations.Clone()
 
 		// Build a list of URLs
 		body := shared.Jmap{}
@@ -234,9 +230,7 @@ func operationsGet(d *Daemon, r *http.Request) response.Response {
 
 	localOperations := func() (shared.Jmap, error) {
 		// Get all the operations
-		operations.Lock()
-		localOps := operations.Operations()
-		operations.Unlock()
+		localOps := operations.Clone()
 
 		// Build a list of operations
 		body := shared.Jmap{}