Use TCP sockets rather than socketpairs in the SSL tests.

This involves more synchronization with child exits as the kernel no longer
closes the pre-created pipes for free, but it works on Windows. As long as
TCP_NODELAY is set, the performance seems comparable. Though it does involve
dealing with graceful socket shutdown. I couldn't get that to work on Windows
without draining the socket; not even SO_LINGER worked. Current (untested)
theory is that Windows refuses to gracefully shutdown a socket if the peer
sends data after we've stopped reading.

cmd.ExtraFiles doesn't work on Windows; it doesn't use fds natively, so you
can't pass fds 4 and 5. (stdin/stdout/stderr are special slots in
CreateProcess.) We can instead use the syscall module directly and mark handles
as inheritable (and then pass the numerical values out-of-band), but that
requires synchronizing all of our shim.Start() calls and assuming no other
thread is spawning a process.

PROC_THREAD_ATTRIBUTE_HANDLE_LIST fixes threading problems, but requires
wrapping more syscalls.  exec.Cmd also doesn't let us launch the process
ourselves. Plus it still requires every handle in the list be marked
inheritable, so it doesn't help if some other thread is launching a process
with bInheritHandles TRUE but NOT using PROC_THREAD_ATTRIBUTE_HANDLE_LIST.
(Like Go, though we can take syscall.ForkLock there.)

http://blogs.msdn.com/b/oldnewthing/archive/2011/12/16/10248328.aspx

