Defer writing the shim settings.

This is prefactoring for a coming change to the shim that will write
handoff and handback messages (which are serialized SSLConnection
objects) to the transcript.

This breaks the slightly tenuous ordering between the runner and the
shim. Fix the runner to wait until the shim has exited before
appending the transcript.

Change-Id: Iae34d28ec1addfe3ec4f3c77008248fe5530687c
Reviewed-on: https://boringssl-review.googlesource.com/27184
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>

fuzz/refresh_ssl_corpora.sh

@@ -70,8 +70,8 @@ fuzzer_mode_shim=$(readlink -f "$fuzzer_mode_build_dir/ssl/test/bssl_shim")
 no_fuzzer_mode_shim=$(readlink -f \
     "$no_fuzzer_mode_build_dir/ssl/test/bssl_shim")
 
-fuzzer_mode_transcripts=$(mktemp -d '/tmp/boringssl-transcript.XXXXXX')
-no_fuzzer_mode_transcripts=$(mktemp -d '/tmp/boringssl-transcript.XXXXXX')
+fuzzer_mode_transcripts=$(mktemp -d '/tmp/boringssl-transcript-fuzzer-mode.XXXXXX')
+no_fuzzer_mode_transcripts=$(mktemp -d '/tmp/boringssl-transcript-no-fuzzer-mode.XXXXXX')
 
 echo Recording fuzzer-mode transcripts
 (cd ../ssl/test/runner/ && go test \
@@ -84,7 +84,7 @@ echo Recording non-fuzzer-mode transcripts
 (cd ../ssl/test/runner/ && go test \
     -shim-path "$no_fuzzer_mode_shim" \
     -transcript-dir "$no_fuzzer_mode_transcripts" \
-    -deterministic) || true
+    -deterministic)
 
 
 # Minimize the existing corpora.

ssl/test/bssl_shim.cc

@@ -1849,66 +1849,85 @@ static bool CheckHandshakeProperties(SSL *ssl, bool is_resume,
   return true;
 }
 
