1347461 - Crash in mozilla::DataStorage::Init

Status: RESOLVED FIXED

(Core :: Security: PSM, defect, P1)

Description

[Calixte Denizet (:calixte)]

This bug was filed from the Socorro interface and is 
report bp-c7ba45aa-99a9-448b-91df-37da82170314.
=============================================================

There is 1 crash on nightly 55 with buildid 20170314030215. In analyzing the backtrace, the regression may have been introduced by patch [1] to fix bug 20170314030215.

[1] https://hg.mozilla.org/mozilla-central/rev?node=bf33ec027cea3933857e578f37e2e577f0cb1e85

Comment 1

[(no longer active)]

Yeah looks like this is my fault indeed.

Comment 2

[(no longer active)]

The DataStorage::Get API is way too flexible.  It allows you to pass in any file name as input.  So technically if the parent side, while trying to initialize the DataStorage object for SiteSecurityServiceState.txt for example fails, no entries for this file will be sent down to the child process, so the DataStorage object for that file will be left uninitialized and Init() will be attempted to be called like this in the child, leading to the crash.

I think instead of doing <https://hg.mozilla.org/mozilla-central/rev?node=bf33ec027cea3933857e578f37e2e577f0cb1e85#l2.13> and hoping for the best (which IIRC I did in respnse to a crash like this before) it's time to design a better API that allows us to initialize the well-known DataStorage objects instead of allowing callers to screw things up like this with runtime failures.

I'll post a patch later today.

Comment 3

[(no longer active)]

Attachment: Part 1: Add a C++ API for the list of DataStorage classes

Comment 4

[(no longer active)]

Attachment: Part 2: Always initialize all DataStorage classes in the content process at initialization time

Comment 5

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Comment on attachment 8847866 [details] [diff] [review]
Part 1: Add a C++ API for the list of DataStorage classes

Review of attachment 8847866 [details] [diff] [review]:
-----------------------------------------------------------------

If I'm understanding correctly, this patch is doing two things: 1. fixing this crash by always initializing every known DataStorage rather than just the ones that nsSiteSecurityService (should) initialize(s) when it starts and 2. fixing the DataStorage::Get interface so code can't just throw any random string at it. Given that, I think it might be best to break this up into two bugs: one that fixes the crash and a follow-up that improves the API.

That said, I have some thoughts on how this new API works. First, I'm a bit concerned about using the DATA_STORAGE macro / #include to accomplish this. It's very nearly too clever, which makes this code more difficult to maintain going forward. It might work fine if it were confined to the DataStorage implementation, but in this patch it's not - ContentParent.cpp uses it. Second, DataStorage::GetIfExists and DataStorage::GetFromRawFileName are still public functions, so it doesn't seem that this completely addresses the issue of having a too-permissive API. It would be great (and more work, I realize) to make all consumers of DataStorage have to pass in a known DataStorageClass value and to never accept raw filenames from outside the API (we might have to do something special for the gtests, I realize).

Finally, in terms of reviews, I feel like I do a better job as reviewer on mozreview rather than splinter, so I'd prefer to use that going forward if possible. Thanks!

::: dom/ipc/ContentParent.cpp
@@ +2227,2 @@
>    }
> +#include "mozilla/DataStorageList.h"

It seems like rather than initializing / getting all of the DataStorages we know about, we should just get the ones we know the child process is interested in.

Comment 6

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Comment on attachment 8847868 [details] [diff] [review]
Part 2: Always initialize all DataStorage classes in the content process at initialization time

Review of attachment 8847868 [details] [diff] [review]:
-----------------------------------------------------------------

Clearing review for now, although I do have the one comment.