The more natively Windows option seems to be named pipes, but that too requires
wrapping more system calls. (To be fair, that isn't too painful.) They also
involve a listening server, so we'd still have to synchronize with shim.Wait()
a la net.TCPListener.

Then there's DuplicateHandle, but then we need an out-of-band signal.

All in all, one cross-platform implementation with a TCP sockets seems
simplest.

Change-Id: I38233e309a0fa6814baf61e806732138902347c0
Reviewed-on: https://boringssl-review.googlesource.com/3563
Reviewed-by: Adam Langley <agl@google.com>

7 files changed, 190 insertions, 50 deletions

diff --git a/BUILDING b/BUILDING
index 4b2ba057..8b7d36d3 100644
--- a/BUILDING
+++ b/BUILDING
@@ -17,8 +17,7 @@ Build Prerequisites:
     with Platform SDK 8.1 or later are supported. Recent versions of GCC and
     Clang should work on non-Windows platforms, and maybe on Windows too.
 
-  * Go[5] is required for running tests, but not for building. Note that the
-    runner.go tests do not work on Windows.
+  * Go[5] is required for running tests, but not for building.
 
 Using Ninja (note the 'N' is capitalized in the cmake invocation):
 
@@ -63,8 +62,6 @@ Known Limitations on Windows:
     don't have steps for assembling the assembly language source files, so they
     currently cannot be used to build BoringSSL.
 
-  * The tests written in Go do not work.
-
 [1] http://www.cmake.org/download/
 
 [2] https://martine.github.io/ninja/
diff --git a/crypto/bio/socket.c b/crypto/bio/socket.c
index 3f3884ce..75412cf2 100644
--- a/crypto/bio/socket.c
+++ b/crypto/bio/socket.c
@@ -66,6 +66,8 @@
 #pragma warning(push, 3)
 #include <WinSock2.h>
 #pragma warning(pop)
+
+#pragma comment(lib, "Ws2_32.lib")
 #endif
 
 #include "internal.h"
diff --git a/ssl/test/bssl_shim.cc b/ssl/test/bssl_shim.cc
index 24ce1f99..28846ca5 100644
--- a/ssl/test/bssl_shim.cc
+++ b/ssl/test/bssl_shim.cc
@@ -17,9 +17,18 @@
 #if !defined(OPENSSL_WINDOWS)
 #include <arpa/inet.h>
 #include <netinet/in.h>
+#include <netinet/tcp.h>
 #include <signal.h>
 #include <sys/socket.h>
 #include <unistd.h>
+#else
+#include <io.h>
+#pragma warning(push, 3)
+#include <WinSock2.h>
+#include <WS2tcpip.h>
+#pragma warning(pop)
+
+#pragma comment(lib, "Ws2_32.lib")
 #endif
 
 #include <string.h>
@@ -35,6 +44,21 @@
 #include "scoped_types.h"
 #include "test_config.h"
 
+
+#if !defined(OPENSSL_WINDOWS)
+static int closesocket(int sock) {
+  return close(sock);
+}
+
+static void PrintSocketError(const char *func) {
+  perror(func);
+}
+#else
+static void PrintSocketError(const char *func) {
+  fprintf(stderr, "%s: %d\n", func, WSAGetLastError());
+}
+#endif
+
 static int Usage(const char *program) {
   fprintf(stderr, "Usage: %s [flags...]\n", program);
   return 1;
@@ -272,6 +296,64 @@ static SSL_SESSION *GetSessionCallback(SSL *ssl, uint8_t *data, int len,
   }
 }
 
+// Connect returns a new socket connected to localhost on |port| or -1 on
+// error.
+static int Connect(uint16_t port) {
+  int sock = socket(AF_INET, SOCK_STREAM, 0);
+  if (sock == -1) {
+    PrintSocketError("socket");
+    return -1;
+  }
+  int nodelay = 1;
+  if (setsockopt(sock, IPPROTO_TCP, TCP_NODELAY,
+          reinterpret_cast<const char*>(&nodelay), sizeof(nodelay)) != 0) {
+    PrintSocketError("setsockopt");
+    closesocket(sock);
+    return -1;
+  }
+  sockaddr_in sin;
+  memset(&sin, 0, sizeof(sin));
+  sin.sin_family = AF_INET;
+  sin.sin_port = htons(port);
+  if (!inet_pton(AF_INET, "127.0.0.1", &sin.sin_addr)) {
+    PrintSocketError("inet_pton");
+    closesocket(sock);
+    return -1;
+  }
+  if (connect(sock, reinterpret_cast<const sockaddr*>(&sin),
+              sizeof(sin)) != 0) {
+    PrintSocketError("connect");
+    closesocket(sock);
+    return -1;
+  }
+  return sock;
+}
+
+class SocketCloser {
+ public:
+  explicit SocketCloser(int sock) : sock_(sock) {}
+  ~SocketCloser() {
+    // Half-close and drain the socket before releasing it. This seems to be
+    // necessary for graceful shutdown on Windows. It will also avoid write
+    // failures in the test runner.
+#if defined(OPENSSL_WINDOWS)
+    shutdown(sock_, SD_SEND);
+#else
+    shutdown(sock_, SHUT_WR);
+#endif
+    while (true) {
+      char buf[1024];
+      if (recv(sock_, buf, sizeof(buf), 0) <= 0) {
+        break;
+      }
+    }
+    closesocket(sock_);
+  }
+
+ private:
+  const int sock_;
+};
+
 static ScopedSSL_CTX SetupCtx(const TestConfig *config) {
   ScopedSSL_CTX ssl_ctx(SSL_CTX_new(
       config->is_dtls ? DTLS_method() : TLS_method()));
@@ -387,13 +469,13 @@ static bool RetryAsync(SSL *ssl, int ret, BIO *async,
   }
 }
 
-// DoExchange runs a test SSL exchange against the peer on file descriptor
-// |fd|. On success, it returns true and sets |*out_session| to the negotiated
-// SSL session. If the test is a resumption attempt, |is_resume| is true and
-// |session| is the session from the previous exchange.
+// DoExchange runs a test SSL exchange against the peer. On success, it returns
+// true and sets |*out_session| to the negotiated SSL session. If the test is a
+// resumption attempt, |is_resume| is true and |session| is the session from the
+// previous exchange.
 static bool DoExchange(ScopedSSL_SESSION *out_session, SSL_CTX *ssl_ctx,
                        const TestConfig *config, bool is_resume,
-                       int fd, SSL_SESSION *session) {
+                       SSL_SESSION *session) {
   OPENSSL_timeval clock = {0}, clock_delta = {0};
   ScopedSSL ssl(SSL_new(ssl_ctx));
   if (!ssl) {
@@ -503,7 +585,13 @@ static bool DoExchange(ScopedSSL_SESSION *out_session, SSL_CTX *ssl_ctx,
     SSL_set_mtu(ssl.get(), config->mtu);
   }
 
-  ScopedBIO bio(BIO_new_fd(fd, 1 /* take ownership */));
+  int sock = Connect(config->port);
+  if (sock == -1) {
+    return false;
+  }
+  SocketCloser closer(sock);
+
+  ScopedBIO bio(BIO_new_socket(sock, BIO_NOCLOSE));
   if (!bio) {
     return false;
   }
@@ -775,7 +863,20 @@ static bool DoExchange(ScopedSSL_SESSION *out_session, SSL_CTX *ssl_ctx,
 }
 
 int main(int argc, char **argv) {
-#if !defined(OPENSSL_WINDOWS)
+#if defined(OPENSSL_WINDOWS)
+  /* Initialize Winsock. */
+  WORD wsa_version = MAKEWORD(2, 2);
+  WSADATA wsa_data;
+  int wsa_err = WSAStartup(wsa_version, &wsa_data);
+  if (wsa_err != 0) {
+    fprintf(stderr, "WSAStartup failed: %d\n", wsa_err);
+    return 1;
+  }
+  if (wsa_data.wVersion != wsa_version) {
+    fprintf(stderr, "Didn't get expected version: %x\n", wsa_data.wVersion);
+    return 1;
+  }
+#else
   signal(SIGPIPE, SIG_IGN);
 #endif
 
@@ -802,14 +903,14 @@ int main(int argc, char **argv) {
 
   ScopedSSL_SESSION session;
   if (!DoExchange(&session, ssl_ctx.get(), &config, false /* is_resume */,
-                  3 /* fd */, NULL /* session */)) {
+                  NULL /* session */)) {
     BIO_print_errors_fp(stdout);
     return 1;
   }
 
   if (config.resume &&
       !DoExchange(NULL, ssl_ctx.get(), &config, true /* is_resume */,
-                  4 /* fd */, session.get())) {
+                  session.get())) {
     BIO_print_errors_fp(stdout);
     return 1;
   }
diff --git a/ssl/test/packeted_bio.cc b/ssl/test/packeted_bio.cc
index 8f4f3de8..77cd17c9 100644
--- a/ssl/test/packeted_bio.cc
+++ b/ssl/test/packeted_bio.cc
@@ -49,8 +49,12 @@ static int PacketedWrite(BIO *bio, const char *in, int inl) {
     return ret;
   }
 
-  // Write the buffer. BIOs for which this operation fails are not supported.
+  // Write the buffer.
   ret = BIO_write(bio->next_bio, in, inl);
+  if (ret < 0 || (inl > 0 && ret == 0)) {
+    BIO_copy_next_retry(bio);
+    return ret;
+  }
   assert(ret == inl);
   return ret;
 }
@@ -98,6 +102,9 @@ static int PacketedRead(BIO *bio, char *out, int outl) {
 
     // Send an ACK to the peer.
     ret = BIO_write(bio->next_bio, &kOpcodeTimeoutAck, 1);
+    if (ret <= 0) {
+      return ret;
+    }
     assert(ret == 1);
 
     // Signal to the caller to retry the read, after processing the
diff --git a/ssl/test/runner/runner.go b/ssl/test/runner/runner.go
index 8e9a9480..1c844407 100644
--- a/ssl/test/runner/runner.go
+++ b/ssl/test/runner/runner.go
@@ -970,27 +970,6 @@ func gdbOf(path string, args ...string) *exec.Cmd {
 	return exec.Command("xterm", xtermArgs...)
 }
 
-func openSocketPair() (shimEnd *os.File, conn net.Conn) {
-	socks, err := syscall.Socketpair(syscall.AF_UNIX, syscall.SOCK_STREAM, 0)
-	if err != nil {
-		panic(err)
-	}
-
-	syscall.CloseOnExec(socks[0])
-	syscall.CloseOnExec(socks[1])
-	shimEnd = os.NewFile(uintptr(socks[0]), "shim end")
-	connFile := os.NewFile(uintptr(socks[1]), "our end")
-	conn, err = net.FileConn(connFile)
-	if err != nil {
-		panic(err)
-	}
-	connFile.Close()
-	if err != nil {
-		panic(err)
-	}
-	return shimEnd, conn
-}
-
 type moreMallocsError struct{}
 
 func (moreMallocsError) Error() string {
@@ -999,16 +978,45 @@ func (moreMallocsError) Error() string {
 
 var errMoreMallocs = moreMallocsError{}
 
+// accept accepts a connection from listener, unless waitChan signals a process
+// exit first.
+func acceptOrWait(listener net.Listener, waitChan chan error) (net.Conn, error) {
+	type connOrError struct {
+		conn net.Conn
+		err  error
+	}
+	connChan := make(chan connOrError, 1)
+	go func() {
+		conn, err := listener.Accept()
+		connChan <- connOrError{conn, err}
+		close(connChan)
+	}()
+	select {
+	case result := <-connChan:
+		return result.conn, result.err
+	case childErr := <-waitChan:
+		waitChan <- childErr
+		return nil, fmt.Errorf("child exited early: %s", childErr)
+	}
+}
+
 func runTest(test *testCase, buildDir string, mallocNumToFail int64) error {
 	if !test.shouldFail && (len(test.expectedError) > 0 || len(test.expectedLocalError) > 0) {
 		panic("Error expected without shouldFail in " + test.name)
 	}
 
-	shimEnd, conn := openSocketPair()
-	shimEndResume, connResume := openSocketPair()
+	listener, err := net.ListenTCP("tcp4", &net.TCPAddr{IP: net.IP{127, 0, 0, 1}})
+	if err != nil {
+		panic(err)
+	}
+	defer func() {
+		if listener != nil {
+			listener.Close()
+		}
+	}()
 
 	shim_path := path.Join(buildDir, "ssl/test/bssl_shim")
-	var flags []string
+	flags := []string{"-port", strconv.Itoa(listener.Addr().(*net.TCPAddr).Port)}
 	if test.testType == serverTest {
 		flags = append(flags, "-server")
 
@@ -1049,7 +1057,6 @@ func runTest(test *testCase, buildDir string, mallocNumToFail int64) error {
 	} else {
 		shim = exec.Command(shim_path, flags...)
 	}
-	shim.ExtraFiles = []*os.File{shimEnd, shimEndResume}
 	shim.Stdin = os.Stdin
 	var stdoutBuf, stderrBuf bytes.Buffer
 	shim.Stdout = &stdoutBuf
@@ -1066,8 +1073,8 @@ func runTest(test *testCase, buildDir string, mallocNumToFail int64) error {
 	if err := shim.Start(); err != nil {
 		panic(err)
 	}
-	shimEnd.Close()
-	shimEndResume.Close()
+	waitChan := make(chan error, 1)
+	go func() { waitChan <- shim.Wait() }()
 
 	config := test.config
 	config.ClientSessionCache = NewLRUClientSessionCache(1)
@@ -1076,16 +1083,27 @@ func runTest(test *testCase, buildDir string, mallocNumToFail int64) error {
 		if len(config.Certificates) == 0 {
 			config.Certificates = []Certificate{getRSACertificate()}
 		}
+	} else {
+		// Supply a ServerName to ensure a constant session cache key,
+		// rather than falling back to net.Conn.RemoteAddr.
+		if len(config.ServerName) == 0 {
+			config.ServerName = "test"
+		}
 	}
 
-	err := doExchange(test, &config, conn, test.messageLen,
-		false /* not a resumption */)
-	conn.Close()
+	conn, err := acceptOrWait(listener, waitChan)
+	if err == nil {
+		err = doExchange(test, &config, conn, test.messageLen, false /* not a resumption */)
+		conn.Close()
+	}
 
 	if err == nil && test.resumeSession {
 		var resumeConfig Config
 		if test.resumeConfig != nil {
 			resumeConfig = *test.resumeConfig
+			if len(resumeConfig.ServerName) == 0 {
+				resumeConfig.ServerName = config.ServerName
+			}
 			if len(resumeConfig.Certificates) == 0 {
 				resumeConfig.Certificates = []Certificate{getRSACertificate()}
 			}
@@ -1097,12 +1115,20 @@ func runTest(test *testCase, buildDir string, mallocNumToFail int64) error {
 		} else {
 			resumeConfig = config
 		}
-		err = doExchange(test, &resumeConfig, connResume, test.messageLen,
-			true /* resumption */)
+		var connResume net.Conn
+		connResume, err = acceptOrWait(listener, waitChan)
+		if err == nil {
+			err = doExchange(test, &resumeConfig, connResume, test.messageLen, true /* resumption */)
+			connResume.Close()
+		}
 	}
-	connResume.Close()
 
-	childErr := shim.Wait()
+	// Close the listener now. This is to avoid hangs should the shim try to
+	// open more connections than expected.
+	listener.Close()
+	listener = nil
+
+	childErr := <-waitChan
 	if exitError, ok := childErr.(*exec.ExitError); ok {
 		if exitError.Sys().(syscall.WaitStatus).ExitStatus() == 88 {
 			return errMoreMallocs
@@ -2816,7 +2842,11 @@ func statusPrinter(doneChan chan *testOutput, statusChan chan statusMsg, total i
 	for msg := range statusChan {
 		if !*pipe {
 			// Erase the previous status line.
-			fmt.Printf("\x1b[%dD\x1b[K", lineLen)
+			var erase string
+			for i := 0; i < lineLen; i++ {
+				erase += "\b \b"
+			}
+			fmt.Print(erase)
 		}
 
 		if msg.started {
diff --git a/ssl/test/test_config.cc b/ssl/test/test_config.cc
index 78dcb012..afcb1067 100644
--- a/ssl/test/test_config.cc
+++ b/ssl/test/test_config.cc
@@ -102,6 +102,7 @@ const Flag<std::string> kBase64Flags[] = {
 };
 
 const Flag<int> kIntFlags[] = {
+  { "-port", &TestConfig::port },
   { "-min-version", &TestConfig::min_version },
   { "-max-version", &TestConfig::max_version },
   { "-mtu", &TestConfig::mtu },
@@ -110,7 +111,8 @@ const Flag<int> kIntFlags[] = {
 }  // namespace
 
 TestConfig::TestConfig()
-    : is_server(false),
+    : port(0),
+      is_server(false),
       is_dtls(false),
       resume(false),
       fallback_scsv(false),
diff --git a/ssl/test/test_config.h b/ssl/test/test_config.h
index 3bd65e66..61b25c04 100644
--- a/ssl/test/test_config.h
+++ b/ssl/test/test_config.h
@@ -21,6 +21,7 @@
 struct TestConfig {
   TestConfig();
 
+  int port;
   bool is_server;
   bool is_dtls;
   bool resume;