pkg/shell: Simplify the logic to find run directory.

Elvish uses a subdirectory under XDG_RUNTIME_DIR if the environment variable is
non-empty, but falls back to a subdirectory of tmpdir it it is unset or empty.

The tmpdir-based one used to be the only one. When support for XDG_RUNTIME_DIR
was added in 0.14.0, to ensure a smooth transition, Elvish would still use the
tmpdir-based one if it exists.

Enough time has passed since 0.14.0, and we can now simplify the run directory
to be solely determined by XDG_RUNTIME_DIR.

Also convert the test to a transcript test.

0.22.0-release-notes.md

@@ -5,3 +5,11 @@
 # Deprecations
 
 # Breaking changes
+
+-   If you are upgrading from a version older than 0.14.0, make sure that you
+    close all Elvish processes running the old version before starting any new
+    ones.
+
+    This is because the logic for determining how to connect to daemon has
+    changed and is no longer backward compatible with versions older than
+    0.14.0.

pkg/shell/paths_test.elvts

@@ -0,0 +1,60 @@
+/////////////////////////
+# run directory on Unix #
+/////////////////////////
+
+//only-on unix
+//each:in-temp-dir
+//each:secure-run-dir-in-global
+//each:unset-env XDG_RUNTIME_DIR
+//each:unset-env TMPDIR
+
+## uses XDG_RUNTIME_DIR if it's non-empty, tmpdir otherwise ##
+//uid-in-global
+~> use os
+   os:mkdir-all &perm=0o700 tmpdir/elvish-$uid
+   set E:TMPDIR = $pwd/tmpdir
+~> eq (secure-run-dir) $E:TMPDIR/elvish-$uid
+▶ $true
+~> set E:XDG_RUNTIME_DIR = ''
+   eq (secure-run-dir) $E:TMPDIR/elvish-$uid
+▶ $true
+~> set E:XDG_RUNTIME_DIR = $pwd/xdg_runtime_dir
+   os:mkdir-all &perm=0o700 xdg_runtime_dir/elvish
+   eq (secure-run-dir) $E:XDG_RUNTIME_DIR/elvish
+▶ $true
+
+// Tests below all use XDG_RUNTIME_DIR for simplicity.
+
+## creates run directory if it doesn't exist yet ##
+~> use os
+   set E:XDG_RUNTIME_DIR = $pwd/xdg_runtime_dir
+   os:mkdir &perm=0o700 xdg_runtime_dir
+~> eq (secure-run-dir) $E:XDG_RUNTIME_DIR/elvish
+▶ $true
+~> put (os:stat (secure-run-dir))[type]
+▶ dir
+
+## errors if run directory exists but has wrong permission ##
+//umask 0
+~> use os
+   os:mkdir-all &perm=0o777 xdg_runtime_dir/elvish
+   set E:XDG_RUNTIME_DIR = $pwd/xdg_runtime_dir
+~> use re
+   try { secure-run-dir } catch e {
+     re:match 'existing run directory .* is not secure' (to-string $e[reason])
+   }
+▶ $true
+
+## errors if unable to create run directory ##
+~> use os
+   os:mkdir-all &perm=0o000 xdg_runtime_dir
+   set E:XDG_RUNTIME_DIR = $pwd/xdg_runtime_dir
+~> use re
+   try { secure-run-dir } catch e {
+     re:match 'create new run directory: ' (to-string $e[reason])
+   }
+▶ $true
+
+// TODO: Test the remaining error conditions in secureRunDir. I'm not aware of a
+// way to make a real OS trigger those conditions, so testing them probably
+// requires faking os.MkdirAll.

pkg/shell/paths_unix.go

@@ -32,46 +32,45 @@ func homePath(suffix string) (string, error) {
 }
 
 // Returns a "run directory" for storing ephemeral files, which is guaranteed
-// to be only accessible to the current user.
+// to be only accessible to the current user. The path is one of the following:
 //
