crypto/tls: replace net.Pipe in tests with real TCP connections

crypto/tls is meant to work over network connections with buffering, not
synchronous connections, as explained in #24198. Tests based on net.Pipe
are unrealistic as reads and writes are matched one to one. Such tests
worked just thanks to the implementation details of the tls.Conn
internal buffering, and would break if for example the flush of the
first flight of the server was not entirely assimilated by the client
rawInput buffer before the client attempted to reply to the ServerHello.

Note that this might run into the Darwin network issues at #25696.

Fixed a few test races that were either hidden or synchronized by the
use of the in-memory net.Pipe.

Also, this gets us slightly more realistic benchmarks, reflecting some
syscall cost of Read and Write operations.

Change-Id: I5a597b3d7a81b8ccc776030cc837133412bf50f8
Reviewed-on: https://go-review.googlesource.com/c/142817
Run-TryBot: Filippo Valsorda <[email protected]>
TryBot-Result: Gobot Gobot <[email protected]>
Reviewed-by: Brad Fitzpatrick <[email protected]>

Author: FiloSottile

src/crypto/tls/conn_test.go

@@ -134,12 +134,13 @@ func TestCertificateSelection(t *testing.T) {
 
 // Run with multiple crypto configs to test the logic for computing TLS record overheads.
 func runDynamicRecordSizingTest(t *testing.T, config *Config) {
-	clientConn, serverConn := net.Pipe()
+	clientConn, serverConn := localPipe(t)
 
 	serverConfig := config.Clone()
 	serverConfig.DynamicRecordSizingDisabled = false
 	tlsConn := Server(serverConn, serverConfig)
 
+	handshakeDone := make(chan struct{})
 	recordSizesChan := make(chan []int, 1)
 	go func() {
 		// This goroutine performs a TLS handshake over clientConn and
@@ -153,6 +154,7 @@ func runDynamicRecordSizingTest(t *testing.T, config *Config) {
 			t.Errorf("Error from client handshake: %v", err)
 			return
 		}
+		close(handshakeDone)
 
 		var recordHeader [recordHeaderLen]byte
 		var record []byte
@@ -192,6 +194,7 @@ func runDynamicRecordSizingTest(t *testing.T, config *Config) {
 	if err := tlsConn.Handshake(); err != nil {
 		t.Fatalf("Error from server handshake: %s", err)
 	}
+	<-handshakeDone
 
 	// The server writes these plaintexts in order.
 	plaintext := bytes.Join([][]byte{
@@ -269,7 +272,7 @@ func (conn *hairpinConn) Close() error {
 func TestHairpinInClose(t *testing.T) {
 	// This tests that the underlying net.Conn can call back into the
 	// tls.Conn when being closed without deadlocking.
-	client, server := net.Pipe()
+	client, server := localPipe(t)
 	defer server.Close()
 	defer client.Close()
 

src/crypto/tls/handshake_client_test.go

@@ -179,7 +179,7 @@ func (test *clientTest) connFromCommand() (conn *recordingConn, child *exec.Cmd,
 	var pemOut bytes.Buffer
 	pem.Encode(&pemOut, &pem.Block{Type: pemType + " PRIVATE KEY", Bytes: derBytes})
 
-	keyPath := tempFile(string(pemOut.Bytes()))
+	keyPath := tempFile(pemOut.String())
 	defer os.Remove(keyPath)
 
 	var command []string
@@ -293,7 +293,7 @@ func (test *clientTest) run(t *testing.T, write bool) {
 		}
 		clientConn = recordingConn
 	} else {
-		clientConn, serverConn = net.Pipe()
+		clientConn, serverConn = localPipe(t)
 	}
 
 	config := test.config
@@ -682,7 +682,7 @@ func TestClientResumption(t *testing.T) {
 	}
 
 	testResumeState := func(test string, didResume bool) {
-		_, hs, err := testHandshake(clientConfig, serverConfig)
+		_, hs, err := testHandshake(t, clientConfig, serverConfig)
 		if err != nil {
 			t.Fatalf("%s: handshake failed: %s", test, err)
 		}
@@ -800,7 +800,7 @@ func TestKeyLog(t *testing.T) {
 	serverConfig := testConfig.Clone()
 	serverConfig.KeyLogWriter = &serverBuf
 
-	c, s := net.Pipe()
+	c, s := localPipe(t)
 	done := make(chan bool)
 
 	go func() {
@@ -838,8 +838,8 @@ func TestKeyLog(t *testing.T) {
 		}
 	}
 
-	checkKeylogLine("client", string(clientBuf.Bytes()))
-	checkKeylogLine("server", string(serverBuf.Bytes()))
+	checkKeylogLine("client", clientBuf.String())
+	checkKeylogLine("server", serverBuf.String())
 }
 
 func TestHandshakeClientALPNMatch(t *testing.T) {
@@ -1021,7 +1021,7 @@ var hostnameInSNITests = []struct {
 
 func TestHostnameInSNI(t *testing.T) {
 	for _, tt := range hostnameInSNITests {
-		c, s := net.Pipe()
+		c, s := localPipe(t)
 
 		go func(host string) {
 			Client(c, &Config{ServerName: host, InsecureSkipVerify: true}).Handshake()
@@ -1059,7 +1059,7 @@ func TestServerSelectingUnconfiguredCipherSuite(t *testing.T) {
 	// This checks that the server can't select a cipher suite that the
 	// client didn't offer. See #13174.
 
-	c, s := net.Pipe()
+	c, s := localPipe(t)
 	errChan := make(chan error, 1)
 
 	go func() {
@@ -1228,7 +1228,7 @@ func TestVerifyPeerCertificate(t *testing.T) {
 	}
 
 	for i, test := range tests {
-		c, s := net.Pipe()
+		c, s := localPipe(t)
 		done := make(chan error)
 
 		var clientCalled, serverCalled bool
@@ -1287,7 +1287,7 @@ func (b *brokenConn) Write(data []byte) (int, error) {
 func TestFailedWrite(t *testing.T) {
 	// Test that a write error during the handshake is returned.
 	for _, breakAfter := range []int{0, 1} {
-		c, s := net.Pipe()
+		c, s := localPipe(t)
 		done := make(chan bool)
 
 		go func() {
@@ -1321,7 +1321,7 @@ func (wcc *writeCountingConn) Write(data []byte) (int, error) {
 }
 
 func TestBuffering(t *testing.T) {
-	c, s := net.Pipe()
+	c, s := localPipe(t)
 	done := make(chan bool)
 
 	clientWCC := &writeCountingConn{Conn: c}
@@ -1350,7 +1350,7 @@ func TestBuffering(t *testing.T) {
 }
 
 func TestAlertFlushing(t *testing.T) {
-	c, s := net.Pipe()
+	c, s := localPipe(t)
 	done := make(chan bool)
 
 	clientWCC := &writeCountingConn{Conn: c}
@@ -1399,7 +1399,7 @@ func TestHandshakeRace(t *testing.T) {
 	// order to provide some evidence that there are no races or deadlocks
 	// in the handshake locking.
 	for i := 0; i < 32; i++ {
-		c, s := net.Pipe()
+		c, s := localPipe(t)
 
 		go func() {
 			server := Server(s, testConfig)
@@ -1430,7 +1430,7 @@ func TestHandshakeRace(t *testing.T) {
 		go func() {
 			<-startRead
 			var reply [1]byte
-			if n, err := client.Read(reply[:]); err != nil || n != 1 {
+			if _, err := io.ReadFull(client, reply[:]); err != nil {
 				panic(err)
 			}
 			c.Close()
@@ -1559,7 +1559,7 @@ func TestGetClientCertificate(t *testing.T) {
 			err error
 		}
 
-		c, s := net.Pipe()
+		c, s := localPipe(t)
 		done := make(chan serverResult)
 
 		go func() {
@@ -1637,7 +1637,7 @@ RwBA9Xk1KBNF
 }
 
 func TestCloseClientConnectionOnIdleServer(t *testing.T) {
-	clientConn, serverConn := net.Pipe()
+	clientConn, serverConn := localPipe(t)
 	client := Client(clientConn, testConfig.Clone())
 	go func() {
 		var b [1]byte
@@ -1647,8 +1647,8 @@ func TestCloseClientConnectionOnIdleServer(t *testing.T) {
 	client.SetWriteDeadline(time.Now().Add(time.Second))
 	err := client.Handshake()
 	if err != nil {
-		if !strings.Contains(err.Error(), "read/write on closed pipe") {
-			t.Errorf("Error expected containing 'read/write on closed pipe' but got '%s'", err.Error())
+		if err, ok := err.(net.Error); ok && err.Timeout() {
+			t.Errorf("Expected a closed network connection error but got '%s'", err.Error())
 		}
 	} else {
 		t.Errorf("Error expected, but no error returned")