From 6bbc91ae36a1efd78030341288f76d8d078d8968 Mon Sep 17 00:00:00 2001
From: Ben Hoyt <ben.hoyt@canonical.com>
Date: Fri, 19 Jul 2024 14:25:01 +1200
Subject: [PATCH] fix: actually turn off child subreaping in reaper.Stop (#454)

Previously we weren't actually turning off child subreaping at the OS
level in `reaper.Stop`, so it didn't reverse `reaper.Start`. Fix this,
and also simplify the error handling a little.

This almost certainly didn't cause problems, because as soon as the
Pebble process exits it doesn't matter. But it wouldn't have turned it
off in tests, and it's just the Right Thing To Do.

Fixes #412
---
 internals/reaper/reaper.go | 35 ++++++++++++++++++++---------------
 1 file changed, 20 insertions(+), 15 deletions(-)

diff --git a/internals/reaper/reaper.go b/internals/reaper/reaper.go
index fb1d7b66d..28b6f7819 100644
--- a/internals/reaper/reaper.go
+++ b/internals/reaper/reaper.go
@@ -16,6 +16,7 @@ package reaper
 
 import (
 	"bytes"
+	"errors"
 	"fmt"
 	"os"
 	"os/exec"
@@ -45,12 +46,9 @@ func Start() error {
 		return nil // already started
 	}
 
-	isSubreaper, err := setChildSubreaper()
+	err := setChildSubreaper(1)
 	if err != nil {
-		return fmt.Errorf("cannot set child subreaper: %w", err)
-	}
-	if !isSubreaper {
-		return fmt.Errorf("child subreaping unavailable on this platform")
+		return err
 	}
 
 	started = true
@@ -67,6 +65,12 @@ func Stop() error {
 	}
 	mutex.Unlock()
 
+	// Disable receiving of SIGCHLD before we stop the loop that handles them.
+	err := setChildSubreaper(0)
+	if err != nil {
+		return err
+	}
+
 	reaperTomb.Kill(nil)
 	reaperTomb.Wait()
 	reaperTomb = tomb.Tomb{}
@@ -78,19 +82,20 @@ func Stop() error {
 	return nil
 }
 
-// setChildSubreaper sets the current process as a "child subreaper" so we
-// become the parent of dead child processes rather than PID 1. This allows us
-// to wait for processes that are started by a Pebble service but then die, to
-// "reap" them (see https://unix.stackexchange.com/a/250156/73491).
+// setChildSubreaper sets the "child subreaper" attribute of the current
+// process, turning it on if the argument is nonzero, off otherwise.
 //
-// The function returns true if sub-reaping is available (Linux 3.4+) along
-// with an error if it's available but can't be set.
-func setChildSubreaper() (bool, error) {
-	err := unix.Prctl(unix.PR_SET_CHILD_SUBREAPER, 1, 0, 0, 0)
+// If turning it on, we become the parent of dead child processes rather than
+// PID 1. This allows us to wait for processes that are started by a Pebble
+// service but then die, to "reap" them (see
+// https://unix.stackexchange.com/a/250156/73491).
+func setChildSubreaper(set int) error {
+	err := unix.Prctl(unix.PR_SET_CHILD_SUBREAPER, uintptr(set), 0, 0, 0)
 	if err == unix.EINVAL {
-		return false, nil
+		// Not available in kernels before Linux 3.4.
+		return errors.New("child subreaping unavailable on this platform")
 	}
-	return true, err
+	return err
 }
 
 // reapChildren "reaps" (waits for) child processes whose parents didn't