Certificate Transparency: Notify STH Observers of known STHs

net/cert/sth_distributor.cc

 // Copyright 2016 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/sth_distributor.h"
 
 #include "base/metrics/histogram_macros.h"
 #include "base/time/time.h"
 #include "net/cert/signed_tree_head.h"
 
 namespace {
 const char kPilotLogID[33] =
     "\xa4\xb9\x09\x90\xb4\x18\x58\x14\x87\xbb\x13\xa2\xcc\x67\x70\x0a\x3c\x35"
     "\x98\x04\xf9\x1b\xdf\xb8\xe3\x77\xcd\x0e\xc8\x0d\xdc\x10";
 }
 
 namespace net {
 
 namespace ct {
 
 STHDistributor::STHDistributor()
     : observer_list_(base::ObserverList<STHObserver>::NOTIFY_EXISTING_ONLY) {}
 
 STHDistributor::~STHDistributor() {}
 
 void STHDistributor::NewSTHObserved(const SignedTreeHead& sth) {
+  auto it = std::find_if(observed_sths_.begin(), observed_sths_.end(),
+                         [&sth](const SignedTreeHead& other) {
+                           return sth.log_id == other.log_id;
+                         });
+
+  if (it == observed_sths_.end())
+    observed_sths_.push_back(sth);
+  else
+    *it = sth;
+
   FOR_EACH_OBSERVER(STHObserver, observer_list_, NewSTHObserved(sth));
 
   if (sth.log_id.compare(0, sth.log_id.size(), kPilotLogID,
                          arraysize(kPilotLogID) - 1) != 0)
     return;
 
   const base::TimeDelta sth_age = base::Time::Now() - sth.timestamp;
   UMA_HISTOGRAM_CUSTOM_TIMES("Net.CertificateTransparency.PilotSTHAge", sth_age,
                              base::TimeDelta::FromHours(1),
                              base::TimeDelta::FromDays(4), 100);
 }
 
 void STHDistributor::RegisterObserver(STHObserver* observer) {
   observer_list_.AddObserver(observer);
+  for (const auto& sth : observed_sths_)
+    observer->NewSTHObserved(sth);

[Ryan Sleevi, 2016/05/16 19:19:01]

DESIGN/DANGER/SAFETY: It seems possible that |observer->NewSTHObserved()| may
end up calling STHDistributor::NewSTHObserved, which would invalidate the
iterator being used here.

Nothing about the API contract prevents this, especially if end its up
propagating through a number of dependent classes.

// Make a local copy, because notifying the |observer| of a
// new STH may result in this class being notified of a
// (different) new STH, thus invalidating the iterator.
std::vector<SignedTreeHeads> local_sths(observed_sths_);
for (const auto& sth : local_sths)
  observer->NewSTHObserved(sth);

That's because the is-a contract doesn't provide any guarantees against this
reentrancy. If you wanted to violate the is-a rule and add that condition, then
I'd suggest something like

base::AutoReset auto_reset(&registering_observer_, true);
for (const auto& sth : observed_sths_)
  observer->NewSTHObserved(sth);


And in NewSTHObserved:
DCHECK(!registering_observer_);


However, imposing that constraint (on when RegisterObserver can be called) seems
to violate the is-a rule, which is why I'm much more keen to explicitly handle
it. But that's more memory intensive, so *shrug*

[Eran Messeri, 2016/05/17 09:51:03]

Good point - switched to your first suggestion of making a local copy.
I'd like to point out that since observer registration is a rare event (start-up
/ new profile creation) memory consumption during the process may not be such a
major concern.

 }
 
 void STHDistributor::UnregisterObserver(STHObserver* observer) {
   observer_list_.RemoveObserver(observer);
 }
 
 }  // namespace ct
 
 }  // namespace net