readpassword: bubble up errors instead of exiting the process

This allows cleanups to happen in the caller, like removing
the control socket.

Fixes https://github.com/rfjakob/gocryptfs/issues/634

5 files changed, 143 insertions, 142 deletions

diff --git a/internal/readpassword/extpass_test.go b/internal/readpassword/extpass_test.go
index 5c23ab3..d38f42f 100644
--- a/internal/readpassword/extpass_test.go
+++ b/internal/readpassword/extpass_test.go
@@ -2,7 +2,6 @@ package readpassword
 
 import (
 	"os"
-	"os/exec"
 	"testing"
 
 	"github.com/rfjakob/gocryptfs/v2/internal/tlog"
@@ -16,68 +15,76 @@ func TestMain(m *testing.M) {
 
 func TestExtpass(t *testing.T) {
 	p1 := "ads2q4tw41reg52"
-	p2 := string(readPasswordExtpass([]string{"echo " + p1}))
-	if p1 != p2 {
-		t.Errorf("p1=%q != p2=%q", p1, p2)
+	p2, err := readPasswordExtpass([]string{"echo " + p1})
+	if err != nil {
+		t.Fatal(err)
+	}
+	if p1 != string(p2) {
+		t.Errorf("p1=%q != p2=%q", p1, string(p2))
 	}
 }
 
 func TestOnceExtpass(t *testing.T) {
 	p1 := "lkadsf0923rdfi48rqwhdsf"
-	p2 := string(Once([]string{"echo " + p1}, nil, ""))
-	if p1 != p2 {
-		t.Errorf("p1=%q != p2=%q", p1, p2)
+	p2, err := Once([]string{"echo " + p1}, nil, "")
+	if err != nil {
+		t.Fatal(err)
+	}
+	if p1 != string(p2) {
+		t.Errorf("p1=%q != p2=%q", p1, string(p2))
 	}
 }
 
 // extpass with two arguments
 func TestOnceExtpass2(t *testing.T) {
 	p1 := "foo"
-	p2 := string(Once([]string{"echo", p1}, nil, ""))
-	if p1 != p2 {
-		t.Errorf("p1=%q != p2=%q", p1, p2)
+	p2, err := Once([]string{"echo", p1}, nil, "")
+	if err != nil {
+		t.Fatal(err)
+	}
+	if p1 != string(p2) {
+		t.Errorf("p1=%q != p2=%q", p1, string(p2))
 	}
 }
 
 // extpass with three arguments
 func TestOnceExtpass3(t *testing.T) {
 	p1 := "foo bar baz"
-	p2 := string(Once([]string{"echo", "foo", "bar", "baz"}, nil, ""))
-	if p1 != p2 {
-		t.Errorf("p1=%q != p2=%q", p1, p2)
+	p2, err := Once([]string{"echo", "foo", "bar", "baz"}, nil, "")
+	if err != nil {
+		t.Fatal(err)
+	}
+	if p1 != string(p2) {
+		t.Errorf("p1=%q != p2=%q", p1, string(p2))
 	}
 }
 
 func TestOnceExtpassSpaces(t *testing.T) {
 	p1 := "mypassword"
-	p2 := string(Once([]string{"cat", "passfile_test_files/file with spaces.txt"}, nil, ""))
-	if p1 != p2 {
-		t.Errorf("p1=%q != p2=%q", p1, p2)
+	p2, err := Once([]string{"cat", "passfile_test_files/file with spaces.txt"}, nil, "")
+	if err != nil {
+		t.Fatal(err)
+	}
+	if p1 != string(p2) {
+		t.Errorf("p1=%q != p2=%q", p1, string(p2))
 	}
 }
 
 func TestTwiceExtpass(t *testing.T) {
 	p1 := "w5w44t3wfe45srz434"
-	p2 := string(Once([]string{"echo " + p1}, nil, ""))
-	if p1 != p2 {
-		t.Errorf("p1=%q != p2=%q", p1, p2)
+	p2, err := Once([]string{"echo " + p1}, nil, "")
+	if err != nil {
+		t.Fatal(err)
+	}
+	if p1 != string(p2) {
+		t.Errorf("p1=%q != p2=%q", p1, string(p2))
 	}
 }
 
-// When extpass returns an empty string, we should crash.
-//
-// The TEST_SLAVE magic is explained at
-// https://talks.golang.org/2014/testing.slide#23 .
+// Empty extpass should fail
 func TestExtpassEmpty(t *testing.T) {
-	if os.Getenv("TEST_SLAVE") == "1" {
-		readPasswordExtpass([]string{"echo"})
-		return
-	}
-	cmd := exec.Command(os.Args[0], "-test.run=TestExtpassEmpty$")
-	cmd.Env = append(os.Environ(), "TEST_SLAVE=1")
-	err := cmd.Run()
-	if err != nil {
-		return
+	_, err := readPasswordExtpass([]string{"echo"})
+	if err == nil {
+		t.Fatal("empty password should have failed")
 	}
-	t.Fatal("empty password should have failed")
 }
diff --git a/internal/readpassword/passfile.go b/internal/readpassword/passfile.go
index 29fde6c..60902b7 100644
--- a/internal/readpassword/passfile.go
+++ b/internal/readpassword/passfile.go
@@ -2,28 +2,31 @@ package readpassword
 
 import (
 	"bytes"
+	"fmt"
 	"os"
 
-	"github.com/rfjakob/gocryptfs/v2/internal/exitcodes"
 	"github.com/rfjakob/gocryptfs/v2/internal/tlog"
 )
 
 // readPassFileConcatenate reads the first line from each file name and
 // concatenates the results. The result does not contain any newlines.
-func readPassFileConcatenate(passfileSlice []string) (result []byte) {
+func readPassFileConcatenate(passfileSlice []string) (result []byte, err error) {
 	for _, e := range passfileSlice {
-		result = append(result, readPassFile(e)...)
+		add, err := readPassFile(e)
+		if err != nil {
+			return nil, err
+		}
+		result = append(result, add...)
 	}
-	return result
+	return result, nil
 }
 
 // readPassFile reads the first line from the passed file name.
-func readPassFile(passfile string) []byte {
+func readPassFile(passfile string) ([]byte, error) {
 	tlog.Info.Printf("passfile: reading from file %q", passfile)
 	f, err := os.Open(passfile)
 	if err != nil {
-		tlog.Fatal.Printf("fatal: passfile: could not open %q: %v", passfile, err)
-		os.Exit(exitcodes.ReadPassword)
+		return nil, fmt.Errorf("fatal: passfile: could not open %q: %v", passfile, err)
 	}
 	defer f.Close()
 	// +1 for an optional trailing newline,
@@ -31,23 +34,20 @@ func readPassFile(passfile string) []byte {
 	buf := make([]byte, maxPasswordLen+2)
 	n, err := f.Read(buf)
 	if err != nil {
-		tlog.Fatal.Printf("fatal: passfile: could not read from %q: %v", passfile, err)
-		os.Exit(exitcodes.ReadPassword)
+		return nil, fmt.Errorf("fatal: passfile: could not read from %q: %v", passfile, err)
 	}
 	buf = buf[:n]
 	// Split into first line and "trailing garbage"
 	lines := bytes.SplitN(buf, []byte("\n"), 2)
 	if len(lines[0]) == 0 {
-		tlog.Fatal.Printf("fatal: passfile: empty first line in %q", passfile)
-		os.Exit(exitcodes.ReadPassword)
+		return nil, fmt.Errorf("fatal: passfile: empty first line in %q", passfile)
 	}
 	if len(lines[0]) > maxPasswordLen {
-		tlog.Fatal.Printf("fatal: passfile: max password length (%d bytes) exceeded", maxPasswordLen)
-		os.Exit(exitcodes.ReadPassword)
+		return nil, fmt.Errorf("fatal: passfile: max password length (%d bytes) exceeded", maxPasswordLen)
 	}
 	if len(lines) > 1 && len(lines[1]) > 0 {
 		tlog.Warn.Printf("warning: passfile: ignoring trailing garbage (%d bytes) after first line",
 			len(lines[1]))
 	}
-	return lines[0]
+	return lines[0], nil
 }
diff --git a/internal/readpassword/passfile_test.go b/internal/readpassword/passfile_test.go
index dbfe159..425e4e5 100644
--- a/internal/readpassword/passfile_test.go
+++ b/internal/readpassword/passfile_test.go
@@ -1,8 +1,6 @@
 package readpassword
 
 import (
-	"os"
-	"os/exec"
 	"testing"
 )
 
@@ -17,76 +15,49 @@ func TestPassfile(t *testing.T) {
 		{"file with spaces.txt", "mypassword"},
 	}
 	for _, tc := range testcases {
-		pw := readPassFile("passfile_test_files/" + tc.file)
+		pw, err := readPassFile("passfile_test_files/" + tc.file)
+		if err != nil {
+			t.Fatal(err)
+		}
 		if string(pw) != tc.want {
 			t.Errorf("Wrong result: want=%q have=%q", tc.want, pw)
 		}
 		// Calling readPassFileConcatenate with only one element should give the
 		// same result
-		pw = readPassFileConcatenate([]string{"passfile_test_files/" + tc.file})
+		pw, err = readPassFileConcatenate([]string{"passfile_test_files/" + tc.file})
+		if err != nil {
+			t.Fatal(err)
+		}
 		if string(pw) != tc.want {
 			t.Errorf("Wrong result: want=%q have=%q", tc.want, pw)
 		}
 	}
 }
 
-// readPassFile() should exit instead of returning an empty string.
-//
-// The TEST_SLAVE magic is explained at
-// https://talks.golang.org/2014/testing.slide#23 , mirror:
-// http://web.archive.org/web/20200426174352/https://talks.golang.org/2014/testing.slide#23
+// readPassFile() should fail instead of returning an empty string.
 func TestPassfileEmpty(t *testing.T) {
-	if os.Getenv("TEST_SLAVE") == "1" {
-		readPassFile("passfile_test_files/empty.txt")
-		return
+	_, err := readPassFile("passfile_test_files/empty.txt")
+	if err == nil {
+		t.Fatal("should have failed")
 	}
-	cmd := exec.Command(os.Args[0], "-test.run=TestPassfileEmpty$")
-	cmd.Env = append(os.Environ(), "TEST_SLAVE=1")
-	err := cmd.Run()
-	if err != nil {
-		return
-	}
-	t.Fatal("should have exited")
 }
 
 // File containing just a newline.
-// readPassFile() should exit instead of returning an empty string.
-//
-// The TEST_SLAVE magic is explained at
-// https://talks.golang.org/2014/testing.slide#23 , mirror:
-// http://web.archive.org/web/20200426174352/https://talks.golang.org/2014/testing.slide#23
+// readPassFile() should fal instead of returning an empty string.
 func TestPassfileNewline(t *testing.T) {
-	if os.Getenv("TEST_SLAVE") == "1" {
-		readPassFile("passfile_test_files/newline.txt")
-		return
-	}
-	cmd := exec.Command(os.Args[0], "-test.run=TestPassfileNewline$")
-	cmd.Env = append(os.Environ(), "TEST_SLAVE=1")
-	err := cmd.Run()
-	if err != nil {
-		return
+	_, err := readPassFile("passfile_test_files/newline.txt")
+	if err == nil {
+		t.Fatal("should have failed")
 	}
-	t.Fatal("should have exited")
 }
 
 // File containing "\ngarbage".
-// readPassFile() should exit instead of returning an empty string.
-//
-// The TEST_SLAVE magic is explained at
-// https://talks.golang.org/2014/testing.slide#23 , mirror:
-// http://web.archive.org/web/20200426174352/https://talks.golang.org/2014/testing.slide#23
+// readPassFile() should return an error.
 func TestPassfileEmptyFirstLine(t *testing.T) {
-	if os.Getenv("TEST_SLAVE") == "1" {
-		readPassFile("passfile_test_files/empty_first_line.txt")
-		return
-	}
-	cmd := exec.Command(os.Args[0], "-test.run=TestPassfileEmptyFirstLine$")
-	cmd.Env = append(os.Environ(), "TEST_SLAVE=1")
-	err := cmd.Run()
-	if err != nil {
-		return
+	_, err := readPassFile("passfile_test_files/empty_first_line.txt")
+	if err == nil {
+		t.Fatal("should have failed")
 	}
-	t.Fatal("should have exited")
 }
 
 // TestPassFileConcatenate tests readPassFileConcatenate
@@ -95,8 +66,11 @@ func TestPassFileConcatenate(t *testing.T) {
 		"passfile_test_files/file with spaces.txt",
 		"passfile_test_files/mypassword_garbage.txt",
 	}
-	res := string(readPassFileConcatenate(files))
-	if res != "mypasswordmypassword" {
+	res, err := readPassFileConcatenate(files)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if string(res) != "mypasswordmypassword" {
 		t.Errorf("wrong result: %q", res)
 	}
 }
diff --git a/internal/readpassword/read.go b/internal/readpassword/read.go
index c0dce43..498d09b 100644
--- a/internal/readpassword/read.go
+++ b/internal/readpassword/read.go
@@ -11,7 +11,6 @@ import (
 
 	"golang.org/x/crypto/ssh/terminal"
 
-	"github.com/rfjakob/gocryptfs/v2/internal/exitcodes"
 	"github.com/rfjakob/gocryptfs/v2/internal/tlog"
 )
 
@@ -22,7 +21,7 @@ const (
 
 // Once tries to get a password from the user, either from the terminal, extpass, passfile
 // or stdin. Leave "prompt" empty to use the default "Password: " prompt.
-func Once(extpass []string, passfile []string, prompt string) []byte {
+func Once(extpass []string, passfile []string, prompt string) ([]byte, error) {
 	if len(passfile) != 0 {
 		return readPassFileConcatenate(passfile)
 	}
@@ -40,7 +39,7 @@ func Once(extpass []string, passfile []string, prompt string) []byte {
 
 // Twice is the same as Once but will prompt twice if we get the password from
 // the terminal.
-func Twice(extpass []string, passfile []string) []byte {
+func Twice(extpass []string, passfile []string) ([]byte, error) {
 	if len(passfile) != 0 {
 		return readPassFileConcatenate(passfile)
 	}
@@ -50,54 +49,59 @@ func Twice(extpass []string, passfile []string) []byte {
 	if !terminal.IsTerminal(int(os.Stdin.Fd())) {
 		return readPasswordStdin("Password")
 	}
-	p1 := readPasswordTerminal("Password: ")
-	p2 := readPasswordTerminal("Repeat: ")
+	p1, err := readPasswordTerminal("Password: ")
+	if err != nil {
+		return nil, err
+	}
+	p2, err := readPasswordTerminal("Repeat: ")
+	if err != nil {
+		return nil, err
+	}
 	if !bytes.Equal(p1, p2) {
-		tlog.Fatal.Println("Passwords do not match")
-		os.Exit(exitcodes.ReadPassword)
+		return nil, fmt.Errorf("Passwords do not match")
 	}
 	// Wipe the password duplicate from memory
 	for i := range p2 {
 		p2[i] = 0
 	}
-	return p1
+	return p1, nil
 }
 
 // readPasswordTerminal reads a line from the terminal.
 // Exits on read error or empty result.
-func readPasswordTerminal(prompt string) []byte {
+func readPasswordTerminal(prompt string) ([]byte, error) {
 	fd := int(os.Stdin.Fd())
 	fmt.Fprintf(os.Stderr, prompt)
 	// terminal.ReadPassword removes the trailing newline
 	p, err := terminal.ReadPassword(fd)
 	if err != nil {
-		tlog.Fatal.Printf("Could not read password from terminal: %v\n", err)
-		os.Exit(exitcodes.ReadPassword)
+		return nil, fmt.Errorf("Could not read password from terminal: %v\n", err)
 	}
 	fmt.Fprintf(os.Stderr, "\n")
 	if len(p) == 0 {
-		tlog.Fatal.Println("Password is empty")
-		os.Exit(exitcodes.PasswordEmpty)
+		return nil, fmt.Errorf("Password is empty")
 	}
-	return p
+	return p, nil
 }
 
 // readPasswordStdin reads a line from stdin.
 // It exits with a fatal error on read error or empty result.
-func readPasswordStdin(prompt string) []byte {
+func readPasswordStdin(prompt string) ([]byte, error) {
 	tlog.Info.Printf("Reading %s from stdin", prompt)
-	p := readLineUnbuffered(os.Stdin)
+	p, err := readLineUnbuffered(os.Stdin)
+	if err != nil {
+		return nil, err
+	}
 	if len(p) == 0 {
-		tlog.Fatal.Printf("Got empty %s from stdin", prompt)
-		os.Exit(exitcodes.ReadPassword)
+		return nil, fmt.Errorf("Got empty %s from stdin", prompt)
 	}
-	return p
+	return p, nil
 }
 
 // readPasswordExtpass executes the "extpass" program and returns the first line
 // of the output.
 // Exits on read error or empty result.
-func readPasswordExtpass(extpass []string) []byte {
+func readPasswordExtpass(extpass []string) ([]byte, error) {
 	var parts []string
 	if len(extpass) == 1 {
 		parts = strings.Split(extpass[0], " ")
@@ -109,50 +113,47 @@ func readPasswordExtpass(extpass []string) []byte {
 	cmd.Stderr = os.Stderr
 	pipe, err := cmd.StdoutPipe()
 	if err != nil {
-		tlog.Fatal.Printf("extpass pipe setup failed: %v", err)
-		os.Exit(exitcodes.ReadPassword)
+		return nil, fmt.Errorf("extpass pipe setup failed: %v", err)
 	}
 	err = cmd.Start()
 	if err != nil {
-		tlog.Fatal.Printf("extpass cmd start failed: %v", err)
-		os.Exit(exitcodes.ReadPassword)
+		return nil, fmt.Errorf("extpass cmd start failed: %v", err)
+	}
+	p, err := readLineUnbuffered(pipe)
+	if err != nil {
+		return nil, err
 	}
-	p := readLineUnbuffered(pipe)
 	pipe.Close()
 	err = cmd.Wait()
 	if err != nil {
-		tlog.Fatal.Printf("extpass program returned an error: %v", err)
-		os.Exit(exitcodes.ReadPassword)
+		return nil, fmt.Errorf("extpass program returned an error: %v", err)
 	}
 	if len(p) == 0 {
-		tlog.Fatal.Println("extpass: password is empty")
-		os.Exit(exitcodes.ReadPassword)
+		return nil, fmt.Errorf("extpass: password is empty")
 	}
-	return p
+	return p, nil
 }
 
 // readLineUnbuffered reads single bytes from "r" util it gets "\n" or EOF.
 // The returned string does NOT contain the trailing "\n".
-func readLineUnbuffered(r io.Reader) (l []byte) {
+func readLineUnbuffered(r io.Reader) (l []byte, err error) {
 	b := make([]byte, 1)
 	for {
 		if len(l) > maxPasswordLen {
-			tlog.Fatal.Printf("fatal: maximum password length of %d bytes exceeded", maxPasswordLen)
-			os.Exit(exitcodes.ReadPassword)
+			return nil, fmt.Errorf("fatal: maximum password length of %d bytes exceeded", maxPasswordLen)
 		}
 		n, err := r.Read(b)
 		if err == io.EOF {
-			return l
+			return l, nil
 		}
 		if err != nil {
-			tlog.Fatal.Printf("readLineUnbuffered: %v", err)
-			os.Exit(exitcodes.ReadPassword)
+			return nil, fmt.Errorf("readLineUnbuffered: %v", err)
 		}
 		if n == 0 {
 			continue
 		}
 		if b[0] == '\n' {
-			return l
+			return l, nil
 		}
 		l = append(l, b...)
 	}
diff --git a/internal/readpassword/stdin_test.go b/internal/readpassword/stdin_test.go
index 01dd701..5d5d846 100644
--- a/internal/readpassword/stdin_test.go
+++ b/internal/readpassword/stdin_test.go
@@ -8,12 +8,20 @@ import (
 )
 
 // Provide password via stdin, terminated by "\n".
+//
+// The TEST_SLAVE magic is explained at
+// https://talks.golang.org/2014/testing.slide#23 , mirror:
+// http://web.archive.org/web/20200426174352/https://talks.golang.org/2014/testing.slide#23
 func TestStdin(t *testing.T) {
 	p1 := "g55434t55wef"
 	if os.Getenv("TEST_SLAVE") == "1" {
-		p2 := string(readPasswordStdin("foo"))
-		if p1 != p2 {
-			fmt.Fprintf(os.Stderr, "%q != %q", p1, p2)
+		p2, err := readPasswordStdin("foo")
+		if err != nil {
+			fmt.Fprint(os.Stderr, err)
+			os.Exit(1)
+		}
+		if p1 != string(p2) {
+			fmt.Fprintf(os.Stderr, "%q != %q", p1, string(p2))
 			os.Exit(1)
 		}
 		return
@@ -41,12 +49,20 @@ func TestStdin(t *testing.T) {
 
 // Provide password via stdin, terminated by EOF (pipe close). This should not
 // hang.
+//
+// The TEST_SLAVE magic is explained at
+// https://talks.golang.org/2014/testing.slide#23 , mirror:
+// http://web.archive.org/web/20200426174352/https://talks.golang.org/2014/testing.slide#23
 func TestStdinEof(t *testing.T) {
 	p1 := "asd45as5f4a36"
 	if os.Getenv("TEST_SLAVE") == "1" {
-		p2 := string(readPasswordStdin("foo"))
-		if p1 != p2 {
-			fmt.Fprintf(os.Stderr, "%q != %q", p1, p2)
+		p2, err := readPasswordStdin("foo")
+		if err != nil {
+			fmt.Fprint(os.Stderr, err)
+			os.Exit(1)
+		}
+		if p1 != string(p2) {
+			fmt.Fprintf(os.Stderr, "%q != %q", p1, string(p2))
 			os.Exit(1)
 		}
 		return
@@ -76,7 +92,10 @@ func TestStdinEof(t *testing.T) {
 // Provide empty password via stdin
 func TestStdinEmpty(t *testing.T) {
 	if os.Getenv("TEST_SLAVE") == "1" {
-		readPasswordStdin("foo")
+		_, err := readPasswordStdin("foo")
+		if err != nil {
+			os.Exit(1)
+		}
 	}
 	cmd := exec.Command(os.Args[0], "-test.run=TestStdinEmpty$")
 	cmd.Env = append(os.Environ(), "TEST_SLAVE=1")