-// The path of the run directory is either $XDG_RUNTIME_DIR/elvish or
-// $tmpdir/elvish-$uid (where $tmpdir is the system temporary directory). The
-// former is used if the XDG_RUNTIME_DIR environment variable exists and the
-// latter directory does not exist.
+//   - $XDG_RUNTIME_DIR/elvish if $XDG_RUNTIME_DIR is non-empty.
+//   - $tmpdir/elvish-$uid otherwise.
 func secureRunDir() (string, error) {
-	runDirs := runDirCandidates()
-	for _, runDir := range runDirs {
-		if checkExclusiveAccess(runDir) {
-			return runDir, nil
+	runDir := runDirPath()
+	info, err := os.Stat(runDir)
+	if err == nil {
+		// Already exists; just check if it's secure.
+		if !secureAsRunDir(info) {
+			return "", fmt.Errorf("existing run directory %v is not secure", runDir)
 		}
+		return runDir, nil
+	}
+	// Create new run directory.
+	err = os.MkdirAll(runDir, 0700)
+	if err != nil {
+		return "", fmt.Errorf("create new run directory: %v", err)
 	}
-	runDir := runDirs[0]
-	err := os.MkdirAll(runDir, 0700)
+	// The OS may have set a different owner or permission bits, so still check
+	// if it's secure.
+	info, err = os.Stat(runDir)
 	if err != nil {
-		return "", fmt.Errorf("mkdir: %v", err)
+		return "", fmt.Errorf("stat newly created run directory: %v", err)
 	}
-	if !checkExclusiveAccess(runDir) {
-		return "", fmt.Errorf("cannot create %v as a secure run directory", runDir)
+	if !secureAsRunDir(info) {
+		return "", fmt.Errorf("newly created run directory %v is not secure", runDir)
 	}
 	return runDir, nil
 }
 
-// Returns one or more candidates for the run directory, in descending order of
-// preference.
-func runDirCandidates() []string {
-	tmpDirPath := filepath.Join(os.TempDir(), fmt.Sprintf("elvish-%d", os.Getuid()))
+func runDirPath() string {
 	if os.Getenv(env.XDG_RUNTIME_DIR) != "" {
-		xdgDirPath := filepath.Join(os.Getenv(env.XDG_RUNTIME_DIR), "elvish")
-		return []string{xdgDirPath, tmpDirPath}
+		return filepath.Join(os.Getenv(env.XDG_RUNTIME_DIR), "elvish")
 	}
-	return []string{tmpDirPath}
+	return filepath.Join(os.TempDir(), fmt.Sprintf("elvish-%d", os.Getuid()))
 }
 
-func checkExclusiveAccess(runDir string) bool {
-	info, err := os.Stat(runDir)
-	if err != nil {
-		return false
-	}
+func secureAsRunDir(info os.FileInfo) bool {
 	stat := info.Sys().(*syscall.Stat_t)
 	return info.IsDir() && int(stat.Uid) == os.Getuid() && stat.Mode&077 == 0
 }

pkg/shell/testexports_test.go

@@ -0,0 +1,3 @@
+package shell
+
+var SecureRunDir = secureRunDir

pkg/shell/transcripts_test.go

@@ -6,6 +6,7 @@ import (
 	"io"
 	"os"
 	"path/filepath"
+	"strconv"
 	"testing"
 	"time"
 
@@ -14,6 +15,7 @@ import (
 	"src.elv.sh/pkg/eval"
 	"src.elv.sh/pkg/eval/evaltest"
 	"src.elv.sh/pkg/eval/vars"
+	"src.elv.sh/pkg/must"
 	"src.elv.sh/pkg/prog/progtest"
 	"src.elv.sh/pkg/shell"
 	"src.elv.sh/pkg/testutil"
@@ -38,6 +40,11 @@ func TestTranscripts(t *testing.T) {
 		"kill-wait-in-global", addGlobal("kill-wait",
 			testutil.Scaled(10*time.Millisecond).String()),
 		"sigchld-name-in-global", addGlobal("sigchld-name", sigCHLDName),
+		"secure-run-dir-in-global", evaltest.GoFnInGlobal("secure-run-dir", shell.SecureRunDir),
+		"uid-in-global", addGlobal("uid", os.Getuid()),
+		"umask", func(t *testing.T, arg string) {
+			testutil.Umask(t, must.OK1(strconv.Atoi(arg)))
+		},
 		"in-temp-home", func(t *testing.T) { testutil.InTempHome(t) },
 	)
 }