-static bool WriteSettings(int i, const TestConfig *config,
-                          const SSL_SESSION *session) {
-  if (config->write_settings.empty()) {
-    return true;
-  }
+// SettingsWriter writes fuzzing inputs to a file if |write_settings| is set.
+struct SettingsWriter {
+ public:
+  SettingsWriter() {}
 
-  // Treat write_settings as a path prefix for each connection in the run.
-  char buf[DECIMAL_SIZE(int)];
-  snprintf(buf, sizeof(buf), "%d", i);
-  std::string path = config->write_settings + buf;
+  // Init initializes the writer for a new connection, given by |i|.  Each
+  // connection gets a unique output file.
+  bool Init(int i, const TestConfig *config, SSL_SESSION *session) {
+    if (config->write_settings.empty()) {
+      return true;
+    }
+    // Treat write_settings as a path prefix for each connection in the run.
+    char buf[DECIMAL_SIZE(int)];
+    snprintf(buf, sizeof(buf), "%d", i);
+    path_ = config->write_settings + buf;
 
-  bssl::ScopedCBB cbb;
-  if (!CBB_init(cbb.get(), 64)) {
-    return false;
-  }
+    if (!CBB_init(cbb_.get(), 64)) {
+      return false;
+    }
 
-  if (session != nullptr) {
-    uint8_t *data;
-    size_t len;
-    if (!SSL_SESSION_to_bytes(session, &data, &len)) {
+    if (session != nullptr) {
+      uint8_t *data;
+      size_t len;
+      if (!SSL_SESSION_to_bytes(session, &data, &len)) {
+        return false;
+      }
+      bssl::UniquePtr<uint8_t> free_data(data);
+      CBB child;
+      if (!CBB_add_u16(cbb_.get(), kSessionTag) ||
+          !CBB_add_u24_length_prefixed(cbb_.get(), &child) ||
+          !CBB_add_bytes(&child, data, len) ||
+          !CBB_flush(cbb_.get())) {
+        return false;
+      }
+    }
+
+    if (config->is_server &&
+        (config->require_any_client_certificate || config->verify_peer) &&
+        !CBB_add_u16(cbb_.get(), kRequestClientCert)) {
       return false;
     }
-    bssl::UniquePtr<uint8_t> free_data(data);
-    CBB child;
-    if (!CBB_add_u16(cbb.get(), kSessionTag) ||
-        !CBB_add_u24_length_prefixed(cbb.get(), &child) ||
-        !CBB_add_bytes(&child, data, len) ||
-        !CBB_flush(cbb.get())) {
+
+    if (config->tls13_variant != 0 &&
+        (!CBB_add_u16(cbb_.get(), kTLS13Variant) ||
+         !CBB_add_u8(cbb_.get(),
+                     static_cast<uint8_t>(config->tls13_variant)))) {
       return false;
     }
-  }
 
-  if (config->is_server &&
-      (config->require_any_client_certificate || config->verify_peer) &&
-      !CBB_add_u16(cbb.get(), kRequestClientCert)) {
-    return false;
+    return true;
   }
 
-  if (config->tls13_variant != 0 &&
-      (!CBB_add_u16(cbb.get(), kTLS13Variant) ||
-       !CBB_add_u8(cbb.get(), static_cast<uint8_t>(config->tls13_variant)))) {
-    return false;
-  }
+  // Commit writes the buffered data to disk.
+  bool Commit() {
+    if (path_.empty()) {
+      return true;
+    }
 
-  uint8_t *settings;
-  size_t settings_len;
-  if (!CBB_add_u16(cbb.get(), kDataTag) ||
-      !CBB_finish(cbb.get(), &settings, &settings_len)) {
-    return false;
-  }
-  bssl::UniquePtr<uint8_t> free_settings(settings);
+    uint8_t *settings;
+    size_t settings_len;
+    if (!CBB_add_u16(cbb_.get(), kDataTag) ||
+        !CBB_finish(cbb_.get(), &settings, &settings_len)) {
+      return false;
+    }
+    bssl::UniquePtr<uint8_t> free_settings(settings);
 
-  using ScopedFILE = std::unique_ptr<FILE, decltype(&fclose)>;
-  ScopedFILE file(fopen(path.c_str(), "w"), fclose);
-  if (!file) {
-    return false;
+    using ScopedFILE = std::unique_ptr<FILE, decltype(&fclose)>;
+    ScopedFILE file(fopen(path_.c_str(), "w"), fclose);
+    if (!file) {
+      return false;
+    }
+
+    return fwrite(settings, settings_len, 1, file.get()) == 1;
   }
 
-  return fwrite(settings, settings_len, 1, file.get()) == 1;
-}
+ private:
+  std::string path_;
+  bssl::ScopedCBB cbb_;
+};
 
 static bssl::UniquePtr<SSL> NewSSL(SSL_CTX *ssl_ctx, const TestConfig *config,
                                    SSL_SESSION *session, bool is_resume,
@@ -2708,12 +2727,18 @@ int main(int argc, char **argv) {
     }
 
     bssl::UniquePtr<SSL_SESSION> offer_session = std::move(session);
-    if (!WriteSettings(i, config, offer_session.get())) {
+    SettingsWriter writer;
+    if (!writer.Init(i, config, offer_session.get())) {
+      fprintf(stderr, "Error writing settings.\n");
+      return 1;
+    }
+    bool ok = DoConnection(&session, ssl_ctx.get(), config, &retry_config,
+                           is_resume, offer_session.get());
+    if (!writer.Commit()) {
       fprintf(stderr, "Error writing settings.\n");
       return 1;
     }
-    if (!DoConnection(&session, ssl_ctx.get(), config, &retry_config, is_resume,
-                      offer_session.get())) {
+    if (!ok) {
       fprintf(stderr, "Connection %d failed.\n", i + 1);
       ERR_print_errors_fp(stderr);
       return 1;

ssl/test/runner/runner.go

@@ -482,21 +482,23 @@ type testCase struct {
 
 var testCases []testCase
 
-func writeTranscript(test *testCase, path string, data []byte) {
+func appendTranscript(path string, data []byte) error {
 	if len(data) == 0 {
-		return
+		return nil
 	}
 
 	settings, err := ioutil.ReadFile(path)
 	if err != nil {
-		fmt.Fprintf(os.Stderr, "Error reading %s: %s.\n", path, err)
-		return
+		if !os.IsNotExist(err) {
+			return err
+		}
+		// If the shim aborted before writing a file, use a default
+		// settings block, so the transcript is still somewhat valid.
+		settings = []byte{0, 0} // kDataTag
 	}
 
 	settings = append(settings, data...)
-	if err := ioutil.WriteFile(path, settings, 0644); err != nil {
-		fmt.Fprintf(os.Stderr, "Error writing %s: %s\n", path, err)
-	}
+	return ioutil.WriteFile(path, settings, 0644)
 }
 
 // A timeoutConn implements an idle timeout on each Read and Write operation.
@@ -523,7 +525,7 @@ func (t *timeoutConn) Write(b []byte) (int, error) {
 	return t.Conn.Write(b)
 }
 
-func doExchange(test *testCase, config *Config, conn net.Conn, isResume bool, transcriptPrefix string, num int) error {
+func doExchange(test *testCase, config *Config, conn net.Conn, isResume bool, transcripts *[][]byte, num int) error {
 	if !test.noSessionCache {
 		if config.ClientSessionCache == nil {
 			config.ClientSessionCache = NewLRUClientSessionCache(1)
@@ -575,10 +577,13 @@ func doExchange(test *testCase, config *Config, conn net.Conn, isResume bool, tr
 		if *flagDebug {
 			defer connDebug.WriteTo(os.Stdout)
 		}
-		if len(transcriptPrefix) != 0 {
+		if len(*transcriptDir) != 0 {
 			defer func() {
-				path := transcriptPrefix + strconv.Itoa(num)
-				writeTranscript(test, path, connDebug.Transcript())
+				if num == len(*transcripts) {
+					*transcripts = append(*transcripts, connDebug.Transcript())
+				} else {
+					panic("transcripts are out of sync")
+				}
 			}()
 		}
 
@@ -1118,6 +1123,7 @@ func runTest(test *testCase, shimPath string, mallocNumToFail int64) error {
 	}
 
 	var transcriptPrefix string
+	var transcripts [][]byte
 	if len(*transcriptDir) != 0 {
 		protocol := "tls"
 		if test.protocol == dtls {
@@ -1176,7 +1182,7 @@ func runTest(test *testCase, shimPath string, mallocNumToFail int64) error {
 
 	conn, err := acceptOrWait(listener, waitChan)
 	if err == nil {
-		err = doExchange(test, &config, conn, false /* not a resumption */, transcriptPrefix, 0)
+		err = doExchange(test, &config, conn, false /* not a resumption */, &transcripts, 0)
 		conn.Close()
 	}
 
@@ -1196,7 +1202,7 @@ func runTest(test *testCase, shimPath string, mallocNumToFail int64) error {
 		var connResume net.Conn
 		connResume, err = acceptOrWait(listener, waitChan)
 		if err == nil {
-			err = doExchange(test, &resumeConfig, connResume, true /* resumption */, transcriptPrefix, i+1)
+			err = doExchange(test, &resumeConfig, connResume, true /* resumption */, &transcripts, i+1)
 			connResume.Close()
 		}
 	}
@@ -1217,6 +1223,14 @@ func runTest(test *testCase, shimPath string, mallocNumToFail int64) error {
 		waitTimeout.Stop()
 	}
 
+	// Now that the shim has exitted, all the settings files have been
+	// written. Append the saved transcripts.
+	for i, transcript := range transcripts {
+		if err := appendTranscript(transcriptPrefix+strconv.Itoa(i), transcript); err != nil {
+			return err
+		}
+	}
+
 	var isValgrindError, mustFail bool
 	if exitError, ok := childErr.(*exec.ExitError); ok {
 		switch exitError.Sys().(syscall.WaitStatus).ExitStatus() {