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/callback_helpers.h"
+#include "base/memory/ref_counted.h"
+#include "base/strings/string_number_conversions.h"
+#include "net/base/io_buffer.h"
+#include "net/base/net_errors.h"
+
+namespace net {
+
+//-----------------------------------------------------------------------------

[Ryan Sleevi, 2014/06/17 00:00:20]

vertical whitespace comments apply

+
+class DiskBasedCertCache::WriteWorker {

[Ryan Sleevi, 2014/06/17 00:00:20]

Document

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

[Ryan Sleevi, 2014/06/17 00:00:20]

const-refs

+
+  ~WriteWorker();
+
+  void Start();

[Ryan Sleevi, 2014/06/17 00:00:20]

Document

+
+  void AddCallback(SetCallback user_callback);

[Ryan Sleevi, 2014/06/17 00:00:19]

const-ref
document

+
+ private:
+  // Types --------------------------------------------------------------------
+  enum WriteState {
+    CREATE_OR_OPEN,
+    FINISH_CREATE_OR_OPEN,
+    START_WRITE,
+    FINISH_WRITE,
+    WRITE_NONE
+  };
+
+  // Methods ------------------------------------------------------------------
+
+  void OnIOComplete(int rv);
+  void DoLoop(int rv);
+  int DoCreateOrOpen();
+  int DoFinishCreateOrOpen(int rv);
+  int DoStartWrite();
+  int DoFinishWrite(int rv);
+  void CallCallbacks(const std::string& key);
+
+  // Variables ----------------------------------------------------------------
+  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(const std::string&)> cleanup_callback_;
+  std::vector<SetCallback> user_callbacks_;
+  CompletionCallback io_callback_;
+
+  base::WeakPtrFactory<WriteWorker> weak_factory_;
+};
+
+DiskBasedCertCache::WriteWorker::WriteWorker(
+    disk_cache::Backend* backend,
+    std::string key,
+    X509Certificate::OSCertHandle cert_handle,
+    base::Callback<void(const std::string&)> cleanup_callback,
+    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);
+}
+
+void DiskBasedCertCache::WriteWorker::AddCallback(SetCallback user_callback) {
+  user_callbacks_.push_back(user_callback);
+}
+
+void DiskBasedCertCache::WriteWorker::OnIOComplete(int rv) {
+  DoLoop(rv);
+}
+
+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);
+
+  if (state_ == WRITE_NONE) {

[Ryan Sleevi, 2014/06/17 00:00:19]

STYLE: We prefer error-handling/short circuiting as soon as possible

if (state_ != WRITE_NONE)
  return;

if (entry)
  entry->Close();
base::ResetAndReturn(...)

+    if (entry_)
+      entry_->Close();
+    base::ResetAndReturn(&cleanup_callback_).Run(key_);

[Ryan Sleevi, 2014/06/17 00:00:20]

DANGER: We almost always try to avoid calling callbacks as part of DoLoop, since
callbacks may end in the object being deleted, and DoLoop is never an "entry"
function (eg: the result of a callback)

In this case, your two entry functions are Start and OnIOComplete. Those are the
places where you'd want to chain to your next callback

+  }
+}
+
+int DiskBasedCertCache::WriteWorker::DoCreateOrOpen() {
+  DCHECK(entry_ == NULL);

[Ryan Sleevi, 2014/06/17 00:00:20]

For == NULL dchecks, just do

DCHECK(entry_)

+
+  state_ = FINISH_CREATE_OR_OPEN;
+
+  if (create_failed_) {
+    return backend_->OpenEntry(key_, &entry_, io_callback_);
+  }

[Ryan Sleevi, 2014/06/17 00:00:19]

No braces on simple { }

+
+  return backend_->CreateEntry(key_, &entry_, io_callback_);
+}
+
+int DiskBasedCertCache::WriteWorker::DoFinishCreateOrOpen(int rv) {
+  // ERR_FAILED implies create entry failed, and we should try opening instead.
+  //!create_failed is checked to make sure we only try to open once.

[Ryan Sleevi, 2014/06/17 00:00:19]

When possible, it's better to avoid pronouns (like "we") in comments. The
reasoning is listed on
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/NH-S6KCkr2M/yv0z3...

You're also missing a space in //!create_failed (it should be "// create_failed)

+  if (rv == ERR_FAILED && !create_failed_) {
+    create_failed_ = true;
+    state_ = CREATE_OR_OPEN;
+    return OK;
+  } else if (rv < 0) {
+    CallCallbacks("");
+    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(NULL);
+    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) {
+    CallCallbacks("");

[Ryan Sleevi, 2014/06/17 00:00:20]

use std::string() for empty strings, rather than ""

Believe it or not, it avoids an extra memory copy :)

+    state_ = WRITE_NONE;
+    return ERR_FAILED;
+  }
+
+  state_ = WRITE_NONE;
+
+  CallCallbacks(key_);
+  return OK;
+}
+
+void DiskBasedCertCache::WriteWorker::CallCallbacks(const std::string& key) {
+  for (std::vector<SetCallback>::iterator it = user_callbacks_.begin();
+       it != user_callbacks_.end();
+       it++) {
+    if (!it->is_null())
+      base::ResetAndReturn(&(*it)).Run(key_);

[Ryan Sleevi, 2014/06/17 00:00:20]

1) When would it->is_null be true? Seems like they'd be violating your API
contract

2) Can any of these callbacks delete the cache? If so, it seems like deleting
the cache would delete the worker, which would delete this, which would cause
things to blow up?

