net: add OCSP tests.

net/ocsp/nss_ocsp.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "net/ocsp/nss_ocsp.h"
 
 #include <certt.h>
 #include <certdb.h>
 #include <ocsp.h>
 #include <nspr.h>
@@ skipping 31 matching lines @@
 pthread_mutex_t g_request_context_lock = PTHREAD_MUTEX_INITIALIZER;
 static net::URLRequestContext* g_request_context = NULL;
 
 class OCSPRequestSession;
 
 class OCSPIOLoop {
  public:
   void StartUsing() {
     base::AutoLock autolock(lock_);
     used_ = true;
+    io_loop_ = MessageLoopForIO::current();
+    DCHECK(io_loop_);
   }
 
   // Called on IO loop.
   void Shutdown();
 
   bool used() const {
     base::AutoLock autolock(lock_);
     return used_;
   }
 
@@ skipping 387 matching lines @@
  private:
   std::string host_;
   int port_;
 
   DISALLOW_COPY_AND_ASSIGN(OCSPServerSession);
 };
 
 OCSPIOLoop::OCSPIOLoop()
     : shutdown_(false),
       used_(false),
-      io_loop_(MessageLoopForIO::current()) {
-  DCHECK(io_loop_);
+      io_loop_(NULL) {
 }
 
 OCSPIOLoop::~OCSPIOLoop() {
   // IO thread was already deleted before the singleton is deleted
   // in AtExitManager.
   {
     base::AutoLock autolock(lock_);
     DCHECK(!io_loop_);
     DCHECK(!used_);
     DCHECK(shutdown_);
@@ skipping 34 matching lines @@
   base::AutoLock autolock(lock_);
   DCHECK_EQ(MessageLoopForIO::current(), io_loop_);
 }
 
 void OCSPIOLoop::AddRequest(OCSPRequestSession* request) {
   DCHECK(!ContainsKey(requests_, request));
   requests_.insert(request);
 }
 
 void OCSPIOLoop::RemoveRequest(OCSPRequestSession* request) {
-  {
-    // Ignore if we've already shutdown.
-    base::AutoLock auto_lock(lock_);
-    if (shutdown_)
-      return;
-  }
-
   DCHECK(ContainsKey(requests_, request));
   requests_.erase(request);
 }
 
 void OCSPIOLoop::CancelAllRequests() {
   std::set<OCSPRequestSession*> requests;
   requests.swap(requests_);
 
-  for (std::set<OCSPRequestSession*>::iterator it = requests.begin();
-       it != requests.end(); ++it)
-    (*it)->CancelURLRequest();
+  // We can't use an iterator here because CancelURLRequest will call back into
+  // us with RemoveRequest and mutate |requests_|.
+  while (!requests_.empty()) {

[Ryan Sleevi, 2012/03/13 23:06:39]

This is surely wrong - the swap on line 522 should have requests_.empty() always
be true.

Further, I'm not sure how the original code was bugged, since mutating
|requests_| would be fine, since we swapped on line 528 (original) and only
iterate the stack copy (line 530).

[agl, 2012/03/13 23:44:03]

Yes, sorry. That was a slip with git add -p, it shouldn't have been staged. This
was me misunderstanding what was going wrong. The rest of the changes in this
file are the bug fix. In short:

1) We would grab the MessageLoop at construction time, which broke if we did an
Init/Shutdown several times with a different MessageLoop.

2) The code in RemoveRequest to ignore removals at shutdown time would mean that
they were left in requests_ and triggered a use-after-free when we shut it down
again.

[Ryan Sleevi, 2012/03/13 23:50:20]

For unit tests that are affected by such globals, you're supposed to construct a
base::ShadowingAtExitManager within the test (fixture or individual test).

This should allow you to avoid most of the changes, mod this one.
Is this unnecessary if using a ShadowingAtExitManager?

I'm just trying to figure out where the root bug is.

+    (*requests.begin())->CancelURLRequest();
+  }
 }
 
 OCSPNSSInitialization::OCSPNSSInitialization() {
   // NSS calls the functions in the function table to download certificates
   // or CRLs or talk to OCSP responders over HTTP.  These functions must
   // set an NSS/NSPR error code when they fail.  Otherwise NSS will get the
   // residual error code from an earlier failed function call.
   client_fcn_.version = 1;
   SEC_HttpClientFcnV1Struct *ft = &client_fcn_.fcnTable.ftable1;
   ft->createSessionFcn = OCSPCreateSession;
@@ skipping 395 matching lines @@
 void SetURLRequestContextForOCSP(URLRequestContext* request_context) {
   pthread_mutex_lock(&g_request_context_lock);
   if (request_context) {
     DCHECK(!g_request_context);
   }
   g_request_context = request_context;
   pthread_mutex_unlock(&g_request_context_lock);
 }
 
 }  // namespace net