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..55a5b2f542347266fec4f968fd1904eb8ae85762
--- /dev/null
+++ b/net/http/disk_based_cert_cache.cc
@@ -0,0 +1,499 @@
+// 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 {
+std::string GetCacheKeyToCert(

[wtc, 2014/06/20 01:27:06]

Nit: document this function.

+    const net::X509Certificate::OSCertHandle cert_handle) {
+  net::SHA1HashValue fingerprint =
+      net::X509Certificate::CalculateFingerprint(cert_handle);

[wtc, 2014/06/20 01:27:06]

Nit: you can nest the unnamed namespace inside the net namespace, so that you
can omit net::.

+
+  return "cert:" +
+         base::HexEncode(fingerprint.data, arraysize(fingerprint.data));
+}
+}  // namespace

[wtc, 2014/06/20 01:27:06]

Nit: I usually add a blank line after

  namespace {

and a blank line before the closing

  } // namespace

+
+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.
+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,
+              const 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 notified when this
+  // WriteWorker finishes processing.
+  void AddCallback(const SetCallback& user_callback);
+
+  void Cancel();
+
+ private:
+  enum WriteState {
+    CREATE_OR_OPEN,
+    FINISH_CREATE_OR_OPEN,
+    START_WRITE,
+    FINISH_WRITE,
+    WRITE_NONE

[wtc, 2014/06/20 01:27:06]

I suggest adding STATE_ prefix to these states.

+  };
+
+  void OnIOComplete(int rv);
+  int DoLoop(int rv);
+  int DoCreateOrOpen();
+  int DoFinishCreateOrOpen(int rv);
+  int DoStartWrite();
+  int DoFinishWrite(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_;
+  bool create_failed_;
+  scoped_refptr<IOBuffer> buffer;
+
+  base::Closure cleanup_callback_;
+  std::vector<SetCallback> user_callbacks_;
+  CompletionCallback io_callback_;
+
+  base::WeakPtrFactory<WriteWorker> weak_factory_;
+};

[wtc, 2014/06/20 01:27:06]

Please move the WriteWorker method definitions here, unless Ryan suggested that
you declare the two *Worker classes first.

+
+// 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 notified when this
+  // ReadWorker finishes processing.
+  void AddCallback(const GetCallback& user_callback);
+
+  void Cancel();
+
+ private:
+  enum ReadState { OPEN, START_READ, FINISH_READ, READ_NONE };

[wtc, 2014/06/20 01:27:06]

Nit: I suggest adding STATE_ prefix to these states.

Nit: list one state per line. That's our convention.