3) What if someone, as a result of CallCallbacks, attempted to start a new Write
operation? AFAICT, you'd invoke AddCallback(), which would mutate
|user_callbacks_|, thus invalidating |it| and causing things to crash!

+  }
+}
+
+DiskBasedCertCache::WriteWorker::~WriteWorker() {
+  weak_factory_.InvalidateWeakPtrs();

[Ryan Sleevi, 2014/06/17 00:00:19]

You don't need to do this explicitly. WeakPtrFactory's destructor invalidates
its issued weak pointers.

+}
+
+//---------------------------------------------------------------------------
+
+class DiskBasedCertCache::ReadWorker {
+ public:
+  ReadWorker(disk_cache::Backend* backend,
+             std::string key,
+             base::Callback<void(const std::string&)> cleanup_callback,
+             GetCallback user_callback);
+
+  ~ReadWorker();
+
+  void Start();
+
+  void AddCallback(GetCallback user_callback);

[Ryan Sleevi, 2014/06/17 00:00:19]

Same comments regarding const-refs, comments, etc

+
+ private:
+  // Types --------------------------------------------------------------------

[Ryan Sleevi, 2014/06/17 00:00:20]

Same comments regarding vertical whitespace (eg: don't include these block
markers)

+  enum ReadState { OPEN, START_READ, FINISH_READ, READ_NONE };
+
+  // Methods ------------------------------------------------------------------
+  void OnIOComplete(int rv);
+  void DoLoop(int rv);
+  int DoOpen();
+  int DoStartRead(int rv);
+  int DoFinishRead(int rv);
+
+  void CallCallbacks(X509Certificate::OSCertHandle cert_handle);
+
+  // Variables ----------------------------------------------------------------
+  disk_cache::Backend* backend_;
+
+  std::string key_;
+  disk_cache::Entry* entry_;
+  ReadState state_;
+  int entry_size_;
+  scoped_refptr<IOBuffer> buffer;
+
+  base::Callback<void(const std::string&)> cleanup_callback_;
+  std::vector<GetCallback> user_callbacks_;
+  CompletionCallback io_callback_;
+  base::WeakPtrFactory<ReadWorker> weak_factory_;
+};
+
+DiskBasedCertCache::ReadWorker::ReadWorker(
+    disk_cache::Backend* backend,
+    std::string key,
+    base::Callback<void(const std::string&)> cleanup_callback,
+    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);
+}
+
+void DiskBasedCertCache::ReadWorker::AddCallback(GetCallback user_callback) {
+  user_callbacks_.push_back(user_callback);
+}
+
+void DiskBasedCertCache::ReadWorker::OnIOComplete(int rv) {
+  DoLoop(rv);
+}
+
+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);
+
+  if (state_ == READ_NONE) {
+    if (entry_)
+      entry_->Close();
+    base::ResetAndReturn(&cleanup_callback_).Run(key_);

[Ryan Sleevi, 2014/06/17 00:00:19]

Same comments re: entry functions calling callbacks

+  }
+}
+
+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_);

[Ryan Sleevi, 2014/06/17 00:00:20]

Lots of unnecessary vertical whitespace here (and 304, 320)

