Move preference MACs to the protected preference stores.

base/prefs/json_pref_store.cc

 // Copyright (c) 2012 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 "base/prefs/json_pref_store.h"
 
 #include <algorithm>
 
 #include "base/bind.h"
 #include "base/callback.h"
@@ skipping 279 matching lines @@
 }
 
 void JsonPrefStore::RegisterOnNextSuccessfulWriteCallback(
     const base::Closure& on_next_successful_write) {
   writer_.RegisterOnNextSuccessfulWriteCallback(on_next_successful_write);
 }
 
 void JsonPrefStore::OnFileRead(scoped_ptr<base::Value> value,
                                PersistentPrefStore::PrefReadError error,
                                bool no_dir) {
-  scoped_ptr<base::DictionaryValue> unfiltered_prefs(new base::DictionaryValue);
-
   read_error_ = error;
 
   bool initialization_successful = !no_dir;
 
   if (initialization_successful) {
     switch (read_error_) {
       case PREF_READ_ERROR_ACCESS_DENIED:
       case PREF_READ_ERROR_FILE_OTHER:
       case PREF_READ_ERROR_FILE_LOCKED:
       case PREF_READ_ERROR_JSON_TYPE:
       case PREF_READ_ERROR_FILE_NOT_SPECIFIED:
         read_only_ = true;
         break;
       case PREF_READ_ERROR_NONE:
         DCHECK(value.get());
-        unfiltered_prefs.reset(
-            static_cast<base::DictionaryValue*>(value.release()));
+        prefs_.reset(static_cast<base::DictionaryValue*>(value.release()));
         break;
       case PREF_READ_ERROR_NO_FILE:
         // If the file just doesn't exist, maybe this is first run.  In any case
         // there's no harm in writing out default prefs in this case.
         break;
       case PREF_READ_ERROR_JSON_PARSE:
       case PREF_READ_ERROR_JSON_REPEAT:
         break;
       case PREF_READ_ERROR_ASYNCHRONOUS_TASK_INCOMPLETE:
         // This is a special error code to be returned by ReadPrefs when it
         // can't complete synchronously, it should never be returned by the read
         // operation itself.
         NOTREACHED();
         break;
       case PREF_READ_ERROR_LEVELDB_IO:
       case PREF_READ_ERROR_LEVELDB_CORRUPTION_READ_ONLY:
       case PREF_READ_ERROR_LEVELDB_CORRUPTION:
         // These are specific to LevelDBPrefStore.
         NOTREACHED();
       case PREF_READ_ERROR_MAX_ENUM:
         NOTREACHED();
         break;
     }
   }
 
   if (pref_filter_) {
     filtering_in_progress_ = true;
     const PrefFilter::PostFilterOnLoadCallback post_filter_on_load_callback(
         base::Bind(
             &JsonPrefStore::FinalizeFileRead, this, initialization_successful));

[gab, 2014/06/06 21:54:58]

This callback was using ref-counting simply to avoid changing the old behaviour
where the reader (FileThreadDeserializer) would own a ref to this JsonPrefStore
all the way until the end of OnFileRead() -- which would guarantee that
observers are always notified of a read (success or fail) after calling
ReadPrefsAsync().

We (me and Bernhard) thought this was a side-effect of the current
implementation and ideally wanted to eventually remove that (i.e., have the read
task be weakly bound to this JsonPrefStore, allowing the store to go away if it
needs to before the read comes back), but kept it as-is for now for the sake of
not introducing more changes into the big change that was
TrackedPreferencesMigrator et al.

By making FilterOnLoad take a raw pointer to this store's prefs rather than own
them until it's ready to give them back to us you are making the above callback
HAVE to use ref-counting or the following could happen:

1) JsonPrefStore 1 hands a raw pointer to its prefs post-read.
2) JsonPrefStore 1 goes away.
2) JsonPrefStore 2 hands a raw pointer to its prefs post-read; which triggers
migration, which will use the invalid pointer to 1's prefs.

At the very least a comment should be added to this effect above.

Overall however I think this is the wrong approach. This strengthens my belief
in the other approach I'd suggested earlier:

Let the TrackedPreferencesMigrator be aware of where to find the MACs in the
pref dictionaries (or even better: wrap the dictionary in a HashStoreContents
that already knows how to do this to avoid duplicating this logic).

The TrackedPreferencesMigrator also needs to be given a HashStoreContents
wrapping local_state.

Then the TrackedPreferencesMigrator does ALL the migration work while owning the
prefs and hands them back to their respective stores when everything is done.

Also, in my proposal, PrefHashStores don't have a reference to the PrefStore
storing the MACs so there is no cyclic dependency like there is in the model
you're proposing (JsonPrefStore => PrefHashFilter => PrefHashStore ->
JsonPrefStore). Instead PrefHashFilters are always handed a DictionaryValue* in
operations where they're meant to do work on the prefs and that DictionaryValue*
is passed to PrefHashStore::BeginTransaction() -- most likely as a
DictionaryHashStoreContents.

PrefHashStores essentially now only own the state of whether the hash store was
initially trusted (this state would be initialized in FilterOnLoad) -- in fact
PrefHashStore can probably go away altogether and we can let that boolean state
live in PrefHashFilter.

[erikwright (departed), 2014/06/10 14:25:36]