::: security/manager/ssl/DataStorage.cpp
@@ +164,5 @@
>    const InfallibleTArray<mozilla::dom::DataStorageEntry>& aEntries)
>  {
>    MOZ_ASSERT(XRE_IsContentProcess());
>  
> +  // Make sure to initialize all DataStorage classes.

This seems a bit unnecessary. Shouldn't we just trust the parent to have sent the child the right data? Or maybe we can turn this into more of a verification that the parent actually did send what we are expecting/interested in?

Comment 7

[(no longer active)]

(In reply to David Keeler [:keeler] (use needinfo?) from comment #5)
> Comment on attachment 8847866 [details] [diff] [review]
> Part 1: Add a C++ API for the list of DataStorage classes
> 
> Review of attachment 8847866 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> If I'm understanding correctly, this patch is doing two things: 1. fixing
> this crash by always initializing every known DataStorage rather than just
> the ones that nsSiteSecurityService (should) initialize(s) when it starts

After looking over things again, it seems that in the rush to fix this I made a mistake.  I meant to remove the hunk in ContentParent.cpp here before submitting it.  Sorry about that.

The first part shouldn't have changed the behavior at all.  It should basically make it so that the callers don't pass in a string as the name of the file.

> and 2. fixing the DataStorage::Get interface so code can't just throw any
> random string at it.

This is the job of the first part.

> Given that, I think it might be best to break this up
> into two bugs: one that fixes the crash and a follow-up that improves the
> API.

Well, I'm only doing the first part so that I have a sane way to do the second part (aka initialize all DataStorage objects in the child process.)  Do you have another suggestion on how to do that?  If yes, I'd be happy to just do that (I don't really care about the API niceties that much in this case!)

(I thought about hardcoding the three file names here, but that's way too error prone.  The next time someone adds a new file names I promise they will forget to add the file name to the right place for it to be initialized in the content process...)

> That said, I have some thoughts on how this new API works. First, I'm a bit
> concerned about using the DATA_STORAGE macro / #include to accomplish this.
> It's very nearly too clever, which makes this code more difficult to
> maintain going forward.

FWIW it's a relatively common technique we use in a number of headers, such as IframeSandboxKeywordList.h, WorkerPrefs.h, nsGkAtomList.h, and so on...

> It might work fine if it were confined to the
> DataStorage implementation, but in this patch it's not - ContentParent.cpp
> uses it.

Yeah that part isn't nice, I agree.  But FWIW the ContentParent.cpp hunk needs to be removed as I mentioned.  Does that change your opinion about this?

> Second, DataStorage::GetIfExists and
> DataStorage::GetFromRawFileName are still public functions, so it doesn't
> seem that this completely addresses the issue of having a too-permissive
> API. It would be great (and more work, I realize) to make all consumers of
> DataStorage have to pass in a known DataStorageClass value and to never
> accept raw filenames from outside the API (we might have to do something
> special for the gtests, I realize).

I tried to do that initially, but I don't know the right magic for gtests, I'm not familiar with their macro system and don't know what I need to befriend to make sure that works.  Do you know?  Would it be OK to put that part in a global function and befriend that one instead?

(Basically I'd be happy to address any and all of these comments but I'd appreciate more concrete suggestions!)

> Finally, in terms of reviews, I feel like I do a better job as reviewer on
> mozreview rather than splinter, so I'd prefer to use that going forward if
> possible. Thanks!

Ugh.  :(  How strongly do you feel about this?

Long story short, there are two reasons I don't use MozReview.  One reason is that I use git locally and the setup instructions seemed long and pretty complex and it hasn't been so far worth my time to try to set it up, but more importantly because it's incompatible with my local workflow, so even if I do set it up it will be for this one bug and I won't use it again (or I'd have to change my local workflow, which would be fine except that it slows me down due to increasing the number of rebuilds I'd have to do; long story...).  I'd still do it I guess if I have to in order to fix this crash, but it will be something that I will be using for one bug only, so if possible I'd prefer to do something more productive with my time.  :-)

> ::: dom/ipc/ContentParent.cpp
> @@ +2227,2 @@
> >    }
> > +#include "mozilla/DataStorageList.h"
> 
> It seems like rather than initializing / getting all of the DataStorages we
> know about, we should just get the ones we know the child process is
> interested in.

This part needs to go away...

(In reply to David Keeler [:keeler] (use needinfo?) from comment #6)
> Comment on attachment 8847868 [details] [diff] [review]
> Part 2: Always initialize all DataStorage classes in the content process at
> initialization time
> 
> Review of attachment 8847868 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Clearing review for now, although I do have the one comment.
> 
> ::: security/manager/ssl/DataStorage.cpp
> @@ +164,5 @@
> >    const InfallibleTArray<mozilla::dom::DataStorageEntry>& aEntries)
> >  {
> >    MOZ_ASSERT(XRE_IsContentProcess());
> >  
> > +  // Make sure to initialize all DataStorage classes.
> 
> This seems a bit unnecessary. Shouldn't we just trust the parent to have
> sent the child the right data? Or maybe we can turn this into more of a
> verification that the parent actually did send what we are
> expecting/interested in?

Again, sorry for the confusion about this.

Comment 8

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

(In reply to :Ehsan Akhgari from comment #7)
...
> > Given that, I think it might be best to break this up
> > into two bugs: one that fixes the crash and a follow-up that improves the
> > API.
> 
> Well, I'm only doing the first part so that I have a sane way to do the
> second part (aka initialize all DataStorage objects in the child process.) 
> Do you have another suggestion on how to do that?  If yes, I'd be happy to
> just do that (I don't really care about the API niceties that much in this
> case!)
> 
> (I thought about hardcoding the three file names here, but that's way too
> error prone.  The next time someone adds a new file names I promise they
> will forget to add the file name to the right place for it to be initialized
> in the content process...)

I definitely agree :)

> > That said, I have some thoughts on how this new API works. First, I'm a bit
> > concerned about using the DATA_STORAGE macro / #include to accomplish this.
> > It's very nearly too clever, which makes this code more difficult to
> > maintain going forward.
> 
> FWIW it's a relatively common technique we use in a number of headers, such
> as IframeSandboxKeywordList.h, WorkerPrefs.h, nsGkAtomList.h, and so on...
> 
> > It might work fine if it were confined to the
> > DataStorage implementation, but in this patch it's not - ContentParent.cpp
> > uses it.
> 
> Yeah that part isn't nice, I agree.  But FWIW the ContentParent.cpp hunk
> needs to be removed as I mentioned.  Does that change your opinion about
> this?

Yes, if it's not exposed outside of the implementation I think that's probably fine.

> > Second, DataStorage::GetIfExists and
> > DataStorage::GetFromRawFileName are still public functions, so it doesn't
> > seem that this completely addresses the issue of having a too-permissive
> > API. It would be great (and more work, I realize) to make all consumers of
> > DataStorage have to pass in a known DataStorageClass value and to never
> > accept raw filenames from outside the API (we might have to do something
> > special for the gtests, I realize).
> 
> I tried to do that initially, but I don't know the right magic for gtests,
> I'm not familiar with their macro system and don't know what I need to
> befriend to make sure that works.  Do you know?  Would it be OK to put that
> part in a global function and befriend that one instead?

I fiddled around with this and it looks like if DataStorage.h forward-declares 'class psm_DataStorageTest' in the global namespace and then specifies 'friend class ::psm_DataStorageTest', it works as expected (well, on my system with the version of clang I'm running).

> (Basically I'd be happy to address any and all of these comments but I'd
> appreciate more concrete suggestions!)

Here's what I was thinking, I guess: if the friend class declaration above does work, then we should be able to make the DataStorage::Get API only take DataStorageClass values and not have the get-by-string version exposed. At that point, the only way for someone to add a new DataStorage instance would be to add a new DataStorageClass value in the enum. I think we can use that to force them to think about whether or not they'll need access to that data in child processes (somehow - I haven't fleshed that aspect out). Then, in ContentParent::InitInternal, we can just call something like DataStorage::GetAllChildProcessData(), which consults the list of DataStorages we know are needed in the child process and sends that data along. How does that sound?

> > Finally, in terms of reviews, I feel like I do a better job as reviewer on
> > mozreview rather than splinter, so I'd prefer to use that going forward if
> > possible. Thanks!
> 
> Ugh.  :(  How strongly do you feel about this?

It's not a big deal - I just thought if you were already using mercurial it wouldn't be much more work to use mozreview, but since you're not, then I can see how that would be a hassle.

Anyway, hope my suggestions are more clear this time, and thanks for working with me on this!

Comment 9

[(no longer active)]

Ugh sorry for dropping the ball on this.  I'll get back to it ASAP.

Comment 10

[(no longer active)]

OK, I think I have patches that are reasonably green on try and address your concerns.

Comment 11

[(no longer active)]

Attachment: Part 1: Add a C++ API for the list of DataStorage classes

Comment 12

[(no longer active)]

Attachment: Part 2: Always initialize all DataStorage classes in the content process at initialization time

Comment 13

[(no longer active)]

[Tracking Requested - why for this release]: regression, crash introduced in 55.

Comment 14

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Comment on attachment 8856317 [details] [diff] [review]
Part 1: Add a C++ API for the list of DataStorage classes

Review of attachment 8856317 [details] [diff] [review]:
-----------------------------------------------------------------

Awesome - thanks for doing this. Just the one nit and a comment about documentation.

::: security/manager/ssl/DataStorage.cpp
@@ +96,5 @@
> +      return GetFromRawFileName(NS_LITERAL_STRING(#_ ".txt"));
> +#include "mozilla/DataStorageList.h"
> +#undef DATA_STORAGE
> +    default:
> +      MOZ_ASSERT_UNREACHABLE("Invalid DataStorages type passed?");

nit: DataStorage singular? (and similarly in other messages like this)

@@ +169,5 @@
> +  for (auto& file : storageFiles) {
> +    dom::DataStorageEntry entry;
> +    entry.filename() = file;
> +    RefPtr<DataStorage> storage = DataStorage::GetFromRawFileName(file);
> +    if (!storage->mInitCalled) {

mInitCalled is currently documented as being protected by mMutex. I think that since it's atomic and only changes from false to true once, and since calling Init more than once is not actually harmful (other than for performance), it is ok to read mInitCalled without holding the lock, but a) if I'm wrong then we shouldn't and b) it would be nice to update the documentation.

Comment 15

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Comment on attachment 8856318 [details] [diff] [review]
Part 2: Always initialize all DataStorage classes in the content process at initialization time

Review of attachment 8856318 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM.

Comment 16

[(no longer active)]

I did check mInitCalled and came to the same conclusion as yours but actually didn't notice the documentation issue at all (my eyes ignored the comments after seeing the atomic!)  I'll update the documentation.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0fa8897bf56a
Part 1: Add a C++ API for the list of DataStorage classes; r=keeler
https://hg.mozilla.org/integration/mozilla-inbound/rev/4947199c855f
Part 2: Always initialize all DataStorage classes in the content process at initialization time; r=keeler

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0fa8897bf56a
https://hg.mozilla.org/mozilla-central/rev/4947199c855f