crypto/tls: fix error handling in recorded test connections

Before, we'd drop both OpenSSL and client/server errors on the floor,
record the partial transcript including the error, and then replay them
again ignoring the error.

To actually catch the s_server error, we need it to exit cleanly after
one client connection, so add -naccept 1 to the arguments.

Change-Id: I6f40f5a9b3446b47ddf5e4d4c372e6e36a6a6964
Reviewed-on: https://go-review.googlesource.com/c/go/+/765922
LUCI-TryBot-Result: golang-scoped@luci-project-accounts.iam.gserviceaccount.com <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Carlos Amedee <carlos@golang.org>
Reviewed-by: Daniel McCarney <daniel@binaryparadox.net>
Reviewed-by: Michael Pratt <mpratt@google.com>
Auto-Submit: Filippo Valsorda <filippo@golang.org>

src/crypto/tls/handshake_client_test.go

@@ -163,7 +163,7 @@ type clientTest struct {
 	sendKeyUpdate bool
 }
 
-var serverCommand = []string{"openssl", "s_server", "-no_ticket", "-num_tickets", "0"}
+var serverCommand = []string{"openssl", "s_server", "-no_ticket", "-num_tickets", "0", "-naccept", "1"}
 
 // connFromCommand starts the reference server process, connects to it and
 // returns a recordingConn for the connection. The stdin return value is an
@@ -435,19 +435,24 @@ func (test *clientTest) run(t *testing.T, write bool) {
 
 	if write {
 		client.Close()
+		recordingConn.Close()
+		close(stdin)
+		if err := childProcess.Wait(); err != nil {
+			t.Errorf("OpenSSL exited with error: %s", err)
+		}
+		if t.Failed() {
+			t.Logf("OpenSSL output:\n\n%s", stdout.all)
+			return
+		}
+		if len(recordingConn.flows) < 3 {
+			t.Fatalf("Client connection didn't work")
+		}
 		path := test.dataPath()
 		out, err := os.OpenFile(path, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, 0644)
 		if err != nil {
 			t.Fatalf("Failed to create output file: %s", err)
 		}
 		defer out.Close()
-		recordingConn.Close()
-		close(stdin)
-		childProcess.Process.Kill()
-		childProcess.Wait()
-		if len(recordingConn.flows) < 3 {
-			t.Fatalf("Client connection didn't work")
-		}
 		recordingConn.WriteTo(out)
 		t.Logf("Wrote %s\n", path)
 	}

src/crypto/tls/handshake_server_test.go

@@ -609,13 +609,13 @@ var defaultClientCommand = []string{"openssl", "s_client", "-no_ticket"}
 // connFromCommand starts opens a listening socket and starts the reference
 // client to connect to it. It returns a recordingConn that wraps the resulting
 // connection.
-func (test *serverTest) connFromCommand() (conn *recordingConn, child *exec.Cmd, err error) {
+func (test *serverTest) connFromCommand() (conn *recordingConn, child *exec.Cmd, exit <-chan error, err error) {
 	l, err := net.ListenTCP("tcp", &net.TCPAddr{
 		IP:   net.IPv4(127, 0, 0, 1),
 		Port: 0,
 	})
 	if err != nil {
-		return nil, nil, err
+		return nil, nil, nil, err
 	}
 	defer l.Close()
 
@@ -634,9 +634,14 @@ func (test *serverTest) connFromCommand() (conn *recordingConn, child *exec.Cmd,
 	cmd.Stdout = &output
 	cmd.Stderr = &output
 	if err := cmd.Start(); err != nil {
-		return nil, nil, err
+		return nil, nil, nil, err
 	}
 
+	exitChan := make(chan error, 1)
+	go func() {
+		exitChan <- cmd.Wait()
+	}()
+
 	connChan := make(chan any, 1)
 	go func() {
 		tcpConn, err := l.Accept()
@@ -651,18 +656,21 @@ func (test *serverTest) connFromCommand() (conn *recordingConn, child *exec.Cmd,
 	select {
 	case connOrError := <-connChan:
 		if err, ok := connOrError.(error); ok {
-			return nil, nil, err
+			return nil, nil, nil, err
 		}
 		tcpConn = connOrError.(net.Conn)
+	case err := <-exitChan:
+		return nil, nil, nil, fmt.Errorf("child process exited before connecting: %v\n%s", err, output.String())
 	case <-time.After(2 * time.Second):
-		return nil, nil, errors.New("timed out waiting for connection from child process")
+		cmd.Process.Kill()
+		return nil, nil, nil, fmt.Errorf("timed out waiting for connection from child process\n%s", output.String())
 	}
 
 	record := &recordingConn{
 		Conn: tcpConn,
 	}
 
-	return record, cmd, nil
+	return record, cmd, exitChan, nil
 }
 
 func (test *serverTest) dataPath() string {
@@ -682,19 +690,15 @@ func (test *serverTest) run(t *testing.T, write bool) {
 	var serverConn net.Conn
 	var recordingConn *recordingConn
 	var childProcess *exec.Cmd
+	var childExit <-chan error
 
 	if write {
 		var err error
-		recordingConn, childProcess, err = test.connFromCommand()
+		recordingConn, childProcess, childExit, err = test.connFromCommand()
 		if err != nil {
 			t.Fatalf("Failed to start subcommand: %s", err)
 		}
 		serverConn = recordingConn
-		defer func() {
-			if t.Failed() {
-				t.Logf("OpenSSL output:\n\n%s", childProcess.Stdout)
-			}
-		}()
 	} else {
 		flows, err := test.loadData()
 		if err != nil {
@@ -717,7 +721,7 @@ func (test *serverTest) run(t *testing.T, write bool) {
 		}
 	} else {
 		if err != nil {
-			t.Logf("Error from Server.Write: '%s'", err)
+			t.Errorf("Error from Server.Write: '%s'", err)
 		}
 	}
 	server.Close()
@@ -743,21 +747,27 @@ func (test *serverTest) run(t *testing.T, write bool) {
 
 	if write {
 		serverConn.Close()
+		recordingConn.Close()
+		if err := <-childExit; err != nil && len(test.expectHandshakeErrorIncluding) == 0 {
+			t.Errorf("OpenSSL exited with error: %s", err)
+		}
+		if t.Failed() {
+			t.Logf("OpenSSL output:\n\n%s", childProcess.Stdout)
+			return
+		}
+		if len(recordingConn.flows) < 3 {
+			if len(test.expectHandshakeErrorIncluding) == 0 {
+				t.Fatalf("Handshake failed")
+			}
+		}
 		path := test.dataPath()
 		out, err := os.OpenFile(path, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, 0644)
 		if err != nil {
 			t.Fatalf("Failed to create output file: %s", err)
 		}
 		defer out.Close()
-		recordingConn.Close()
-		if len(recordingConn.flows) < 3 {
-			if len(test.expectHandshakeErrorIncluding) == 0 {
-				t.Fatalf("Handshake failed")
-			}
-		}
 		recordingConn.WriteTo(out)
 		t.Logf("Wrote %s\n", path)
-		childProcess.Wait()
 	}
 }