base/prefs/json_pref_store.cc - Issue 324493002: Move preference MACs to the protected preference stores.

Unified Diff: base/prefs/json_pref_store.cc

Issue 324493002: Move preference MACs to the protected preference stores. (Closed) Base URL: svn://svn.chromium.org/chrome/trunk/src

Patch Set: Pull some stuff from Persistent to WriteablePrefStore. Created 6 years, 6 months ago

Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.

Jump to:

View side-by-side diff with in-line comments

Download patch

Index: base/prefs/json_pref_store.cc

diff --git a/base/prefs/json_pref_store.cc b/base/prefs/json_pref_store.cc

index a6c236261ba5d4426f2cff61bf398b8330302a9c..3eebff1486350dc5e8ed4be721716a6dc25a474c 100644

--- a/base/prefs/json_pref_store.cc

+++ b/base/prefs/json_pref_store.cc

@@ -297,8 +297,6 @@ void JsonPrefStore::RegisterOnNextSuccessfulWriteCallback(

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;

@@ -314,8 +312,7 @@ void JsonPrefStore::OnFileRead(scoped_ptr<base::Value> value,

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

@@ -346,10 +343,9 @@ void JsonPrefStore::OnFileRead(scoped_ptr<base::Value> value,

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 avo

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,

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: //

On 2014/06/10 14:25:36, erikwright wrote: > On 2014/06/06 21:54:58, gab wrote: > > 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. > > > > 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. > > > At the very least a comment should be added to this effect above. > > I don't mind adding a comment - would you mind supplying your preferred wording? > > > 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). > > 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. > > > The TrackedPreferencesMigrator also needs to be given a HashStoreContents > > wrapping local_state. > > As it is in this patchset. > > > Then the TrackedPreferencesMigrator does ALL the migration work while owning > the > > prefs and hands them back to their respective stores when everything is done. > > 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. > > > 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. > > I agree that we should break the circular dependency. > > > 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. > > 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.

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 Pr

On 2014/06/10 16:54:33, gab wrote: > On 2014/06/10 14:25:36, erikwright wrote: > > On 2014/06/06 21:54:58, gab wrote: > > > 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. > > > > > > > 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. > > > > > At the very least a comment should be added to this effect above. > > > > I don't mind adding a comment - would you mind supplying your preferred > wording? > > > > > 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). > > > > 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. > > > > > The TrackedPreferencesMigrator also needs to be given a HashStoreContents > > > wrapping local_state. > > > > As it is in this patchset. > > > > > Then the TrackedPreferencesMigrator does ALL the migration work while owning > > the > > > prefs and hands them back to their respective stores when everything is > done. > > > > 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. > > > > > 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. > > > > I agree that we should break the circular dependency. > > > > > 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. > > > > 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. > > 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.

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);

}

@@ -367,7 +363,6 @@ bool JsonPrefStore::SerializeData(std::string* output) {

}

void JsonPrefStore::FinalizeFileRead(bool initialization_successful,

- scoped_ptr<base::DictionaryValue> prefs,

bool schedule_write) {

filtering_in_progress_ = false;

@@ -378,8 +373,6 @@ void JsonPrefStore::FinalizeFileRead(bool initialization_successful,

return;

}

- prefs_ = prefs.Pass();

initialized_ = true;

if (schedule_write && !read_only_)

« no previous file with comments | « base/prefs/json_pref_store.h ('k') | base/prefs/persistent_pref_store.h » ('j') | chrome/browser/prefs/pref_hash_filter.cc » ('J')