Disk Based Certificate Cache Implementation

net/cert_cache/disk_based_cert_cache.cc

Index: net/cert_cache/disk_based_cert_cache.cc
diff --git a/net/cert_cache/disk_based_cert_cache.cc b/net/cert_cache/disk_based_cert_cache.cc
new file mode 100644
index 0000000000000000000000000000000000000000..486b99d70f950b296677861b1876e4a4029bea4d
--- /dev/null
+++ b/net/cert_cache/disk_based_cert_cache.cc
@@ -0,0 +1,170 @@
+// 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/cert_cache/disk_based_cert_cache.h"
+
+#include <string>
+
+#include "base/bind.h"
+#include "base/callback.h"
+#include "base/memory/ref_counted.h"
+#include "base/memory/weak_ptr.h"
+#include "base/pickle.h"
+#include "base/strings/string_number_conversions.h"
+#include "net/base/io_buffer.h"
+#include "net/base/net_errors.h"
+#include "net/cert/x509_certificate.h"
+#include "net/disk_cache/disk_cache.h"
+
+namespace net {
+
+DiskBasedCertCache::DiskBasedCertCache(disk_cache::Backend* backend)
+    : backend_(backend),
+      active_entry_(NULL),
+      weak_factory_(this),
+      io_callback_(base::Bind(&DiskBasedCertCache::OnIOComplete,
+                              weak_factory_.GetWeakPtr())),
+      state_(NONE) {
+  DCHECK(backend_);
+}
+
+DiskBasedCertCache::~DiskBasedCertCache() {
+  weak_factory_.InvalidateWeakPtrs();
+}
+
+void DiskBasedCertCache::Get(
+    std::string& key,
+    base::Callback<void(X509Certificate::OSCertHandle cert_handle)> cb) {
+  user_read_callback_ = cb;
+
+  state_ = START_READ;
+
+  int rv = backend_->OpenEntry(key, &active_entry_, io_callback_);
+
+  // todo: implement other error codes.
+  if (rv >= 0) {
+    DoStartRead();
+  } else if (rv == ERR_IO_PENDING) {
+  } else {
+  }

[Ryan Sleevi, 2014/06/11 22:00:08]

A couple common coding conventions:

Handle errors first, so you can short-circuit early

if (rv == ERR_IO_PENDING)
  return;
if (rv < 0) {
  // TODO(brandonsalmon): implement other error codes.
  NOTIMPLEMENTED();
}

DoStartRead();


The use of NOTIMPLEMENTED is handy, because it will make you crash when you hit
it :)

+}
+
+void DiskBasedCertCache::DoStartRead() {
+  active_entry_size_ = active_entry_->GetDataSize(0 /* index */);
+  buffer = (new IOBuffer(active_entry_size_));

[Ryan Sleevi, 2014/06/11 22:00:08]

No need for the extra () around new.

+
+  state_ = FINISH_READ;
+
+  int rv = active_entry_->ReadData(
+      0 /* index */, 0 /* offset */, buffer, active_entry_size_, io_callback_);
+
+  // todo: implement other error codes.
+  if (rv >= 0) {
+    DoFinishRead();
+  } else if (rv == ERR_IO_PENDING) {
+  } else {
+  }
+}
+
+void DiskBasedCertCache::DoFinishRead() {
+  if (!user_read_callback_.is_null()) {

[Ryan Sleevi, 2014/06/11 22:00:08]

if (user_read_callback_.is_null())
  return;

active_cert_handle_ ...

+    active_cert_handle_ = X509Certificate::CreateOSCertHandleFromBytes(
+        buffer->data(), active_entry_size_);
+
+    CHECK(active_cert_handle_ != NULL);

[Ryan Sleevi, 2014/06/11 22:00:08]

CHECK(active_cert_handle_);

+
+    base::Callback<void(X509Certificate::OSCertHandle cert_handle)> callback =
+        user_read_callback_;
+    callback.Run(active_cert_handle_);

[Ryan Sleevi, 2014/06/11 22:00:08]

base::ResetAndReturn(&user_read_callback_).Run(active_cert_handle_);

+    reset_state();

[Ryan Sleevi, 2014/06/11 22:00:08]

It looks like your state machine breaks if user_read_callback_ is null - that
is, they can read one cert, but never read any others?

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

[Ryan Sleevi, 2014/06/11 22:00:08]

CHECK(active_entry_)

+
+  active_cert_handle_ = cert_handle;
+
+  state_ = START_WRITE;
+  user_write_callback_ = cb;
+
+  if (backend_->CreateEntry(Key(), &active_entry_, io_callback_) !=
+      ERR_IO_PENDING) {
+    DoStartWrite();

[Ryan Sleevi, 2014/06/11 22:00:08]

what about errors?

+  }
+}
+
+std::string DiskBasedCertCache::Key() {
+  CHECK(active_cert_handle_ != NULL);

[Ryan Sleevi, 2014/06/11 22:00:08]

CHECK(active_cert_handle_);

+
+  SHA1HashValue fingerprint =
+      X509Certificate::CalculateFingerprint(active_cert_handle_);
+
+  // should update to store the key so the data doesn't have to be encoded
+  // multiple times.
+
+  return "cert:" + base::HexEncode(fingerprint.data, 20);
+}
+
+void DiskBasedCertCache::OnIOComplete(int rv) {
+  switch (state_) {
+    case START_WRITE:
+      DoStartWrite();
+      break;
+    case FINISH_WRITE:
+      DoFinishWrite();
+      break;
+    case START_READ:
+      DoStartRead();
+      break;
+    case FINISH_READ:
+      break;
+      DoFinishRead();
+    case NONE:
+      break;
+  }
+
+  state_ = NONE;
+}
+
+void DiskBasedCertCache::DoStartWrite() {
+  std::string write_data;
+
+  // encode data
+  CHECK(X509Certificate::GetDEREncoded(active_cert_handle_, &write_data));

[Ryan Sleevi, 2014/06/11 22:00:08]

DANGER: You never want to put function calls in CHECK/DCHECK, as they may be
optimized out depending on build flags.

Instead
bool encoded = X509Certificate::GetDEREncoded(...);
CHECK(encoded);

However, using CHECK here would be inappropriate - it returns bool because it
CAN fail. So you shouldn't CHECK, but should handle/return.

+
+  buffer = new IOBuffer(write_data.size());
+  memcpy(buffer->data(), write_data.data(), write_data.size());
+
+  state_ = FINISH_WRITE;
+
+  if (active_entry_->WriteData(0 /* index */,
+                               0 /* offset */,
+                               buffer,
+                               write_data.size(),
+                               io_callback_,
+                               true /* truncate */) != ERR_IO_PENDING)
+    DoFinishWrite();
+}
+
+void DiskBasedCertCache::DoFinishWrite() {
+  if (!user_write_callback_.is_null()) {

[Ryan Sleevi, 2014/06/11 22:00:08]

if (user_write_callback_.is_null())
  return;
...

+    base::Callback<void(const std::string&)> callback = user_write_callback_;
+    callback.Run(Key());
+    reset_state();
+  }
+}
+
+void DiskBasedCertCache::reset_state() {

[Ryan Sleevi, 2014/06/11 22:00:08]

Naming: This would be ResetState(), not reset_state(), because it's not a
'trivial, inlined function'.

+  state_ = NONE;
+  active_entry_->Close();
+  active_entry_ = NULL;
+  active_cert_handle_ = NULL;
+  user_write_callback_.Reset();
+  user_read_callback_.Reset();
+}
+
+}  // namespace net