Disk Based Certificate Cache Implementation

net/http/disk_based_cert_cache.cc

Index: net/http/disk_based_cert_cache.cc
diff --git a/net/http/disk_based_cert_cache.cc b/net/http/disk_based_cert_cache.cc
new file mode 100644
index 0000000000000000000000000000000000000000..38cd8db5498b889665fa52507f95446cc5d77eb6
--- /dev/null
+++ b/net/http/disk_based_cert_cache.cc
@@ -0,0 +1,470 @@
+// Copyright (c) 2014 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/http/disk_based_cert_cache.h"
+
+#include <list>

[Ryan Sleevi, 2014/06/18 22:16:03]

Curious: Was there a particular reason for choosing <list>?

We traditionally use <vector> for ordered sequences like this. <list> is only
used if we particularly need qualities that only a list affords (eg: efficient
re-ordering)

[brandonsalmon, 2014/06/19 21:32:31]

In this circumstance, I could not see any particular reason for using a vector.
I switched to using a list because list::push_back guarantees iterator validity
(and push_back could conceivably be called by one of the user_callbacks_).

+
+#include "base/callback_helpers.h"
+#include "base/memory/ref_counted.h"
+#include "base/stl_util.h"
+#include "base/strings/string_number_conversions.h"
+#include "net/base/io_buffer.h"
+#include "net/base/net_errors.h"
+#include "net/disk_cache/disk_cache.h"
+
+namespace net {
+
+// WriteWorkers represent pending set jobs in the DiskBasedCertCache. Each
+// certificate requested to be cached is assigned a Writeworker on a one-to-one
+// basis. The same certificate should not have multiple WriteWorkers at the same
+// time, instead, add a user_callback_ to the existing WriteWorker.

[Ryan Sleevi, 2014/06/18 22:16:04]

comment nit:
// time, instead, add a user_callback_ to the existing WriteWorker.
becomes

// time; instead, use AddCallback to register a callback to be notified when
// the write has completed

+class DiskBasedCertCache::WriteWorker {
+ public:
+  // |backend| is a pointer to the initialized disk_cache::backend stored in
+  // DiskBasedCertCache. |key| is the key corresponding to this certificate
+  // to be used in caching. |cert_handle| is the certificate to be stored.
+  // |cleanup_callback| is a callback to the function that will
+  // delete this worker. |user_callback| is a callback to the user
+  // to signify that the certificate has been correctly stored, or that the
+  // operation failed.

[Ryan Sleevi, 2014/06/18 22:16:04]

comment style: we generally don't note types in prose, but instead note
purpose/use;

// |backend| is the backend to store |certificate| in, using
// |key| as the key for the disk_cache::Entry.
// |cleanup_callback| is called to clean up this WriteWorker,
// regardless of success or failure.

+  WriteWorker(disk_cache::Backend* backend,
+              const std::string& key,
+              const X509Certificate::OSCertHandle cert_handle,
+              const base::Callback<void(void)>& cleanup_callback,
+              const SetCallback& user_callback);

[Ryan Sleevi, 2014/06/18 22:16:03]

It seems like, for consistency, that the caller should call AddCallback() to add
|user_callback|, rather than passing it in the constructor.

+
+  ~WriteWorker();
+
+  // Begin the do-loop for this WriteWorker (will call all user_callbacks_ on
+  // completion).

[Ryan Sleevi, 2014/06/18 22:16:03]

Comment rewording:

// Writes the given certificate to the cache. On completion, will invoke
// all user callbacks set.

+  void Start();
+
+  // Add a callback to this worker. Should be used when |cert_handle_| is
+  // requested to be stored in the cache multiple times within the lifetime
+  // of this worker.

[Ryan Sleevi, 2014/06/18 22:16:03]

Comment rewording: We try to avoid descriptions of how you "should" use it (or
how it "is" used), and instead focus on what it does.

// Adds |user_callback| to the set of callbacks to be notified when this
// WriteWorker has completed processing.

+  void AddCallback(const SetCallback& user_callback);
+
+ private:
+  enum WriteState {
+    CREATE_OR_OPEN,
+    FINISH_CREATE_OR_OPEN,
+    START_WRITE,
+    FINISH_WRITE,
+    WRITE_NONE
+  };
+
+  void OnIOComplete(int rv);
+  void DoLoop(int rv);
+  int DoCreateOrOpen();
+  int DoFinishCreateOrOpen(int rv);
+  int DoStartWrite();
+  int DoFinishWrite(int rv);
+
+  // CallCallbacks is used to call all user callbacks (i.e. callbacks that were
+  // given by the user to the DiskBasedCertCache when calling Set).

[Ryan Sleevi, 2014/06/18 22:16:03]

// Invokes all of the |user_callbacks_|

+  void CallCallbacks(const std::string& key);

[Ryan Sleevi, 2014/06/18 22:16:03]

Why do you pass |key|, when you have |key_| as a member variable and this is a
member method?

[brandonsalmon, 2014/06/19 21:32:31]

I did this so I could use CallCallbacks(std::string()), however it might be
better to just use a boolean flag for errors.

+
+  disk_cache::Backend* backend_;
+  const X509Certificate::OSCertHandle cert_handle_;
+  std::string key_;
+
+  disk_cache::Entry* entry_;
+  WriteState state_;
+  bool create_failed_;
+  scoped_refptr<IOBuffer> buffer;
+
+  base::Callback<void(void)> cleanup_callback_;

[Ryan Sleevi, 2014/06/18 22:16:03]

the name for this is "base::Closure" - avoids the template specialization.

+  std::list<SetCallback> user_callbacks_;
+  CompletionCallback io_callback_; /* used to access the backend */

[Ryan Sleevi, 2014/06/18 22:16:03]

Delete this comment; Among other things, io_callback_ itself is not used to
access the backend.

+
+  base::WeakPtrFactory<WriteWorker> weak_factory_;
+};
+
+// ReadWorkers represent pending get jobs in the DiskBasedCertCache. Each

[Ryan Sleevi, 2014/06/18 22:16:03]

s/get/Get/

+// certificate requested to be retrieved from the cache is assigned a ReadWorker
+// on a one-to-one basis. The same |key| should not have multiple ReadWorkers
+// at the same time, instead, call AddCallback to add a user_callback_ to
+// the the existing ReadWorker.

[Ryan Sleevi, 2014/06/18 22:16:04]

Similar comment cleanup as above.

+class DiskBasedCertCache::ReadWorker {
+ public:
+  // |backend| is a pointer to the initialized disk_cache::backend stored in
+  // DiskBasedCertCache. |key| is the key corresponding to this certificate
+  // to be used in caching. |cleanup_callback| is a callback to the function
+  // that will delete this worker. |user_callback| is a callback to the user
+  // to signify that the certificate has been correctly stored, or that the
+  // operation failed.
+  ReadWorker(disk_cache::Backend* backend,

[Ryan Sleevi, 2014/06/18 22:16:03]

Same comment comments as above

+             const std::string& key,
+             const base::Callback<void(void)>& cleanup_callback,
+             const GetCallback& user_callback);

[Ryan Sleevi, 2014/06/18 22:16:03]

ditto AddCallback

+
+  ~ReadWorker();
+
+  // Begin the do-loop for this ReadWorker (will call all user_callbacks_ on
+  // completion).
+  void Start();
+
+  // Add a callback to this worker. Should be used when |key_| is
+  // requested to be retrieved from the cache multiple times within lifetime
+  // of this worker.
+  void AddCallback(const GetCallback& user_callback);
+
+ private:
+  enum ReadState { OPEN, START_READ, FINISH_READ, READ_NONE };
+
+  void OnIOComplete(int rv);
+  void DoLoop(int rv);
+  int DoOpen();
+  int DoStartRead(int rv);
+  int DoFinishRead(int rv);
+
+  // CallCallbacks is used to call all user callbacks (i.e. callbacks that were
+  // given by the user to the DiskBasedCertCache when calling Get).
+  void CallCallbacks(X509Certificate::OSCertHandle cert_handle);
+
+  disk_cache::Backend* backend_;
+
+  std::string key_;
+  disk_cache::Entry* entry_;
+  ReadState state_;
+  int entry_size_;
+  scoped_refptr<IOBuffer> buffer;
+
+  base::Callback<void(void)> cleanup_callback_;
+  std::list<GetCallback> user_callbacks_;
+  CompletionCallback io_callback_;
+  base::WeakPtrFactory<ReadWorker> weak_factory_;
+};
+
+std::string GetCacheKeyToCert(const X509Certificate::OSCertHandle cert_handle) {
+  SHA1HashValue fingerprint =
+      X509Certificate::CalculateFingerprint(cert_handle);
+
+  return "cert:" +
+         base::HexEncode(fingerprint.data, arraysize(fingerprint.data));
+}

[Ryan Sleevi, 2014/06/18 22:16:03]

This function should be between lines 18/19 in an unnamed namespace

+
+DiskBasedCertCache::WriteWorker::WriteWorker(
+    disk_cache::Backend* backend,
+    const std::string& key,
+    X509Certificate::OSCertHandle cert_handle,
+    const base::Callback<void(void)>& cleanup_callback,
+    const SetCallback& user_callback)
+    : backend_(backend),
+      cert_handle_(cert_handle),
+      key_(key),
+      entry_(NULL),
+      state_(CREATE_OR_OPEN),
+      create_failed_(false),
+      cleanup_callback_(cleanup_callback),
+      weak_factory_(this) {
+  io_callback_ =
+      base::Bind(&WriteWorker::OnIOComplete, weak_factory_.GetWeakPtr());
+  AddCallback(user_callback);
+}
+
+void DiskBasedCertCache::WriteWorker::Start() {
+  DoLoop(OK);
+
+  if (state_ == WRITE_NONE) {

[Ryan Sleevi, 2014/06/18 22:16:03]

if (state_ != WRITE_NONE)
  return;

if (entry_)
  entry->close;
base::ResetAndReturn(&cleanup_callback_).Run();

+    if (entry_)
+      entry_->Close();
+    base::ResetAndReturn(&cleanup_callback_).Run();
+  }
+}
+
+void DiskBasedCertCache::WriteWorker::AddCallback(
+    const SetCallback& user_callback) {
+  user_callbacks_.push_back(user_callback);
+}
+
+void DiskBasedCertCache::WriteWorker::OnIOComplete(int rv) {
+  DoLoop(rv);
+
+  if (state_ == WRITE_NONE) {
+    if (entry_)
+      entry_->Close();
+    base::ResetAndReturn(&cleanup_callback_).Run();
+  }

[Ryan Sleevi, 2014/06/18 22:16:03]

Ditto cleanup as above.

+}
+
+void DiskBasedCertCache::WriteWorker::DoLoop(int rv) {
+  do {
+    switch (state_) {
+      case CREATE_OR_OPEN:
+        rv = DoCreateOrOpen();
+        break;
+      case FINISH_CREATE_OR_OPEN:
+        rv = DoFinishCreateOrOpen(rv);
+        break;
+      case START_WRITE:
+        rv = DoStartWrite();
+        break;
+      case FINISH_WRITE:
+        rv = DoFinishWrite(rv);
+        break;
+      case WRITE_NONE:
+        break;
+    }
+  } while (rv != ERR_IO_PENDING && state_ != WRITE_NONE);
+}
+
+int DiskBasedCertCache::WriteWorker::DoCreateOrOpen() {
+  state_ = FINISH_CREATE_OR_OPEN;
+
+  if (create_failed_)
+    return backend_->OpenEntry(key_, &entry_, io_callback_);
+
+  return backend_->CreateEntry(key_, &entry_, io_callback_);
+}
+
+int DiskBasedCertCache::WriteWorker::DoFinishCreateOrOpen(int rv) {
+  // ERR_FAILED implies create entry failed. In order to avoid trying
+  // to open the entry multiple times, the flag create_failed_ is set and
+  // checked.
+  if (rv == ERR_FAILED && !create_failed_) {

[Ryan Sleevi, 2014/06/18 22:16:03]

Why combine CREATE_OR_OPEN into one state?

It seems like it should be two

CREATE -> CREATE_COMPLETE
OPEN -> OPEN_COMPLETE

if CREATE_COMPLETE succeeded, it goes to START_WRITE
if CREATE_COMPLETE failed, it goes to OPEN

if OPEN_COMPLETE succeeded, it goes to START_WRITE
if OPEN_COMPLETE failed, it goes to WRITE_NONE and fails.

+    create_failed_ = true;
+    state_ = CREATE_OR_OPEN;
+    return OK;
+  } else if (rv < 0) {
+    CallCallbacks(std::string());

[Ryan Sleevi, 2014/06/18 22:16:03]

DANGER!

So the same comments I made on the previous version about the cleanup_callback_
apply here to using CallCallbacks.

That is, if you CallCallback, the Set failed, and that leads to the DBCC being
deleted, then it's going to delete this worker (while CallCallbacks) is running.
You'll then crash within CallCallbacks if you're lucky, or, if not, you'll crash
here on line 233 (attempting to write to a member of a deleted object)

Assuming you got past 233, when you return ERR_FAILED, you'll end up on line
200, which will break the loop, and you'll end up in either 172/173 or 187/188 -
at which point, you'll attempt to access entry_ and crash there too.


This is why, in the MultiThreadedCertVerifier example I gave you, the
CertVerifierWorker notifies the MTCV to HandleResult, and the MTCV then cleans
up its internal state, and *then* delegates to the CertVerifierJob to
HandleResult.


I think you'll find that any scenario where the DiskBasedCertCache is
responsible for deleting a worker is going to result in threading bugs that can
crash your application, because the Worker owns the Entry.

The Entry *needs* to stay valid until all pending callbacks from the
disk_cache::Backend() have been invoked. Using WeakPtr for calls to the Backend
doesn't actually do that

+    state_ = WRITE_NONE;
+    return ERR_FAILED;
+  }
+
+  state_ = START_WRITE;
+  return OK;
+}
+
+int DiskBasedCertCache::WriteWorker::DoStartWrite() {
+  std::string write_data;
+  bool encoded = X509Certificate::GetDEREncoded(cert_handle_, &write_data);
+
+  if (!encoded) {
+    CallCallbacks(std::string());
+    state_ = WRITE_NONE;
+    return ERR_FAILED;
+  }

[Ryan Sleevi, 2014/06/18 22:16:04]

Let's chat in person how to structure these 'failure' callbacks.

It actually works out that both your failure callbacks and your success
callbacks can be hermetically handled in Start() / OnIOComplete(), safely,
without having to duplicate this logic for errors throughout the WriteWorker.

+
+  buffer = new IOBuffer(write_data.size());
+  memcpy(buffer->data(), write_data.data(), write_data.size());
+
+  state_ = FINISH_WRITE;
+
+  return entry_->WriteData(0 /* index */,
+                           0 /* offset */,
+                           buffer,
+                           write_data.size(),
+                           io_callback_,
+                           true /* truncate */);
+}
+
+int DiskBasedCertCache::WriteWorker::DoFinishWrite(int rv) {
+  if (rv < 0) {
+    CallCallbacks(std::string());
+    state_ = WRITE_NONE;
+    return ERR_FAILED;
+  }
+
+  state_ = WRITE_NONE;
+
+  CallCallbacks(key_);
+  return OK;
+}
+
+void DiskBasedCertCache::WriteWorker::CallCallbacks(const std::string& key) {
+  for (std::list<SetCallback>::iterator it = user_callbacks_.begin();
+       it != user_callbacks_.end();
+       it++) {
+    base::ResetAndReturn(&(*it)).Run(key_);

[Ryan Sleevi, 2014/06/18 22:16:03]

Same danger regarding the risk of the following:

Cache->Set(cert, callback1);
  worker = New Worker(cert, callback1);
    .. async stuff happening
(Another request)
Cache->Set(cert, callback2);
  worker->AddCallback(callback2);
.. async stuff finishes
CallCallbacks()
  Callback1.run(...)
    Cache->Set(cert, callback3);
      worker->AddCallback(callback3);  // DANGER!

I suspect, having read to this point now, you chose <list> to try to allow this;
That's a pattern, but one we actually tend to discourage.

This is another place where the MultiThreadedCertVerifier setup works in your
favour. In this case, when callback1.run(...) causes Cache->Set(...) to be
called, you would have already removed the Worker from the Cache's map, so it
would use, instead, "new Worker(cert, callback3)"

Much easier design to reason about, even if it gets into trickier stuff.

+  }
+}
+
+DiskBasedCertCache::WriteWorker::~WriteWorker() {
+}
+
+DiskBasedCertCache::ReadWorker::ReadWorker(
+    disk_cache::Backend* backend,
+    const std::string& key,
+    const base::Callback<void(void)>& cleanup_callback,
+    const GetCallback& user_callback)
+    : backend_(backend),
+      key_(key),
+      entry_(NULL),
+      state_(OPEN),
+      entry_size_(0),
+      cleanup_callback_(cleanup_callback),
+      weak_factory_(this) {
+  io_callback_ =
+      base::Bind(&ReadWorker::OnIOComplete, weak_factory_.GetWeakPtr());
+  AddCallback(user_callback);
+}
+
+void DiskBasedCertCache::ReadWorker::Start() {
+  DoLoop(OK);
+
+  if (state_ == READ_NONE) {
+    if (entry_)
+      entry_->Close();
+    base::ResetAndReturn(&cleanup_callback_).Run();
+  }
+}
+
+void DiskBasedCertCache::ReadWorker::AddCallback(
+    const GetCallback& user_callback) {
+  user_callbacks_.push_back(user_callback);
+}
+
+void DiskBasedCertCache::ReadWorker::OnIOComplete(int rv) {
+  DoLoop(rv);
+
+  if (state_ == READ_NONE) {
+    if (entry_)
+      entry_->Close();
+    base::ResetAndReturn(&cleanup_callback_).Run();
+  }
+}
+
+void DiskBasedCertCache::ReadWorker::DoLoop(int rv) {
+  do {
+    switch (state_) {
+      case OPEN:
+        rv = DoOpen();
+        break;
+      case START_READ:
+        rv = DoStartRead(rv);
+        break;
+      case FINISH_READ:
+        rv = DoFinishRead(rv);
+        break;
+      case READ_NONE:
+        break;
+    }
+  } while (rv != ERR_IO_PENDING && state_ != READ_NONE);
+}
+
+int DiskBasedCertCache::ReadWorker::DoOpen() {
+  state_ = START_READ;
+  return backend_->OpenEntry(key_, &entry_, io_callback_);
+}
+
+int DiskBasedCertCache::ReadWorker::DoStartRead(int rv) {
+  if (rv < 0) {
+    CallCallbacks(NULL);
+    state_ = READ_NONE;
+    return ERR_FAILED;
+  }
+
+  entry_size_ = entry_->GetDataSize(0 /* index */);
+  state_ = FINISH_READ;
+  buffer = new IOBuffer(entry_size_);
+  return entry_->ReadData(
+      0 /* index */, 0 /* offset */, buffer, entry_size_, io_callback_);
+}
+
+int DiskBasedCertCache::ReadWorker::DoFinishRead(int rv) {
+  if (rv < 0) {
+    CallCallbacks(NULL);
+    state_ = READ_NONE;
+    return ERR_FAILED;
+  }
+
+  state_ = READ_NONE;
+
+  X509Certificate::OSCertHandle retrieved_cert_handle =
+      X509Certificate::CreateOSCertHandleFromBytes(buffer->data(), entry_size_);
+
+  DCHECK(retrieved_cert_handle);
+
+  CallCallbacks(retrieved_cert_handle);
+  X509Certificate::FreeOSCertHandle(retrieved_cert_handle);
+  return OK;
+}
+
+void DiskBasedCertCache::ReadWorker::CallCallbacks(
+    X509Certificate::OSCertHandle cert_handle) {
+  for (std::list<GetCallback>::iterator it = user_callbacks_.begin();
+       it != user_callbacks_.end();
+       it++) {
+    base::ResetAndReturn(&(*it)).Run(cert_handle);
+  }
+}
+
+DiskBasedCertCache::ReadWorker::~ReadWorker() {
+}
+
+DiskBasedCertCache::DiskBasedCertCache(disk_cache::Backend* backend)
+    : backend_(backend), weak_factory_(this) {
+  DCHECK(backend_);
+}
+
+DiskBasedCertCache::~DiskBasedCertCache() {
+  STLDeleteContainerPairSecondPointers(read_worker_map_.begin(),
+                                       read_worker_map_.end());
+  STLDeleteContainerPairSecondPointers(write_worker_map_.begin(),
+                                       write_worker_map_.end());
+}
+
+void DiskBasedCertCache::Get(const std::string& key, const GetCallback& cb) {
+  DCHECK(!key.empty());
+
+  ReadWorkerMap::iterator it = read_worker_map_.find(key);
+
+  if (it == read_worker_map_.end()) {
+    std::pair<ReadWorkerMap::iterator, bool> entry = read_worker_map_.insert(
+        make_pair(key,
+                  new ReadWorker(
+                      backend_,
+                      key,
+                      base::Bind(&DiskBasedCertCache::FinishedReadOperation,
+                                 weak_factory_.GetWeakPtr(),
+                                 key),
+                      cb)));
+    DCHECK(entry.second);
+    entry.first->second->Start();
+  } else {
+    it->second->AddCallback(cb);
+  }
+}
+
+void DiskBasedCertCache::Set(const X509Certificate::OSCertHandle cert_handle,
+                             const SetCallback& cb) {
+  DCHECK(!cb.is_null());
+  DCHECK(cert_handle);
+  std::string key = GetCacheKeyToCert(cert_handle);
+
+  WriteWorkerMap::iterator it = write_worker_map_.find(key);
+
+  if (it == write_worker_map_.end()) {
+    std::pair<WriteWorkerMap::iterator, bool> entry = write_worker_map_.insert(
+        make_pair(key,
+                  new WriteWorker(
+                      backend_,
+                      key,
+                      cert_handle,
+                      base::Bind(&DiskBasedCertCache::FinishedWriteOperation,
+                                 weak_factory_.GetWeakPtr(),
+                                 key),
+                      cb)));
+    DCHECK(entry.second);
+    entry.first->second->Start();
+  } else {
+    it->second->AddCallback(cb);
+  }
+}
+
+void DiskBasedCertCache::FinishedWriteOperation(const std::string& key) {
+  WriteWorkerMap::iterator it = write_worker_map_.find(key);
+  delete it->second;
+  write_worker_map_.erase(it);
+}
+
+void DiskBasedCertCache::FinishedReadOperation(const std::string& key) {
+  ReadWorkerMap::iterator it = read_worker_map_.find(key);
+  delete it->second;
+  read_worker_map_.erase(it);
+}
+
+}  // namespace net