[lxc-devel] [lxd/master] Close race condition in image download

[stgraber on Github]

A non-text attachment was scrubbed...
Name: not available
Type: text/x-mailbox
Size: 370 bytes
Desc: not available
URL: <http://lists.linuxcontainers.org/pipermail/lxc-devel/attachments/20170124/837c5fd2/attachment.bin>
-------------- next part --------------
From f41a3961ed26c50c8e2976937b24c423eac1cec3 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?St=C3=A9phane=20Graber?= <stgraber at ubuntu.com>
Date: Mon, 23 Jan 2017 23:42:29 -0500
Subject: [PATCH] Close race condition in image download
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Closes #2739

Signed-off-by: Stéphane Graber <stgraber at ubuntu.com>
---
 lxd/daemon.go                |  2 +-
 lxd/daemon_images.go         | 24 +++++++++++-------------
 lxd/main_activateifneeded.go |  2 +-
 lxd/main_test.go             |  2 +-
 4 files changed, 14 insertions(+), 16 deletions(-)

diff --git a/lxd/daemon.go b/lxd/daemon.go
index 7d565a5..c895e84 100644
--- a/lxd/daemon.go
+++ b/lxd/daemon.go
@@ -91,7 +91,7 @@ type Daemon struct {
 	SetupMode bool
 
 	imagesDownloading     map[string]chan bool
-	imagesDownloadingLock sync.RWMutex
+	imagesDownloadingLock sync.Mutex
 
 	tlsConfig *tls.Config
 
diff --git a/lxd/daemon_images.go b/lxd/daemon_images.go
index 3998d7b..97739ab 100644
--- a/lxd/daemon_images.go
+++ b/lxd/daemon_images.go
@@ -188,10 +188,10 @@ func (d *Daemon) ImageDownload(op *operation, server string, protocol string, ce
 	}
 
 	// Now check if we already downloading the image
-	d.imagesDownloadingLock.RLock()
+	d.imagesDownloadingLock.Lock()
 	if waitChannel, ok := d.imagesDownloading[fp]; ok {
 		// We already download the image
-		d.imagesDownloadingLock.RUnlock()
+		d.imagesDownloadingLock.Unlock()
 
 		shared.LogDebug(
 			"Already downloading the image, waiting for it to succeed",
@@ -217,18 +217,7 @@ func (d *Daemon) ImageDownload(op *operation, server string, protocol string, ce
 		return fp, nil
 	}
 
-	d.imagesDownloadingLock.RUnlock()
-
-	if op == nil {
-		ctxMap = log.Ctx{"alias": alias, "server": server}
-	} else {
-		ctxMap = log.Ctx{"trigger": op.url, "image": fp, "operation": op.id, "alias": alias, "server": server}
-	}
-
-	shared.LogInfo("Downloading image", ctxMap)
-
 	// Add the download to the queue
-	d.imagesDownloadingLock.Lock()
 	d.imagesDownloading[fp] = make(chan bool)
 	d.imagesDownloadingLock.Unlock()
 
@@ -242,6 +231,15 @@ func (d *Daemon) ImageDownload(op *operation, server string, protocol string, ce
 		d.imagesDownloadingLock.Unlock()
 	}()
 
+	// Begin downloading
+	if op == nil {
+		ctxMap = log.Ctx{"alias": alias, "server": server}
+	} else {
+		ctxMap = log.Ctx{"trigger": op.url, "image": fp, "operation": op.id, "alias": alias, "server": server}
+	}
+
+	shared.LogInfo("Downloading image", ctxMap)
+
 	exporturl := server
 
 	var info api.Image
diff --git a/lxd/main_activateifneeded.go b/lxd/main_activateifneeded.go
index 50cce63..b0d66d2 100644
--- a/lxd/main_activateifneeded.go
+++ b/lxd/main_activateifneeded.go
@@ -11,7 +11,7 @@ func cmdActivateIfNeeded() error {
 	// Don't start a full daemon, we just need DB access
 	d := &Daemon{
 		imagesDownloading:     map[string]chan bool{},
-		imagesDownloadingLock: sync.RWMutex{},
+		imagesDownloadingLock: sync.Mutex{},
 		lxcpath:               shared.VarPath("containers"),
 	}
 
diff --git a/lxd/main_test.go b/lxd/main_test.go
index f5f4518..e3af4e1 100644
--- a/lxd/main_test.go
+++ b/lxd/main_test.go
@@ -16,7 +16,7 @@ func mockStartDaemon() (*Daemon, error) {
 	d := &Daemon{
 		MockMode:              true,
 		imagesDownloading:     map[string]chan bool{},
-		imagesDownloadingLock: sync.RWMutex{},
+		imagesDownloadingLock: sync.Mutex{},
 	}
 
 	if err := d.Init(); err != nil {