From 123ba763ab6d76e3c9793321fded166ece2a2e07 Mon Sep 17 00:00:00 2001
From: Ryan Harper <rharper@woxford.com>
Date: Fri, 1 Dec 2023 13:21:22 -0800
Subject: [PATCH] fix: Tear down underlying atoms if atomfs mount fails (#565)

The atomfs bad verity test case ends up leaving verity and loop
devices which then break other test cases.  The root cause is that
when the test case attempts to mount the invalid atomfs the underlying
atom was already mounted.  Once the verity fails, stacker exits but
does not remove any previously mounted at atoms in the molecule.  Fix
this by creating a cleanup function which runs if we fail to mount an
atom.

- Add a check to the atomfs.bats which checks for leftover loop mounts

Fixes: #541

Signed-off-by: Ryan Harper <rharper@woxford.com>
---
 pkg/atomfs/molecule.go | 17 ++++++++++++++++-
 test/atomfs.bats       | 21 +++++++++++++++++++++
 2 files changed, 37 insertions(+), 1 deletion(-)

diff --git a/pkg/atomfs/molecule.go b/pkg/atomfs/molecule.go
index 47974813..cd484567 100644
--- a/pkg/atomfs/molecule.go
+++ b/pkg/atomfs/molecule.go
@@ -24,6 +24,19 @@ type Molecule struct {
 // mountUnderlyingAtoms mounts all the underlying atoms at
 // config.MountedAtomsPath().
 func (m Molecule) mountUnderlyingAtoms() error {
+	// in the case that we have a verity or other mount error we need to
+	// tear down the other underlying atoms so we don't leave verity and loop
+	// devices around unused.
+	atomsMounted := []string{}
+	cleanupAtoms := func(err error) error {
+		for _, target := range atomsMounted {
+			if umountErr := squashfs.Umount(target); umountErr != nil {
+				return errors.Wrapf(umountErr, "failed to unmount atom @ target %q while handling error: %s", target, err)
+			}
+		}
+		return err
+	}
+
 	for _, a := range m.Atoms {
 		target := m.config.MountedAtomsPath(a.Digest.Encoded())
 
@@ -58,8 +71,10 @@ func (m Molecule) mountUnderlyingAtoms() error {
 
 		err = squashfs.Mount(m.config.AtomsPath(a.Digest.Encoded()), target, rootHash)
 		if err != nil {
-			return errors.Wrapf(err, "couldn't mount")
+			return cleanupAtoms(err)
 		}
+
+		atomsMounted = append(atomsMounted, target)
 	}
 
 	return nil
diff --git a/test/atomfs.bats b/test/atomfs.bats
index 8215af97..22ac87d4 100644
--- a/test/atomfs.bats
+++ b/test/atomfs.bats
@@ -8,6 +8,23 @@ function teardown() {
     cleanup
 }
 
+function verity_checkusedloops() {
+    # search for loopdevices which have backing files with the current
+    # BATS_TEST_DIRNAME value and complain if they're present.
+    local usedloops="" found="" x=""
+    for ((x=0; x<5; x++)); do
+        usedloops=$(losetup -a | grep $BATS_TEST_DIRNAME || echo)
+        if [ -n "$usedloops" ]; then
+            found=1
+            udevadm settle
+        else
+            return 0
+        fi
+    done
+    echo "found used loops in testdir=$BATS_TEST_DIRNAME :$usedloops" >&3
+    [ $found = 1 ]
+}
+
 function basic_test() {
     require_privilege priv
     local verity_arg=$1
@@ -30,6 +47,7 @@ EOF
 
 @test "--no-squashfs-verity works" {
     basic_test --no-squashfs-verity
+    verity_checkusedloops
 }
 
 @test "mount + umount works" {
@@ -40,6 +58,7 @@ EOF
     last_layer_num=$(($(cat oci/blobs/sha256/$manifest | jq -r '.layers | length')-1))
     last_layer_hash=$(cat oci/blobs/sha256/$manifest | jq -r .layers[$last_layer].digest | cut -f2 -d:)
     [ ! -b "/dev/mapper/$last_layer_hash-verity" ]
+    verity_checkusedloops
 }
 
 @test "mount + umount + mount a tree of images works" {
@@ -113,6 +132,7 @@ EOF
     last_layer_num=$(($(cat oci/blobs/sha256/$manifest | jq -r '.layers | length')-1))
     last_layer_hash=$(cat oci/blobs/sha256/$manifest | jq -r .layers[$last_layer].digest | cut -f2 -d:)
     [ ! -b "/dev/mapper/$last_layer_hash-verity" ]
+    verity_checkusedloops
 }
 
 @test "bad existing verity device is rejected" {
@@ -140,4 +160,5 @@ EOF
     mkdir mountpoint
     bad_stacker internal-go atomfs mount test-squashfs mountpoint | grep "invalid root hash"
     veritysetup close "$devname"
+    verity_checkusedloops
 }