+
+  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_);
+
+  CHECK(retrieved_cert_handle);
+  CallCallbacks(retrieved_cert_handle);
+  X509Certificate::FreeOSCertHandle(retrieved_cert_handle);
+  return OK;
+}
+
+void DiskBasedCertCache::ReadWorker::CallCallbacks(
+    X509Certificate::OSCertHandle cert_handle) {
+  for (std::vector<GetCallback>::iterator it = user_callbacks_.begin();
+       it != user_callbacks_.end();
+       it++) {
+    if (!it->is_null())
+      base::ResetAndReturn(&(*it)).Run(cert_handle);
+  }
+}
+
+DiskBasedCertCache::ReadWorker::~ReadWorker() {
+  weak_factory_.InvalidateWeakPtrs();
+}
+
+//-----------------------------------------------------------------------------
+
+DiskBasedCertCache::DiskBasedCertCache(disk_cache::Backend* backend)
+    : backend_(backend), weak_factory_(this) {

[Ryan Sleevi, 2014/06/17 00:00:19]

Run git-cl format, and I believe it will restructure this as

: backend_(backend),
  weak_factory_(this) {

}

The one-liner is only supposed to be valid if the entire ctor fits on one line,
I believe

+  DCHECK(backend_);
+  write_io_callback_ = base::Bind(&DiskBasedCertCache::FinishedWriteOperation,
+                                  weak_factory_.GetWeakPtr());
+  read_io_callback_ = base::Bind(&DiskBasedCertCache::FinishedReadOperation,
+                                 weak_factory_.GetWeakPtr());

[Ryan Sleevi, 2014/06/17 00:00:20]

You can create these two callbacks in the ctor initializer list, which is more
idiomatic in chrome code

+}
+
+DiskBasedCertCache::~DiskBasedCertCache() {
+  weak_factory_.InvalidateWeakPtrs();
+  for (WriteWorkerMap::iterator it = write_worker_map_.begin();
+       it != write_worker_map_.end();
+       it++) {
+    delete it->second;
+  }

[Ryan Sleevi, 2014/06/17 00:00:19]

You can use base/stl_util.h

STLDeleteContainerPairSecondPointers(write_worker_map_.begin(),
write_worker_map_.end());

+  for (ReadWorkerMap::iterator it = read_worker_map_.begin();
+       it != read_worker_map_.end();
+       it++) {
+    delete it->second;
+  }
+}
+
+void DiskBasedCertCache::Get(
+    std::string& key,
+    base::Callback<void(X509Certificate::OSCertHandle cert_handle)> cb) {
+  CHECK(!key.empty());

[Ryan Sleevi, 2014/06/17 00:00:19]

Use DCHECK() when defining preconditions/post-conditions for API usage.

CHECK appears in final Chrome binaries (and thus contributes to size) and should
only be used for security-sensitive aspects; otherwise, prefer DCHECK.

+
+  ReadWorkerMap::iterator it = read_worker_map_.find(key);
+
+  if (it == read_worker_map_.end()) {
+    read_worker_map_[key] =
+        new ReadWorker(backend_, key, read_io_callback_, cb);
+    read_worker_map_[key]->Start();

[Ryan Sleevi, 2014/06/17 00:00:19]

Avoid duplicate accesses to [key], when possible.

Here, you'd use std::map::insert() with key to get an
std::pair<maptype::iterator, bool>. You'd then

pair->first->second->Start();

+  } else {
+    read_worker_map_[key]->AddCallback(cb);

[Ryan Sleevi, 2014/06/17 00:00:20]

Here, you'd it->second->AddCallback(cb);

+  }
+}
+
+void DiskBasedCertCache::Set(const X509Certificate::OSCertHandle cert_handle,
+                             base::Callback<void(const std::string&)> cb) {
+  CHECK(!cb.is_null());
+  CHECK(cert_handle);

[Ryan Sleevi, 2014/06/17 00:00:20]

ditto comments re: DCHECK

+  std::string key = Key(cert_handle);
+
+  WriteWorkerMap::iterator it = write_worker_map_.find(key);
+
+  if (it == write_worker_map_.end()) {
+    write_worker_map_[key] =
+        new WriteWorker(backend_, key, cert_handle, write_io_callback_, cb);
+    write_worker_map_[key]->Start();
+  } else {
+    write_worker_map_[key]->AddCallback(cb);
+  }
+}
+
+std::string DiskBasedCertCache::Key(
+    const X509Certificate::OSCertHandle cert_handle) const {
+  SHA1HashValue fingerprint =
+      X509Certificate::CalculateFingerprint(cert_handle);
+
+  return "cert:" +
+         base::HexEncode(fingerprint.data, arraysize(fingerprint.data));
+}
+
+void DiskBasedCertCache::FinishedWriteOperation(const std::string& key) {
+  WriteWorkerMap::iterator it = write_worker_map_.find(key);
+  WriteWorker* temp = it->second;
+  write_worker_map_.erase(it);
+  delete temp;
+}
+
+void DiskBasedCertCache::FinishedReadOperation(const std::string& key) {
+  ReadWorkerMap::iterator it = read_worker_map_.find(key);
+  ReadWorker* temp = it->second;
+  read_worker_map_.erase(it);
+  delete temp;
+}
+
+}  // namespace net