Certificate Transparency: Notify STH Observers of known STHs

net/cert/sth_distributor.cc

Index: net/cert/sth_distributor.cc
diff --git a/net/cert/sth_distributor.cc b/net/cert/sth_distributor.cc
index 0b8df2606660cd0e60a47b9756bc7d3d3b5a95fa..38dd525f7320b94b650fc842e363e38789567ada 100644
--- a/net/cert/sth_distributor.cc
+++ b/net/cert/sth_distributor.cc
@@ -24,6 +24,16 @@ STHDistributor::STHDistributor()
 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,
@@ -38,6 +48,8 @@ void STHDistributor::NewSTHObserved(const SignedTreeHead& sth) {
 
 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) {