Fork Bomb Fixes

cmd/launcher/main.go

@@ -42,23 +42,30 @@ func main() {
 	// launcher is _also_ called when we're checking update
 	// validity (with autoupdate.checkExecutable). This is
 	// somewhat awkward as we end up with extra call layers.
-	newerBinary, err := autoupdate.FindNewestSelf(ctx)
-	if err != nil {
-		logutil.Fatal(logger, err, "checking for updated version")
-	}
+	//
+	// Allow a caller to set `LAUNCHER_SKIP_UPDATES` as a way to
+	// skip exec'ing an update. This helps prevent launcher from
+	// fork-bombing itself. This is an ENV, because there's no
+	// good way to pass it through the flags.
+	if !env.Bool("LAUNCHER_SKIP_UPDATES", false) {
+		newerBinary, err := autoupdate.FindNewestSelf(ctx)
+		if err != nil {
+			logutil.Fatal(logger, err, "checking for updated version")
+		}
 
-	if newerBinary != "" {
-		level.Debug(logger).Log(
-			"msg", "preparing to exec new binary",
-			"oldVersion", version.Version().Version,
-			"newBinary", newerBinary,
-		)
-		if err := execwrapper.Exec(ctx, newerBinary, os.Args, os.Environ()); err != nil {
-			logutil.Fatal(logger, err, "exec")
+		if newerBinary != "" {
+			level.Debug(logger).Log(
+				"msg", "preparing to exec new binary",
+				"oldVersion", version.Version().Version,
+				"newBinary", newerBinary,
+			)
+			if err := execwrapper.Exec(ctx, newerBinary, os.Args, os.Environ()); err != nil {
+				logutil.Fatal(logger, err, "exec")
+			}
+			panic("how")
+		} else {
+			level.Debug(logger).Log("msg", "Nothing new")
 		}
-		panic("how")
-	} else {
-		level.Info(logger).Log("msg", "Nothing new")
 	}
 
 	// if the launcher is being ran with a positional argument, handle that

go.mod

@@ -59,7 +59,7 @@ require (
 	github.com/onsi/gomega v1.4.3 // indirect
 	github.com/opencontainers/go-digest v1.0.0-rc1 // indirect
 	github.com/opencontainers/image-spec v1.0.1 // indirect
-	github.com/peterbourgon/ff v1.1.0
+	github.com/peterbourgon/ff v1.7.0
 	github.com/pkg/errors v0.8.1
 	github.com/prometheus/client_golang v0.9.2 // indirect
 	github.com/serenize/snaker v0.0.0-20171204205717-a683aaf2d516

go.sum

@@ -178,6 +178,7 @@ github.com/miekg/pkcs11 v0.0.0-20180208123018-5f6e0d0dad6f h1:8MAK/u+dE11/n8VIHQ
 github.com/miekg/pkcs11 v0.0.0-20180208123018-5f6e0d0dad6f/go.mod h1:WCBAbTOdfhHhz7YXujeZMF7owC4tPb1naKFsgfUISjo=
 github.com/mitchellh/go-ps v0.0.0-20170309133038-4fdf99ab2936 h1:kw1v0NlnN+GZcU8Ma8CLF2Zzgjfx95gs3/GN3vYAPpo=
 github.com/mitchellh/go-ps v0.0.0-20170309133038-4fdf99ab2936/go.mod h1:r1VsdOzOPt1ZSrGZWFoNhsAedKnEd6r9Np1+5blZCWk=
+github.com/mitchellh/go-wordwrap v1.0.0/go.mod h1:ZXFpozHsX6DPmq2I0TCekCxypsnAUbP2oI0UX1GXzOo=
 github.com/mitchellh/mapstructure v1.0.0 h1:vVpGvMXJPqSDh2VYHF7gsfQj8Ncx+Xw5Y1KHeTRY+7I=
 github.com/mitchellh/mapstructure v1.0.0/go.mod h1:FVVH3fgwuzCH5S8UJGiWEs2h04kUh9fWfEaFds41c1Y=
 github.com/mixer/clock v0.0.0-20170901150240-b08e6b4da7ea h1:eWwD4TJbXXLj8Ay2KQ1F3lh1YtgpcWZL3ZZN2sKvSDg=
@@ -200,8 +201,11 @@ github.com/opencontainers/image-spec v1.0.1 h1:JMemWkRwHx4Zj+fVxWoMCFm/8sYGGrUVo
 github.com/opencontainers/image-spec v1.0.1/go.mod h1:BtxoFyWECRxE4U/7sNtV5W15zMzWCbyJoFRP3s7yZA0=
 github.com/pelletier/go-toml v1.2.0 h1:T5zMGML61Wp+FlcbWjRDT7yAxhJNAiPPLOFECq181zc=
 github.com/pelletier/go-toml v1.2.0/go.mod h1:5z9KED0ma1S8pY6P1sdut58dfprrGBbd/94hg7ilaic=
+github.com/pelletier/go-toml v1.6.0/go.mod h1:5N711Q9dKgbdkxHL+MEfF31hpT7l0S0s/t2kKREewys=
 github.com/peterbourgon/ff v1.1.0 h1:mz0/c1gtUwz/MsyJs3GwPjFSTFlD4UEwKEPw5Z+0cm8=
 github.com/peterbourgon/ff v1.1.0/go.mod h1:P2CDTJbjip+iAWS2jnkp6uKBX2Bcvc/K4p5j47CTRv0=
+github.com/peterbourgon/ff v1.7.0 h1:hknvTgsh90jNBIjPq7xeq32Y9AmSbpXvjrFW4sJwW+A=
+github.com/peterbourgon/ff v1.7.0/go.mod h1:/KKxnU5cBj4w21jEMj4Rway/kslRP6XAOHh7CH8AyAM=
 github.com/pkg/errors v0.8.0 h1:WdK/asTD0HN+q6hsWO3/vpuAkAr+tw6aNJNDFFf0+qw=
 github.com/pkg/errors v0.8.0/go.mod h1:bwawxfHBFNV+L2hUp1rHADufV3IMtnDRdf1r5NINEl0=
 github.com/pkg/errors v0.8.1 h1:iURUrRGxPUNPdy5/HRSm+Yj6okJ6UtLINN0Q9M4+h3I=
@@ -324,6 +328,7 @@ golang.org/x/tools v0.0.0-20190506145303-2d16b83fe98c/go.mod h1:RgjU9mgBXZiqYHBn
 golang.org/x/tools v0.0.0-20190524140312-2c0ae7006135/go.mod h1:RgjU9mgBXZiqYHBnxXauZ1Gv1EHHAz9KjViQ78xBX0Q=
 golang.org/x/tools v0.0.0-20190606124116-d0a3d012864b/go.mod h1:/rFqwRUd4F7ZHNgwSSTFct+R/Kf4OFW1sUzUTQQTgfc=
 golang.org/x/tools v0.0.0-20190628153133-6cdbf07be9d0/go.mod h1:/rFqwRUd4F7ZHNgwSSTFct+R/Kf4OFW1sUzUTQQTgfc=
+golang.org/x/xerrors v0.0.0-20190717185122-a985d3407aa7/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0=
 google.golang.org/api v0.4.0/go.mod h1:8k5glujaEP+g9n7WNsDg8QP6cUVNI86fCNMcbazEtwE=
 google.golang.org/api v0.7.0 h1:9sdfJOzWlkqPltHAuzT2Cp+yrBeY1KRVYgms8soxMwM=
 google.golang.org/api v0.7.0/go.mod h1:WtwebWUNSVBH/HAw79HIFXZNqEvBhG+Ra+ax0hx3E3M=
@@ -364,6 +369,8 @@ gopkg.in/yaml.v2 v2.2.1 h1:mUhvW9EsL+naU5Q3cakzfE91YhliOondGd6ZrsDBHQE=
 gopkg.in/yaml.v2 v2.2.1/go.mod h1:hI93XBmqTisBFMUTm0b8Fm+jr3Dg1NNxqwp+5A1VGuI=
 gopkg.in/yaml.v2 v2.2.2 h1:ZCJp+EgiOT7lHqUV2J862kp8Qj64Jo6az82+3Td9dZw=
 gopkg.in/yaml.v2 v2.2.2/go.mod h1:hI93XBmqTisBFMUTm0b8Fm+jr3Dg1NNxqwp+5A1VGuI=
+gopkg.in/yaml.v2 v2.2.4 h1:/eiJrUcujPVeJ3xlSWaiNi3uSVmDGBK1pDHUHAnao1I=
+gopkg.in/yaml.v2 v2.2.4/go.mod h1:hI93XBmqTisBFMUTm0b8Fm+jr3Dg1NNxqwp+5A1VGuI=
 honnef.co/go/tools v0.0.0-20190102054323-c2f93a96b099/go.mod h1:rf3lG4BRIbNafJWhAfAdb/ePZxsR/4RtNHQocxwk9r4=
 honnef.co/go/tools v0.0.0-20190106161140-3f1c8253044a/go.mod h1:rf3lG4BRIbNafJWhAfAdb/ePZxsR/4RtNHQocxwk9r4=
 honnef.co/go/tools v0.0.0-20190418001031-e561f6794a2a/go.mod h1:rf3lG4BRIbNafJWhAfAdb/ePZxsR/4RtNHQocxwk9r4=

pkg/autoupdate/findnew.go

@@ -32,6 +32,7 @@ type newestSettings struct {
 	deleteCorrupt           bool
 	skipFullBinaryPathCheck bool
 	buildTimestamp          string
+	runningExecutable       string
 }
 
 type newestOption func(*newestSettings)
@@ -67,6 +68,15 @@ func SkipFullBinaryPathCheck() newestOption {
 	}
 }
 
+// withRunningExectuable sets the current executable. This is because
+// we never need to run an executable check against ourselves. (And
+// doing so will trigger a fork bomb)
+func withRunningExectuable(exe string) newestOption {
+	return func(no *newestSettings) {
+		no.runningExecutable = exe
+	}
+}
+
 // FindNewestSelf invokes `FindNewest` with the running binary path,
 // as determined by os.Executable. However, if the current running
 // version is the same as the newest on disk, it will return empty string.
@@ -82,7 +92,7 @@ func FindNewestSelf(ctx context.Context, opts ...newestOption) (string, error) {
 		return "", errors.New("can't find newest empty string")
 	}
 
-	opts = append(opts, SkipFullBinaryPathCheck())
+	opts = append(opts, SkipFullBinaryPathCheck(), withRunningExectuable(exPath))
 
 	newest := FindNewest(ctx, exPath, opts...)
 
@@ -183,18 +193,25 @@ func FindNewest(ctx context.Context, fullBinaryPath string, opts ...newestOption
 			}
 		}
 
-		// Sanity check that the executable is executable. Also remove the update if appropriate
-		if err := checkExecutable(ctx, file, "--version"); err != nil {
-			if newestSettings.deleteCorrupt {
-				level.Error(logger).Log("msg", "not executable. Removing", "binary", file, "reason", err)
-				if err := os.RemoveAll(basedir); err != nil {
-					level.Error(logger).Log("msg", "error deleting broken update dir", "dir", basedir, "err", err)
+		// If the file is _not_ the running executable, sanity
+		// check that executions work. If the exec fails,
+		// there's clearly an issue and we should remove it.
+		if newestSettings.runningExecutable != file {
+			if err := checkExecutable(ctx, file, "--version"); err != nil {
+				if newestSettings.deleteCorrupt {
+					level.Error(logger).Log("msg", "not executable. Removing", "binary", file, "reason", err)
+					if err := os.RemoveAll(basedir); err != nil {
+						level.Error(logger).Log("msg", "error deleting broken update dir", "dir", basedir, "err", err)
+					}
+				} else {
+					level.Error(logger).Log("msg", "not executable. Skipping", "binary", file, "reason", err)
 				}
-			} else {
-				level.Error(logger).Log("msg", "not executable. Skipping", "binary", file, "reason", err)
-			}
 
-			continue
+				continue
+			}
+		} else {
+			// This logging is mostly here to make test coverage of the conditional clear
+			level.Debug(logger).Log("msg", "Skipping checkExecutable against self", "file", file)
 		}
 
 		// We always want to increment the foundCount, since it's what triggers deletion.
@@ -268,10 +285,22 @@ func checkExecutable(ctx context.Context, potentialBinary string, args ...string
 		return err
 	}
 
+	// If we can determine that the requested executable is
+	// ourself, don't try to exec. It's needless, and a potential
+	// fork bomb. Ignore errors, either we get an answer or we don't.
+	selfPath, _ := os.Executable()
+	if filepath.Clean(selfPath) == filepath.Clean(potentialBinary) {
+		return nil
+	}
+
 	ctx, cancel := context.WithTimeout(ctx, 5*time.Second)
 	defer cancel()
 
 	cmd := exec.CommandContext(ctx, potentialBinary, args...)
+
+	// Set env, this should prevent launcher for fork-bombing
+	cmd.Env = append(cmd.Env, "LAUNCHER_SKIP_UPDATES=TRUE")
+
 	execErr := cmd.Run()
 
 	if ctx.Err() != nil {

pkg/autoupdate/findnew_test.go

@@ -47,12 +47,14 @@ func TestFindNewestSelf(t *testing.T) {
 		require.Empty(t, newest, "No correct binaries, should be empty")
 	}
 
-	expectedNewest := filepath.Join(updatesDir, "3", filepath.Base(binaryPath))
-
-	require.NoError(t, copyFile(expectedNewest, binaryPath, false), "copy executable")
-	require.NoError(t, os.Chmod(expectedNewest, 0755), "chmod")
+	for _, n := range []string{"2", "3"} {
+		updatedBinaryPath := filepath.Join(updatesDir, n, filepath.Base(binaryPath))
+		require.NoError(t, copyFile(updatedBinaryPath, binaryPath, false), "copy executable")
+		require.NoError(t, os.Chmod(updatedBinaryPath, 0755), "chmod")
+	}
 
 	{
+		expectedNewest := filepath.Join(updatesDir, "3", filepath.Base(binaryPath))
 		newest, err := FindNewestSelf(ctx)
 		require.NoError(t, err)
 		require.Equal(t, expectedNewest, newest, "Should find newer binary")
@@ -345,6 +347,13 @@ func copyFile(dstPath, srcPath string, truncate bool) error {
 
 func TestCheckExecutable(t *testing.T) {
 	t.Parallel()
+
+	// We need to run this from a temp dir, else the early return
+	// from matching os.Executable bypasses the point of this.
+	tmpDir, binaryName, cleanupFunc := setupTestDir(t, executableUpdates)
+	defer cleanupFunc()
+	targetExe := filepath.Join(tmpDir, binaryName)
+
 	var tests = []struct {
 		testName    string
 		expectedErr bool
@@ -369,9 +378,16 @@ func TestCheckExecutable(t *testing.T) {
 
 	for _, tt := range tests {
 		t.Run(tt.testName, func(t *testing.T) {
-			err := checkExecutable(context.TODO(), os.Args[0], "-test.run=TestHelperProcess", "--", tt.testName)
+			err := checkExecutable(context.TODO(), targetExe, "-test.run=TestHelperProcess", "--", tt.testName)
 			if tt.expectedErr {
 				require.Error(t, err, tt.testName)
+
+				// As a bonus, we can test that if we call os.Args[0],
+				// we still don't get an error. This is because we
+				// trigger the match against os.Executable and don't
+				// invoked. This is here, and not a dedicated test,
+				// because we ensure the same test arguments.
+				require.NoError(t, checkExecutable(context.TODO(), os.Args[0], "-test.run=TestHelperProcess", "--", tt.testName), "calling self with %s", tt.testName)
 			} else {
 				require.NoError(t, err, tt.testName)
 			}