Fix a potential deadlock during podman cp

Have one function without a `defer lock.unlock()` as one of the
commands in it calls a function that also takes the same lock,
so the unlock has to happen prior to function completion.
Unfortunately, this is prone to errors, like the one here: I
missed a case, and we could return without unlocking, causing a
deadlock later in the cleanup code as we tried to take the same
lock again.

Refactor the command to use `defer unlock()` to simplify and
avoid any further errors of this type.

Introduced by e66b788 - this
should be included in any backports of that commit.

Fixes #25585

Signed-off-by: Matt Heon <[email protected]>

Author: mheon

libpod/container_copy_common.go

@@ -160,10 +160,10 @@ func (c *Container) copyFromArchive(path string, chown, noOverwriteDirNonDir boo
 			// populated the volume and that will block a future
 			// copy-up.
 			volume.lock.Lock()
+			defer volume.lock.Unlock()
 
 			if err := volume.update(); err != nil {
 				logrus.Errorf("Unable to update volume %s status: %v", volume.Name(), err)
-				volume.lock.Unlock()
 				return
 			}
 
@@ -172,15 +172,12 @@ func (c *Container) copyFromArchive(path string, chown, noOverwriteDirNonDir boo
 				volume.state.CopiedUp = true
 				if err := volume.save(); err != nil {
 					logrus.Errorf("Unable to save volume %s state: %v", volume.Name(), err)
-					volume.lock.Unlock()
 					return
 				}
 
-				volume.lock.Unlock()
-
 				for _, namedVol := range c.config.NamedVolumes {
 					if namedVol.Name == volume.Name() {
-						if err := c.fixVolumePermissions(namedVol); err != nil {
+						if err := c.fixVolumePermissionsUnlocked(namedVol, volume); err != nil {
 							logrus.Errorf("Unable to fix volume %s permissions: %v", volume.Name(), err)
 						}
 						return

libpod/container_internal_common.go

@@ -2916,6 +2916,10 @@ func (c *Container) fixVolumePermissions(v *ContainerNamedVolume) error {
 	vol.lock.Lock()
 	defer vol.lock.Unlock()
 
+	return c.fixVolumePermissionsUnlocked(v, vol)
+}
+
+func (c *Container) fixVolumePermissionsUnlocked(v *ContainerNamedVolume, vol *Volume) error {
 	// The volume may need a copy-up. Check the state.
 	if err := vol.update(); err != nil {
 		return err

test/e2e/cp_test.go

@@ -280,22 +280,18 @@ RUN chown 9999:9999 %s`, ALPINE, ctrVolPath, ctrVolPath)
 		defer srcFile.Close()
 		defer os.Remove(srcFile.Name())
 
-		volCreate := podmanTest.Podman([]string{"volume", "create", volName})
-		volCreate.WaitWithDefaultTimeout()
-		Expect(volCreate).Should(ExitCleanly())
+		_ = podmanTest.PodmanExitCleanly("volume", "create", volName)
+		_ = podmanTest.PodmanExitCleanly("create", "--name", ctrName, "-v", fmt.Sprintf("%s:%s", volName, ctrVolPath), imgName, "sh")
 
-		ctrCreate := podmanTest.Podman([]string{"create", "--name", ctrName, "-v", fmt.Sprintf("%s:%s", volName, ctrVolPath), imgName, "sh"})
-		ctrCreate.WaitWithDefaultTimeout()
-		Expect(ctrCreate).To(ExitCleanly())
+		_ = podmanTest.PodmanExitCleanly("cp", srcFile.Name(), fmt.Sprintf("%s:%s", ctrName, ctrVolPath))
 
-		cp := podmanTest.Podman([]string{"cp", srcFile.Name(), fmt.Sprintf("%s:%s", ctrName, ctrVolPath)})
-		cp.WaitWithDefaultTimeout()
-		Expect(cp).To(ExitCleanly())
-
-		ls := podmanTest.Podman([]string{"run", "-v", fmt.Sprintf("%s:%s", volName, ctrVolPath), ALPINE, "ls", "-al", ctrVolPath})
-		ls.WaitWithDefaultTimeout()
-		Expect(ls).To(ExitCleanly())
+		ls := podmanTest.PodmanExitCleanly("run", "-v", fmt.Sprintf("%s:%s", volName, ctrVolPath), ALPINE, "ls", "-al", ctrVolPath)
 		Expect(ls.OutputToString()).To(ContainSubstring("9999 9999"))
 		Expect(ls.OutputToString()).To(ContainSubstring(filepath.Base(srcFile.Name())))
+
+		// Test for #25585
+		_ = podmanTest.PodmanExitCleanly("rm", ctrName)
+		_ = podmanTest.PodmanExitCleanly("create", "--name", ctrName, "-v", fmt.Sprintf("%s:%s", volName, ctrVolPath), imgName, "sh")
+		_ = podmanTest.PodmanExitCleanly("cp", srcFile.Name(), fmt.Sprintf("%s:%sfile2", ctrName, ctrVolPath))
 	})
 })