Bug 1303450 - Enable LSan for BoGo test runs r=ekr,franziskus

automation/taskcluster/graph/src/extend.js

@@ -45,11 +45,6 @@ queue.filter(task => {
 
 queue.map(task => {
   if (task.collection == "asan") {
-    // Disable LSan on BoGo runs, for now.
-    if (task.tests == "bogo") {
-      task.env.ASAN_OPTIONS = "detect_leaks=0";
-    }
-
     // CRMF and FIPS tests still leak, unfortunately.
     if (task.tests == "crmf" || task.tests == "fips") {
       task.env.ASAN_OPTIONS = "detect_leaks=0";

external_tests/nss_bogo_shim/config.h

@@ -15,6 +15,7 @@
 
 #include <iostream>
 #include <map>
+#include <memory>
 #include <queue>
 #include <string>
 #include <typeinfo>
@@ -25,6 +26,8 @@ class ConfigEntryBase {
   ConfigEntryBase(const std::string& name, const std::string& type)
       : name_(name), type_(type) {}
 
+  virtual ~ConfigEntryBase() {}
+
   const std::string& type() const { return type_; }
   virtual bool Parse(std::queue<const char*>* args) = 0;
 
@@ -62,7 +65,8 @@ class Config {
 
   template <typename T>
   void AddEntry(const std::string& name, T init) {
-    entries_[name] = new ConfigEntry<T>(name, init);
+    entries_[name] = std::unique_ptr<ConfigEntryBase>(
+      new ConfigEntry<T>(name, init));
   }
 
   Status ParseArgs(int argc, char** argv);
@@ -77,12 +81,12 @@ class Config {
  private:
   static std::string XformFlag(const std::string& arg);
 
-  std::map<std::string, ConfigEntryBase*> entries_;
+  std::map<std::string, std::unique_ptr<ConfigEntryBase>> entries_;
 
   const ConfigEntryBase* entry(const std::string& key) const {
     auto e = entries_.find(key);
     if (e == entries_.end()) return nullptr;
-    return e->second;
+    return e->second.get();
   }
 };
 

external_tests/nss_bogo_shim/nss_bogo_shim.cc

@@ -18,6 +18,8 @@
 
 #include "nsskeys.h"
 
+bool exitCodeUnimplemented = false;
+
 std::string FormatError(PRErrorCode code) {
   return std::string(":") + PORT_ErrorToName(code) + ":" + ":" +
          PORT_ErrorToString(code);
@@ -109,7 +111,11 @@ class TestAgent {
 
     if (cfg_.get<std::string>("key-file") != "") {
       key_ = ReadPrivateKey(cfg_.get<std::string>("key-file"));
-      if (!key_) exit(89);  // Temporary to handle our inability to handle ECDSA
+      if (!key_) {
+        // Temporary to handle our inability to handle ECDSA.
+        exitCodeUnimplemented = true;
+        return false;
+      }
     }
     if (cfg_.get<std::string>("cert-file") != "") {
       cert_ = ReadCertificate(cfg_.get<std::string>("cert-file"));
@@ -122,11 +128,6 @@ class TestAgent {
         std::cerr << "Couldn't configure server cert\n";
         return false;
       }
-      rv = SSL_ConfigServerSessionIDCache(1024, 0, 0, ".");
-      if (rv != SECSuccess) {
-        std::cerr << "Couldn't configure session cache\n";
-        return false;
-      }
     } else {
       // Client.
 
@@ -273,43 +274,67 @@ std::unique_ptr<const Config> ReadConfig(int argc, char** argv) {
     case Config::kOK:
       break;
     case Config::kUnknownFlag:
-      exit(89);
-      break;
+      exitCodeUnimplemented = true;
     default:
-      exit(1);
+      return nullptr;
   }
 
   // Needed to change to std::unique_ptr<const Config>
   return std::move(cfg);
 }
 
-void RunCycle(std::unique_ptr<const Config>& cfg) {
+
+bool RunCycle(std::unique_ptr<const Config>& cfg) {
   std::unique_ptr<TestAgent> agent(TestAgent::Create(*cfg));
-  if (!agent) {
-    exit(1);
+  return agent && agent->DoExchange() == SECSuccess;
+}
+
+int GetExitCode(bool success) {
+  if (exitCodeUnimplemented) {
+    return 89;
   }
 
-  SECStatus rv = agent->DoExchange();
-  if (rv) {
-    exit(1);
+  if (success) {
+    return 0;
   }
+
+  return 1;
 }
 
 int main(int argc, char** argv) {
   std::unique_ptr<const Config> cfg = ReadConfig(argc, argv);
+  if (!cfg) {
+    return GetExitCode(false);
+  }
 
-  SECStatus rv = NSS_NoDB_Init(nullptr);
-  if (rv != SECSuccess) return 1;
-  rv = NSS_SetDomesticPolicy();
-  if (rv != SECSuccess) return 1;
+  if (cfg->get<bool>("server")) {
+    if (SSL_ConfigServerSessionIDCache(1024, 0, 0, ".") != SECSuccess) {
+      std::cerr << "Couldn't configure session cache\n";
+      return 1;
+    }
+  }
+
+  if (NSS_NoDB_Init(nullptr) != SECSuccess) {
+    return 1;
+  }
 
   // Run a single test cycle.
-  RunCycle(cfg);
+  bool success = RunCycle(cfg);
 
-  if (cfg->get<bool>("resume")) {
+  if (success && cfg->get<bool>("resume")) {
     std::cout << "Resuming" << std::endl;
-    RunCycle(cfg);
+    success = RunCycle(cfg);
+  }
+
+  SSL_ClearSessionCache();
+
+  if (cfg->get<bool>("server")) {
+    SSL_ShutdownServerSessionIDCache();
+  }
+
+  if (NSS_Shutdown() != SECSuccess) {
+    success = false;
   }
 
-  exit(0);
+  return GetExitCode(success);
 }

external_tests/ssl_gtest/ssl_resumption_unittest.cc

@@ -421,7 +421,6 @@ TEST_F(TlsConnectTest, TestTls13ResumptionTwice) {
   EXPECT_TRUE(client_->cipher_suite(&resumed_suite));
   EXPECT_EQ(original_suite, resumed_suite);
 
-  // TODO(ekr@rtfm.com): This will change when we fix bug 1257047.
-  ASSERT_EQ(initialTicket, c2->extension());
+  ASSERT_NE(initialTicket, c2->extension());
 }
 }  // namespace nss_test

lib/ssl/tls13con.c

@@ -3626,14 +3626,10 @@ tls13_HandleNewSessionTicket(sslSocket *ss, SSL3Opaque *b, PRUint32 length)
      * in use. I believe this works, but I can't test it until the
      * server side supports it. Bug 1257047.
      */
-    if (!ss->opt.noCache && ss->ssl3.hs.kea_def->authKeyType != ssl_auth_psk) {
+    if (!ss->opt.noCache) {
+        PORT_Assert(ss->sec.ci.sid);
 
-        /* Uncache so that we replace. */
-        ss->sec.uncache(ss->sec.ci.sid);
-
-        /* We only support DHE resumption so any ticket which doesn't
-         * support it we don't cache, but it can evict previous
-         * cache entries. */
+        /* We only support DHE resumption. */
         if (!(ticket.flags & ticket_allow_psk_dhe_ke)) {
             return SECSuccess;
         }
@@ -3651,6 +3647,18 @@ tls13_HandleNewSessionTicket(sslSocket *ss, SSL3Opaque *b, PRUint32 length)
                        ticket.ticket.data,
                        ticket.ticket.len));
 
+        /* Replace a previous session ticket when
+         * we receive a second NewSessionTicket message. */
+        if (ss->sec.ci.sid->cached == in_client_cache) {
+            /* Uncache first. */
+            ss->sec.uncache(ss->sec.ci.sid);
+
+            /* Then destroy and rebuild the SID. */
+            ssl_FreeSID(ss->sec.ci.sid);
+            ss->sec.ci.sid = ssl3_NewSessionID(ss, PR_FALSE);
+            ss->sec.ci.sid->cached = never_cached;
+        }
+
         ssl3_SetSIDSessionTicket(ss->sec.ci.sid, &ticket);
         PORT_Assert(!ticket.ticket.data);
 
@@ -3659,7 +3667,6 @@ tls13_HandleNewSessionTicket(sslSocket *ss, SSL3Opaque *b, PRUint32 length)
             return SECFailure;
 
         /* Cache the session. */
-        ss->sec.ci.sid->cached = never_cached;
         ss->sec.cache(ss->sec.ci.sid);
     }