This can be mitigated in other ways. For example, you can have the
post_filter_on_load_callback own the DictionaryValue (or share with this store,
via refcounting, an object that owns the Dictionary). So I don't think that
passing the raw pointer to the dictionary really compromises the other choices
you could make. Really, if there is an asynchronous process ongoing, something
is going to be kept alive. It might be a DictionaryValue, it might be a
JsonPrefStore, or it might be something in between. The only reason to really
care what it is is if there are other side-effects to the destructor of the
JsonPrefStore. And if there are, you can find lots of ways to ensure those
side-effects have the semantics you expect.
I don't mind adding a comment - would you mind supplying your preferred wording?
HashStoreContents and/or PrefHashStore are appropriate layers of abstraction and
can provide whatever APIs we need to move things with the semantics we want.
There is no need to break encapsulation here.
As it is in this patchset.
I believe that's what this patchset does. Is there a distinction you are making
here, other than the part about "owns"? All that is injected is the
implementation of PrefHashStore (and HashStoreContents) which only wraps the
storage model for the hashes.

IIUC the primary difference you want to see is that the PrefStore* or
DictionaryValue* is passed into the PrefHashStore rather than being embedded
within it.
I agree that we should break the circular dependency.
PrefHashStore implements:

(1) the process for calculating a MAC from a value and storing it in a
Dictionary.
(2) the process of retrieving a MAC from a dictionary, comparing it to a value,
and returning a result.
(3) the protocol for maintaing the super MAC and determining whether newly
protected values should be trusted.

Since (3) is intimately tied to the import process it is essential that it be
implemented by a component that participates in the import process. The
PrefHashFilter does not.

I believe that the above demonstrates why it is appropriate for the
PrefHashStore to be injected as a dependency into the migration process, even if
it expects to be handed a DictionaryValue to work on.

[gab, 2014/06/10 16:54:33]

For the comment to be added above, how about:

// This callback must own a reference to this store since it depends on |prefs_|
being alive when it runs.
// TODO(gab): Remove this temporary requirement by handing |prefs_| to
FilterOnLoad() which will hand them back to this JsonPrefStore via
FinalizeFileRead, as was the case prior to
https://codereview.chromium.org/324493002.


As for your point in (3) though, I'm not convinced that PrefHashStore has to be
involved in migration. I think HashStoreContents could be used for everything
since we want to move the MACs as-is (no knowledge of how to store/check them is
required). PrefHashStore would still be used by the PrefHashFilter for MAC
computing/storing/checking logic, but I see it becoming an implementation detail
of PrefHashFilter (and this is what I meant by "it can probably go away
altogether"; not that we would just inline all its logic into PrefHashFilter).
There would be no local_state backed PrefHashStore, the only local_state backed
object would be the PrefServiceHashStoreContents handed to
TrackedPreferencesMigrator.
Similarly the TrackedPreferencesMigrator would build DictionaryHashStoreContents
directly from the prefs dictionaries it receives (no need to inject the
dictionaries into pre-existing objects).
The PrefHashStore owned by the PrefHashFilter would build a
DictionaryHashStoreContents from the raw DictionaryValue for every
PrefHashStoreTransaction.


Lastly, my first comment was describing my complete vision, it wasn't trying to
imply that none of the benefits it provides were present in the current proposal
(apologies if it felt that way); some things are indeed already covered by this
proposal as you mentioned above.

[erikwright (departed), 2014/06/10 20:27:47]

I agree that we don't need a Local State backed PrefHashStore.
The thing that you're missing is that the PrefHashStore validates and calculates
(or not) the super MAC for the stores it is migrating to/from. And in order to
do that it needs the seed, device ID, and to know whether/not to actually do
this. While the "super MAC for Protected, not for regular Preferences" part is
not that big a deal, actually validating/updating the super MAC for the
Protected store using the seed/device ID is not practical without being handed a
non-trivial part of a PrefHashStore. In particular, you probably want something
very similar to the existing "Transaction" semantics that only update the super
MAC once when the import (or clean) is complete.
No hurt feelings, just wanted to make sure I had understood all of your points.

-    pref_filter_->FilterOnLoad(post_filter_on_load_callback,
-                               unfiltered_prefs.Pass());
+    pref_filter_->FilterOnLoad(post_filter_on_load_callback, prefs_.get());
   } else {
-    FinalizeFileRead(initialization_successful, unfiltered_prefs.Pass(), false);
+    FinalizeFileRead(initialization_successful, false);
   }
 }
 
 JsonPrefStore::~JsonPrefStore() {
   CommitPendingWrite();
 }
 
 bool JsonPrefStore::SerializeData(std::string* output) {
   if (pref_filter_)
     pref_filter_->FilterSerializeData(prefs_.get());
 
   JSONStringValueSerializer serializer(output);
   serializer.set_pretty_print(true);
   return serializer.Serialize(*prefs_);
 }
 
 void JsonPrefStore::FinalizeFileRead(bool initialization_successful,
-                                     scoped_ptr<base::DictionaryValue> prefs,
                                      bool schedule_write) {
   filtering_in_progress_ = false;
 
   if (!initialization_successful) {
     FOR_EACH_OBSERVER(PrefStore::Observer,
                       observers_,
                       OnInitializationCompleted(false));
     return;
   }
 
-  prefs_ = prefs.Pass();
-
   initialized_ = true;
 
   if (schedule_write && !read_only_)
     writer_.ScheduleWrite(this);
 
   if (error_delegate_ && read_error_ != PREF_READ_ERROR_NONE)
     error_delegate_->OnError(read_error_);
 
   FOR_EACH_OBSERVER(PrefStore::Observer,
                     observers_,
                     OnInitializationCompleted(true));
 
   return;
 }