From 894b237e0ef0909536934f67a993e6a00f36c849 Mon Sep 17 00:00:00 2001
From: Zeyad Yasser <zeyad.gouda@canonical.com>
Date: Tue, 8 Oct 2024 16:20:04 +0300
Subject: [PATCH] o/snapstate: make kill-snap-apps best effort (#14580)

This avoids a maliciously crafted snap from some how causing the
termination logic to fail (even though we should be able to handle
most scenarios even fork-bombs) and hence causing the snap to never
be removed.

Signed-off-by: Zeyad Gouda <zeyad.gouda@canonical.com>
---
 overlord/snapstate/handlers.go           |  8 +++-
 overlord/snapstate/handlers_link_test.go | 54 ++++++++++++++++++++++--
 2 files changed, 58 insertions(+), 4 deletions(-)

diff --git a/overlord/snapstate/handlers.go b/overlord/snapstate/handlers.go
index 94a9a800f86..7d81da7ce82 100644
--- a/overlord/snapstate/handlers.go
+++ b/overlord/snapstate/handlers.go
@@ -3410,7 +3410,13 @@ func (m *SnapManager) doKillSnapApps(t *state.Task, _ *tomb.Tomb) (err error) {
 	defer st.Lock()
 
 	if err := m.backend.KillSnapApps(snapName, reason, perfTimings); err != nil {
-		return err
+		// Snap processes termination is best-effort and task should continue
+		// without returning an error. This is to avoid a maliciously crafted snap
+		// from causing remove changes to always fail causing the snap to never be
+		// removed.
+		st.Lock()
+		st.Warnf("cannot terminate running app processes for %q: %v", snapName, err)
+		st.Unlock()
 	}
 
 	currentInfo, err := snapst.CurrentInfo()
diff --git a/overlord/snapstate/handlers_link_test.go b/overlord/snapstate/handlers_link_test.go
index 8d5ad2128c2..4a1361de39f 100644
--- a/overlord/snapstate/handlers_link_test.go
+++ b/overlord/snapstate/handlers_link_test.go
@@ -2718,6 +2718,50 @@ func (s *linkSnapSuite) TestDoKillSnapAppsUnlocksOnError(c *C) {
 		Active:   true,
 	})
 
+	snapstate.MockSnapReadInfo(func(name string, si *snap.SideInfo) (*snap.Info, error) {
+		return nil, fmt.Errorf("boom!")
+	})
+
+	task := s.state.NewTask("kill-snap-apps", "")
+	task.Set("kill-reason", snap.KillReasonRemove)
+	task.Set("snap-setup", &snapstate.SnapSetup{SideInfo: si})
+	chg := s.state.NewChange("test", "")
+	chg.AddTask(task)
+
+	s.state.Unlock()
+
+	s.se.Ensure()
+	s.se.Wait()
+
+	s.state.Lock()
+
+	c.Assert(task.Status(), Equals, state.ErrorStatus)
+
+	hint, _, err := runinhibit.IsLocked("some-snap")
+	c.Assert(err, IsNil)
+	// On error hint inhibition file is unlocked
+	c.Check(hint, Equals, runinhibit.HintNotInhibited)
+	// And snap lock is also unlocked
+	testLock, err := snaplock.OpenLock("some-snap")
+	c.Assert(err, IsNil)
+	c.Check(testLock.TryLock(), IsNil)
+	testLock.Close()
+}
+
+func (s *linkSnapSuite) TestDoKillSnapAppsTerminateBestEffort(c *C) {
+	s.state.Lock()
+	defer s.state.Unlock()
+
+	si := &snap.SideInfo{
+		RealName: "some-snap",
+		Revision: snap.R(1),
+	}
+	snapstate.Set(s.state, "some-snap", &snapstate.SnapState{
+		Sequence: snapstatetest.NewSequenceFromSnapSideInfos([]*snap.SideInfo{si}),
+		Current:  si.Revision,
+		Active:   true,
+	})
+
 	s.fakeBackend.maybeInjectErr = func(op *fakeOp) error {
 		if op.op == "kill-snap-apps:remove" {
 			return fmt.Errorf("boom!")
@@ -2738,17 +2782,21 @@ func (s *linkSnapSuite) TestDoKillSnapAppsUnlocksOnError(c *C) {
 
 	s.state.Lock()
 
-	c.Assert(task.Status(), Equals, state.ErrorStatus)
+	c.Assert(task.Status(), Equals, state.DoneStatus)
 
 	hint, _, err := runinhibit.IsLocked("some-snap")
 	c.Assert(err, IsNil)
-	// On error hint inhibition file is unlocked
-	c.Check(hint, Equals, runinhibit.HintNotInhibited)
+	// Error is ignored, inhibition lock is held
+	c.Check(hint, Equals, runinhibit.HintInhibitedForRemove)
 	// And snap lock is also unlocked
 	testLock, err := snaplock.OpenLock("some-snap")
 	c.Assert(err, IsNil)
 	c.Check(testLock.TryLock(), IsNil)
 	testLock.Close()
+	// But a warning is emitted
+	warnings := s.state.AllWarnings()
+	c.Assert(warnings, HasLen, 1)
+	c.Assert(warnings[0].String(), Equals, `cannot terminate running app processes for "some-snap": boom!`)
 }
 
 func (s *linkSnapSuite) testDoUndoKillSnapApps(c *C, svc bool) {