Disk Based Certificate Cache Implementation

net/http/disk_based_cert_cache.cc

+// 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 <vector>
+
+#include "base/bind.h"
+#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 {
+
+namespace {
+
+// Used to obtain a unique cache key for a certificate in the form of
+// "cert:<hash>".
+std::string GetCacheKeyToCert(const X509Certificate::OSCertHandle cert_handle) {
+  SHA1HashValue fingerprint =
+      X509Certificate::CalculateFingerprint(cert_handle);
+
+  return "cert:" +
+         base::HexEncode(fingerprint.data, arraysize(fingerprint.data));
+}
+
+}  // namespace
+
+// 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.
+class DiskBasedCertCache::WriteWorker {
+ public:
+  // |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 ReadWorker,
+  // regardless of success or failure.
+  WriteWorker(disk_cache::Backend* backend,
+              const std::string& key,
+              X509Certificate::OSCertHandle cert_handle,
+              const base::Closure& cleanup_callback);
+
+  ~WriteWorker();
+
+  // Writes the given certificate to the cache. On completion, will invoke all
+  // user callbacks.
+  void Start();
+
+  // Adds a callback to the set of callbacks to be run when this
+  // WriteWorker finishes processing.
+  void AddCallback(const SetCallback& user_callback);
+
+  // Signals the WriteWorker to abort early. The WriteWorker will be destroyed
+  // upon the completion of any pending callbacks. User callbacks will be
+  // invoked with an empty string.
+  void Cancel();
+
+ private:
+  enum WriteState {
+    STATE_CREATE,
+    STATE_CREATE_COMPLETE,
+    STATE_OPEN,
+    STATE_OPEN_COMPLETE,
+    STATE_WRITE,
+    STATE_WRITE_COMPLETE,
+    STATE_NONE
+  };
+
+  void OnIOComplete(int rv);
+  int DoLoop(int rv);
+
+  int DoCreate();
+  int DoCreateComplete(int rv);
+  int DoOpen();
+  int DoOpenComplete(int rv);
+  int DoWrite();
+  int DoWriteComplete(int rv);
+
+  void Finish(int rv);
+
+  // Invokes all of the |user_callbacks_|
+  void CallCallbacks(int rv);
+
+  disk_cache::Backend* backend_;
+  const X509Certificate::OSCertHandle cert_handle_;
+  std::string key_;
+  bool canceled_;
+
+  disk_cache::Entry* entry_;
+  WriteState state_;
+  scoped_refptr<IOBuffer> buffer_;
+
+  base::Closure cleanup_callback_;
+  std::vector<SetCallback> user_callbacks_;
+  CompletionCallback io_callback_;
+
+  base::WeakPtrFactory<WriteWorker> weak_factory_;
+};
+
+DiskBasedCertCache::WriteWorker::WriteWorker(
+    disk_cache::Backend* backend,
+    const std::string& key,
+    X509Certificate::OSCertHandle cert_handle,
+    const base::Closure& cleanup_callback)
+    : backend_(backend),
+      cert_handle_(cert_handle),
+      key_(key),
+      canceled_(false),
+      entry_(NULL),
+      state_(STATE_NONE),
+      cleanup_callback_(cleanup_callback),
+      weak_factory_(this) {
+  io_callback_ =
+      base::Bind(&WriteWorker::OnIOComplete, weak_factory_.GetWeakPtr());

[Ryan Sleevi, 2014/06/23 22:43:58]

In the new design, why would io_callback_ ever need to be bound using a WeakPtr?

The WriteWorker should never go out of scope while there is an asynchronous
operation still pending on a given Entry. That's because the Entry (line 115)
has to stay valid for the duration of the asynchronous op.

Did I miss something when reviewing?

+}
+
+void DiskBasedCertCache::WriteWorker::Start() {
+  state_ = STATE_CREATE;

[Ryan Sleevi, 2014/06/23 22:43:57]

You should add a
DCHECK_EQ(STATE_NONE, state_);

This is a form of noting that your precondition is Start() hasn't already been
called for this worker (otherwise, you'd end up in a bad state)

+  int rv = DoLoop(OK);
+
+  if (rv == ERR_IO_PENDING)
+    return;
+
+  Finish(rv);
+}
+
+void DiskBasedCertCache::WriteWorker::AddCallback(
+    const SetCallback& user_callback) {
+  user_callbacks_.push_back(user_callback);
+}
+
+void DiskBasedCertCache::WriteWorker::OnIOComplete(int rv) {
+  if (canceled_) {
+    Finish(ERR_FAILED);
+    return;
+  }
+
+  rv = DoLoop(rv);
+
+  if (rv == ERR_IO_PENDING)
+    return;
+
+  Finish(rv);
+}
+
+int DiskBasedCertCache::WriteWorker::DoLoop(int rv) {
+  do {
+    switch (state_) {
+      case STATE_CREATE:
+        rv = DoCreate();
+        break;
+      case STATE_CREATE_COMPLETE:
+        rv = DoCreateComplete(rv);
+        break;
+      case STATE_OPEN:
+        rv = DoOpen();
+        break;
+      case STATE_OPEN_COMPLETE:
+        rv = DoOpenComplete(rv);
+        break;
+      case STATE_WRITE:
+        rv = DoWrite();
+        break;
+      case STATE_WRITE_COMPLETE:
+        rv = DoWriteComplete(rv);
+        break;
+      case STATE_NONE:
+        break;
+    }
+  } while (rv != ERR_IO_PENDING && state_ != STATE_NONE);

[Ryan Sleevi, 2014/06/23 22:43:58]

In order to prevent endless loops, we typically initialize such loops as

do {
  State next_state = state_;
  state_ = STATE_NONE;
  switch (next_state) {

  }
} while (rv != ERR_IO_PENDING && state_ != STATE_NONE);

This ensures that every state transition (in the Do* series) must explicitly set
the next state before returning. If any of them fail to do so, the loop will
terminate.

This will also avoid you needing to duplicate logic on lines 202, 214, 232, for
example

+
+  return rv;
+}
+
+int DiskBasedCertCache::WriteWorker::DoCreate() {
+  state_ = STATE_CREATE_COMPLETE;
+
+  return backend_->CreateEntry(key_, &entry_, io_callback_);
+}
+
+int DiskBasedCertCache::WriteWorker::DoCreateComplete(int rv) {
+  if (rv < 0) {
+    state_ = STATE_OPEN;

[Ryan Sleevi, 2014/06/23 22:43:57]

It would be good to add a comment about why you transition to STATE_OPEN here

+    return rv;
+  }
+  state_ = STATE_WRITE;

[Ryan Sleevi, 2014/06/23 22:43:57]

We usually add a line break between 191 and 192

+  return OK;
+}
+
+int DiskBasedCertCache::WriteWorker::DoOpen() {
+  return backend_->OpenEntry(key_, &entry_, io_callback_);
+}
+
+int DiskBasedCertCache::WriteWorker::DoOpenComplete(int rv) {
+  if (rv < 0) {
+    state_ = STATE_NONE;
+    return rv;
+  }
+  state_ = STATE_WRITE;
+  return OK;
+}
+
+int DiskBasedCertCache::WriteWorker::DoWrite() {
+  std::string write_data;
+  bool encoded = X509Certificate::GetDEREncoded(cert_handle_, &write_data);
+
+  if (!encoded) {
+    state_ = STATE_NONE;
+    return ERR_FAILED;
+  }
+
+  buffer_ = new IOBuffer(write_data.size());
+  memcpy(buffer_->data(), write_data.data(), write_data.size());
+
+  state_ = STATE_WRITE_COMPLETE;
+
+  return entry_->WriteData(0 /* index */,
+                           0 /* offset */,
+                           buffer_,
+                           write_data.size(),
+                           io_callback_,
+                           true /* truncate */);
+}
+
+int DiskBasedCertCache::WriteWorker::DoWriteComplete(int rv) {
+  state_ = STATE_NONE;
+  return rv;

[Ryan Sleevi, 2014/06/23 22:43:58]

I don't see any documentation that WriteData() will only invoke |io_callback_|
if-and-only-if it writes all of |buf|

It seems like you might have situations where it only writes a portion of the
data, and you (the caller) are expected to increment offset, the buf pointer,
and decrement len.

You should reach out to a net/disk_cache OWNER (aka Ricardo and Gavin), ask
whether or not this is possible, and if so, will need to update DoWriteComplete
so that it sees if it's written all of buffer_ or not.

If you're not sure what this would look like, let me know and we can dig in
together :)

+}
+
+void DiskBasedCertCache::WriteWorker::CallCallbacks(int rv) {

[Ryan Sleevi, 2014/06/23 22:43:57]

naming OCD: Since Run() is the method name on Callbacks, this might be clearer
as RunCallbacks() [or RunUserCallbacks]

+  std::string key;
+  if (rv >= 0)
+    key = key_;
+
+  for (std::vector<SetCallback>::iterator it = user_callbacks_.begin();
+       it != user_callbacks_.end();
+       ++it) {
+    it->Run(key);
+    it->Reset();

[Ryan Sleevi, 2014/06/23 22:43:58]

You don't need to manually be calling Reset() on these.

If you remove that, you can make it a const_iterator, since Run() is a const
method on a Callback (but Reset is not)

+  }
+  user_callbacks_.clear();
+}
+
+void DiskBasedCertCache::WriteWorker::Finish(int rv) {
+  cleanup_callback_.Run();
+  cleanup_callback_.Reset();

[Ryan Sleevi, 2014/06/23 22:43:57]

So it's ok to use base::ResetAndReturn(&cleanup_callback_).Run() here, as a
short-hand.

I'm not sure if Wan-Teh suggested this wasn't necessary, but it's ok to do, even
when you're not running the risk of cleanup_callback_ deleting the object.

With the changed design, it might make more sense to rename this to
finished_callback or something.

+  CallCallbacks(rv);
+  delete this;
+}
+
+void DiskBasedCertCache::WriteWorker::Cancel() {
+  canceled_ = true;
+}
+
+DiskBasedCertCache::WriteWorker::~WriteWorker() {
+  if (entry_)
+    entry_->Close();
+}
+
+// ReadWorkers represent pending Get jobs in the DiskBasedCertCache. Each
+// 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.
+class DiskBasedCertCache::ReadWorker {
+ public:
+  // |backend| is the backend to read |certificate| from, using
+  // |key| as the key for the disk_cache::Entry.
+  // |cleanup_callback| is called to clean up this ReadWorker,
+  // regardless of success or failure.
+  ReadWorker(disk_cache::Backend* backend,
+             const std::string& key,
+             const base::Closure& cleanup_callback);
+
+  ~ReadWorker();
+
+  // Reads the given certificate from the cache. On completion, will invoke all
+  // user callbacks.
+  void Start();
+
+  // Adds a callback to the set of callbacks to be run when this
+  // ReadWorker finishes processing.
+  void AddCallback(const GetCallback& user_callback);
+
+  // Signals the ReadWorker to abort early. The ReadWorker will be destroyed
+  // upon the completion of any pending callbacks. User callbacks will be
+  // invoked with a NULL cert handle.
+  void Cancel();
+
+ private:
+  enum ReadState {
+    STATE_OPEN,
+    STATE_OPEN_COMPLETE,
+    STATE_READ,
+    STATE_READ_COMPLETE,
+    STATE_NONE
+  };
+
+  void OnIOComplete(int rv);
+  int DoLoop(int rv);
+  int DoOpen();
+  int DoOpenComplete(int rv);
+  int DoRead();
+  int DoReadComplete(int rv);
+  void Finish(int rv);
+
+  // Invokes all of |user_callbacks_|
+  void CallCallbacks();
+
+  disk_cache::Backend* backend_;
+  X509Certificate::OSCertHandle cert_handle_;
+  std::string key_;
+  bool canceled_;
+
+  disk_cache::Entry* entry_;
+
+  ReadState state_;
+  int entry_size_;
+  scoped_refptr<IOBuffer> buffer_;
+
+  base::Closure cleanup_callback_;
+  std::vector<GetCallback> user_callbacks_;
+  CompletionCallback io_callback_;
+  base::WeakPtrFactory<ReadWorker> weak_factory_;
+};
+
+DiskBasedCertCache::ReadWorker::ReadWorker(
+    disk_cache::Backend* backend,
+    const std::string& key,
+    const base::Closure& cleanup_callback)
+    : backend_(backend),
+      cert_handle_(NULL),
+      key_(key),
+      canceled_(false),
+      entry_(NULL),
+      state_(STATE_NONE),
+      entry_size_(0),
+      cleanup_callback_(cleanup_callback),
+      weak_factory_(this) {
+  io_callback_ =
+      base::Bind(&ReadWorker::OnIOComplete, weak_factory_.GetWeakPtr());
+}
+
+void DiskBasedCertCache::ReadWorker::Start() {
+  state_ = STATE_OPEN;
+  int rv = DoLoop(OK);
+
+  if (rv == ERR_IO_PENDING)
+    return;
+
+  Finish(rv);
+}
+
+void DiskBasedCertCache::ReadWorker::AddCallback(
+    const GetCallback& user_callback) {
+  user_callbacks_.push_back(user_callback);
+}
+
+void DiskBasedCertCache::ReadWorker::OnIOComplete(int rv) {
+  if (canceled_) {
+    Finish(ERR_FAILED);
+    return;
+  }
+
+  rv = DoLoop(rv);
+
+  if (rv == ERR_IO_PENDING)
+    return;
+
+  Finish(rv);
+}
+
+int DiskBasedCertCache::ReadWorker::DoLoop(int rv) {
+  do {
+    switch (state_) {
+      case STATE_OPEN:
+        rv = DoOpen();
+        break;
+      case STATE_OPEN_COMPLETE:
+        rv = DoOpenComplete(rv);
+        break;
+      case STATE_READ:
+        rv = DoRead();
+        break;
+      case STATE_READ_COMPLETE:
+        rv = DoReadComplete(rv);
+        break;
+      case STATE_NONE:
+        break;
+    }
+  } while (rv != ERR_IO_PENDING && state_ != STATE_NONE);
+
+  return rv;
+}
+
+int DiskBasedCertCache::ReadWorker::DoOpen() {
+  state_ = STATE_OPEN_COMPLETE;
+  return backend_->OpenEntry(key_, &entry_, io_callback_);
+}
+
+int DiskBasedCertCache::ReadWorker::DoOpenComplete(int rv) {
+  if (rv < 0) {
+    state_ = STATE_NONE;
+    return rv;
+  }
+  state_ = STATE_READ;
+  return OK;
+}
+
+int DiskBasedCertCache::ReadWorker::DoRead() {
+  state_ = STATE_READ_COMPLETE;
+  entry_size_ = entry_->GetDataSize(0 /* index */);
+  buffer_ = new IOBuffer(entry_size_);
+  return entry_->ReadData(
+      0 /* index */, 0 /* offset */, buffer_, entry_size_, io_callback_);
+}
+
+int DiskBasedCertCache::ReadWorker::DoReadComplete(int rv) {
+  state_ = STATE_NONE;
+  if (rv < 0)
+    return rv;
+
+  cert_handle_ = X509Certificate::CreateOSCertHandleFromBytes(buffer_->data(),
+                                                              entry_size_);
+
+  DCHECK(cert_handle_);

[wtc, 2014/06/24 00:21:04]

Replace this DCHECK with error handling:
  if (!cert_handle_)
    return ERR_FAILED;

We should not assume a system call will succeed.

+  return OK;
+}
+
+void DiskBasedCertCache::ReadWorker::CallCallbacks() {
+  for (std::vector<GetCallback>::iterator it = user_callbacks_.begin();
+       it != user_callbacks_.end();
+       ++it) {
+    it->Run(cert_handle_);
+    it->Reset();
+  }
+  user_callbacks_.clear();
+}
+
+void DiskBasedCertCache::ReadWorker::Finish(int rv) {
+  cleanup_callback_.Run();
+  cleanup_callback_.Reset();
+  CallCallbacks();
+
+  delete this;
+}
+
+void DiskBasedCertCache::ReadWorker::Cancel() {
+  canceled_ = true;
+}
+
+DiskBasedCertCache::ReadWorker::~ReadWorker() {
+  if (entry_)
+    entry_->Close();
+  if (cert_handle_)
+    X509Certificate::FreeOSCertHandle(cert_handle_);
+}
+
+DiskBasedCertCache::DiskBasedCertCache(disk_cache::Backend* backend)
+    : backend_(backend), weak_factory_(this) {
+  DCHECK(backend_);
+}
+
+DiskBasedCertCache::~DiskBasedCertCache() {
+  for (WriteWorkerMap::iterator it = write_worker_map_.begin();
+       it != write_worker_map_.end();
+       ++it) {
+    it->second->Cancel();
+  }
+  for (ReadWorkerMap::iterator it = read_worker_map_.begin();
+       it != read_worker_map_.end();
+       ++it) {
+    it->second->Cancel();
+  }
+}
+
+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()) {
+    ReadWorker* worker =
+        new ReadWorker(backend_,
+                       key,
+                       base::Bind(&DiskBasedCertCache::FinishedReadOperation,
+                                  weak_factory_.GetWeakPtr(),
+                                  key));
+    read_worker_map_[key] = worker;
+    worker->AddCallback(cb);
+    worker->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()) {
+    WriteWorker* worker =
+        new WriteWorker(backend_,
+                        key,
+                        cert_handle,
+                        base::Bind(&DiskBasedCertCache::FinishedWriteOperation,
+                                   weak_factory_.GetWeakPtr(),
+                                   key));
+    write_worker_map_[key] = worker;
+    worker->AddCallback(cb);
+    worker->Start();
+  } else {
+    it->second->AddCallback(cb);
+  }
+}
+
+void DiskBasedCertCache::FinishedWriteOperation(const std::string& key) {
+  write_worker_map_.erase(key);
+}
+
+void DiskBasedCertCache::FinishedReadOperation(const std::string& key) {
+  read_worker_map_.erase(key);
+}
+
+}  // namespace net