+
+  void OnIOComplete(int rv);
+  int DoLoop(int rv);
+  int DoOpen();
+  int DoStartRead(int rv);
+  int DoFinishRead(int rv);
+  void Finish(int rv);
+
+  // invokes all of |user_callbacks_|
+  void CallCallbacks(int rv);
+
+  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::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_(CREATE_OR_OPEN),
+      create_failed_(false),
+      cleanup_callback_(cleanup_callback),
+      weak_factory_(this) {
+  io_callback_ =
+      base::Bind(&WriteWorker::OnIOComplete, weak_factory_.GetWeakPtr());
+}
+
+void DiskBasedCertCache::WriteWorker::Start() {
+  int rv = DoLoop(OK);
+
+  if (state_ != WRITE_NONE)
+    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 (state_ != WRITE_NONE)
+    return;
+
+  Finish(rv);
+}
+
+int 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);
+
+  return rv;
+}
+
+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_) {

[wtc, 2014/06/20 01:27:06]

We should test |rv < 0| instead of |rv == ERR_FAILED|, unless |rv| may be
ERR_IO_PENDING here.

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

[wtc, 2014/06/20 01:27:06]

Nit: In general, omit "else" if the previous block ends with a return statement.

+    state_ = WRITE_NONE;
+    return ERR_FAILED;

[wtc, 2014/06/20 01:27:07]

IMPORTANT: in general we should report an accurate error code. Here a negative
|rv| is the error code, so we should just return |rv|.

If you only use ERR_FAILED in this class, essentially only indicating a failure
status, then that's fine.

+  }
+
+  state_ = START_WRITE;
+  return OK;
+}
+
+int DiskBasedCertCache::WriteWorker::DoStartWrite() {
+  std::string write_data;
+  bool encoded = X509Certificate::GetDEREncoded(cert_handle_, &write_data);
+
+  if (!encoded) {
+    state_ = WRITE_NONE;
+    return ERR_FAILED;
+  }
+
+  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) {
+    state_ = WRITE_NONE;
+    return ERR_FAILED;
+  }
+
+  state_ = WRITE_NONE;
+
+  return OK;
+}
+
+void DiskBasedCertCache::WriteWorker::CallCallbacks(int rv) {
+  for (std::vector<SetCallback>::iterator it = user_callbacks_.begin();
+       it != user_callbacks_.end();
+       it++) {

[wtc, 2014/06/20 01:27:06]

Use ++it instead of it++ to increment an iterator, if we don't need the return
value of the increment operator. The pre-increment operator returns a reference
whereas the post-increment operator returns a copy, so the pre-increment
operator is cheaper. (This is a common test for a C++ programmer's proficiency.)

+    base::ResetAndReturn(&(*it)).Run((rv >= 0) ? key_ : std::string());

[wtc, 2014/06/20 01:27:06]

The use of base::ResetAndReturn is wrong.

The purpose of base::ResetAndReturn is to set a callback_ member to null and
then run the callback. Here the relevant member is user_callbacks_. So we need
to clear the entire user_callbacks_ vector. We'll need to do that manually. I
believe we usually use swap() to do this:

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

Note how I set |key| outside the for loop.

[brandonsalmon, 2014/06/20 18:07:23]

Actually, is it even necessary to do this if it is guaranteed that the callbacks
cannot delete the worker?

+  }
+}
+
+void DiskBasedCertCache::WriteWorker::Finish(int rv) {
+  if (entry_)
+    entry_->Close();

[wtc, 2014/06/20 01:27:06]

This kind of cleanup is conventionally done in the destructor.

+  base::ResetAndReturn(&cleanup_callback_).Run();
+  CallCallbacks(rv);
+  delete this;

[wtc, 2014/06/20 01:27:06]

IMPORTANT: the Finish() method must be written very carefully.

I think it should first remove the WriteWorker from the write_worker_map_. So we
should run cleanup_callback_ first. This way this WriterWorker is no longer
reachable from the DiskBasedCertCache.

Then it is safe to run the user_callbacks_, which is arbitrary code. The user
callbacks cannot possibly reach this WriterWorker now.

Finally, delete this.

With this analysis, I believe it is NOT necessary to use base::ResetAndReturn.

+}
+
+void DiskBasedCertCache::WriteWorker::Cancel() {
+  canceled_ = true;
+}
+
+DiskBasedCertCache::WriteWorker::~WriteWorker() {
+}
+
+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_(OPEN),
+      entry_size_(0),
+      cleanup_callback_(cleanup_callback),
+      weak_factory_(this) {
+  io_callback_ =
+      base::Bind(&ReadWorker::OnIOComplete, weak_factory_.GetWeakPtr());

[wtc, 2014/06/20 01:27:06]

Nit: you should be able to initialize io_callback_ in the initializer list if
you declare the io_callback_ member after the weak_factory_ member.

[brandonsalmon, 2014/06/20 18:07:23]

I'm going to leave this for now. I was talking with Ryan and he said that weak
factories should always be last in initialization lists. (although I'm not sure
of the reason.)

+}
+
+void DiskBasedCertCache::ReadWorker::Start() {
+  int rv = DoLoop(OK);
+
+  if (state_ != READ_NONE)
+    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 (state_ != READ_NONE)
+    return;
+
+  Finish(rv);
+}
+
+int 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);
+
+  return rv;
+}
+
+int DiskBasedCertCache::ReadWorker::DoOpen() {
+  state_ = START_READ;
+  return backend_->OpenEntry(key_, &entry_, io_callback_);
+}
+
+int DiskBasedCertCache::ReadWorker::DoStartRead(int rv) {
+  if (rv < 0) {
+    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) {
+    state_ = READ_NONE;
+    return ERR_FAILED;
+  }
+
+  state_ = READ_NONE;
+
+  cert_handle_ =
+      X509Certificate::CreateOSCertHandleFromBytes(buffer->data(), entry_size_);
+
+  DCHECK(cert_handle_);
+  return OK;
+}
+
+void DiskBasedCertCache::ReadWorker::CallCallbacks(int rv) {
+  for (std::vector<GetCallback>::iterator it = user_callbacks_.begin();
+       it != user_callbacks_.end();
+       it++) {
+    base::ResetAndReturn(&(*it)).Run((rv >= 0) ? cert_handle_ : NULL);
+  }
+}
+
+void DiskBasedCertCache::ReadWorker::Finish(int rv) {
+  base::ResetAndReturn(&cleanup_callback_).Run();
+  CallCallbacks(rv);
+
+  if (entry_)
+    entry_->Close();
+  if (cert_handle_)
+    X509Certificate::FreeOSCertHandle(cert_handle_);
+  delete this;
+}
+
+void DiskBasedCertCache::ReadWorker::Cancel() {
+  canceled_ = true;
+}
+
+DiskBasedCertCache::ReadWorker::~ReadWorker() {
+}
+
+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()) {
+    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))));
+    DCHECK(entry.second);
+    entry.first->second->AddCallback(cb);
+    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(

[wtc, 2014/06/20 01:27:06]

Here, you can just use the associative-array notation:

  WriteWorker* worker = new WriteWorker(...);
  write_worker_map_[key] = worker;
  worker->AddCallback(cb);
  worker->Start();

+        make_pair(key,
+                  new WriteWorker(
+                      backend_,
+                      key,
+                      cert_handle,
+                      base::Bind(&DiskBasedCertCache::FinishedWriteOperation,
+                                 weak_factory_.GetWeakPtr(),
+                                 key))));
+    DCHECK(entry.second);
+    entry.first->second->AddCallback(cb);
+    entry.first->second->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