1170311 - crash in mozilla::BasePrincipal::Equals(nsIPrincipal*, bool*)

Status: RESOLVED FIXED

(Core :: Security: CAPS, defect)

Description

[Mats Palmgren (inactive)]

This bug was filed from the Socorro interface and is 
report bp-31d2f9ff-1b19-4e5c-90a2-facfd2150531.
=============================================================

New signature in v41 that first appeared in build 20150519030202.
This method was added in bug 1164977 which landed on m-c
2015-05-19 03:32:17 PDT.

Comment 1

[Bobby Holley (:bholley)]

Hm, I can see how we might crash here, but I'd expect it to be a null-deref, which this isn't. Mats, is this your crash, or are you just filing it from finding it in socorro? Figuring out the concrete type of aPrincipal and capturing the JS stack would be very helpful.

Comment 2

[Mats Palmgren (inactive)]

Sorry, it's not my crash, I just saw that this signature is new in crash data.

Comment 3

[Bobby Holley (:bholley)]

Hm, I'm pretty stumped as to how we'd get that crash signature - it appears that the cast from nsIPrincipal to BasePrincipal is causing trouble, but I don't see any nsIPrincipal implementations that don't inherit BasePrincipal:

bholley@extraordinary /files/mozilla/repos/dd (caps_prin_crash) $ grep -R "public nsIPrincipal" *
caps/nsJSPrincipals.h:class nsJSPrincipals : public nsIPrincipal, public JSPrincipals
obj-x86_64-apple-darwin14.3.0/dist/include/nsIPrincipal.h:class nsPrincipal : public nsIPrincipal
obj-x86_64-apple-darwin14.3.0/dist/include/nsJSPrincipals.h:class nsJSPrincipals : public nsIPrincipal, public JSPrincipals
bholley@extraordinary /files/mozilla/repos/dd (caps_prin_crash) $ grep -R "public nsJSPrincipal" *
caps/BasePrincipal.h:class BasePrincipal : public nsJSPrincipals
obj-x86_64-apple-darwin14.3.0/dist/include/mozilla/BasePrincipal.h:class BasePrincipal : public nsJSPrincipals
bholley@extraordinary /files/mozilla/repos/dd (caps_prin_crash) $ 


On the off chance that it's causing this somehow, I'll try fixing the release-mode assertion against passing null - but that doesn't seem to match the behavior in the crash reports. Ideas welcome.

Comment 4

[Bobby Holley (:bholley)]

Attachment: Stop asserting non-null argument to nsIPrincipal::{subsumes,equals}{,ConsideringDomain}. v1

Comment 5

[(Away)]

I don't think it's the cast. It's the first Subsumes that's failing. (The debug symbols always point to the last line of the statement)

Comment 6

[Bobby Holley (:bholley)]

(In reply to David Major [:dmajor] from comment #5)
> I don't think it's the cast. It's the first Subsumes that's failing.

Hm, how can you tell? Wouldn't the crash be in BasePrincipal::Subsumes in that case?

If that's true, it seems more likely that the problem is the MOZ_RELEASE_ASSERT. But shouldn't that crash at a predictable location?

> (The debug symbols always point to the last line of the statement)

That is good to know.

Comment 7

[(Away)]

Here's the stack:

0029c618  61a6ab8a xul!NS_InvokeByIndex+0x27  <-- return address
0029c61c  0a4d52b0 <-- this
0029c620  00000000 <-- aOther
0029c624  0029c8a0 <-- aResult

In the disassembly I can see BasePrincipal::Subsumes inlined into BasePrincipal::Equals.

Comment 8

[Bobby Holley (:bholley)]

(In reply to David Major [:dmajor] from comment #7)
> Here's the stack:
> 
> 0029c618  61a6ab8a xul!NS_InvokeByIndex+0x27  <-- return address
> 0029c61c  0a4d52b0 <-- this
> 0029c620  00000000 <-- aOther
> 0029c624  0029c8a0 <-- aResult

Ah I see! That's very helpful. Is this from crash-stats.mozilla.com, or are you getting that from the minidump?

Given that, it seems pretty clear that it's the MOZ_RELEASE_ASSERT. I was confused because the crashing address was non-zero, but I just noticed that MOZ_REALLY_CRASH actually does ::TerminateProcess(::GetCurrentProcess(), 3) on windows. Is there any way to see that this is happening from crash-stats?

Comment 9

[(Away)]

(In reply to Bobby Holley (:bholley) from comment #8)
> Ah I see! That's very helpful. Is this from crash-stats.mozilla.com, or are
> you getting that from the minidump?

Minidump.

> Given that, it seems pretty clear that it's the MOZ_RELEASE_ASSERT. I was
> confused because the crashing address was non-zero, but I just noticed that
> MOZ_REALLY_CRASH actually does ::TerminateProcess(::GetCurrentProcess(), 3)
> on windows. Is there any way to see that this is happening from crash-stats?

Before TerminateProcess, it does a __debugbreak(), which compiles to an asm "int 3". That gets caught by Breakpad as the EXCEPTION_BREAKPOINT that we see in the crash reports.

Comment 10

[(Away)]

(In the case of EXCEPTION_BREAKPOINT, the crashing address is actually the code address of the int 3)

Comment 11

[Gabor Krizsanits (INACTIVE)]

Comment on attachment 8613742 [details] [diff] [review]
Stop asserting non-null argument to nsIPrincipal::{subsumes,equals}{,ConsideringDomain}. v1

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

If I get it correctly this is not really a sec crit then just a null deref, no?

::: caps/BasePrincipal.cpp
@@ +75,5 @@
>  
>  bool
>  BasePrincipal::Subsumes(nsIPrincipal* aOther, DocumentDomainConsideration aConsideration)
>  {
> +  MOZ_ASSERT(aOther);

I don't really see the reason to change this from MOZ_RELEASE_ASSERT after the fixes in this patch, could you explain it?

Comment 12

[Bobby Holley (:bholley)]

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #11)
> Comment on attachment 8613742 [details] [diff] [review]
> Stop asserting non-null argument to
> nsIPrincipal::{subsumes,equals}{,ConsideringDomain}. v1
> 
> Review of attachment 8613742 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> If I get it correctly this is not really a sec crit then just a null deref,
> no?
> 
> ::: caps/BasePrincipal.cpp
> @@ +75,5 @@
> >  
> >  bool
> >  BasePrincipal::Subsumes(nsIPrincipal* aOther, DocumentDomainConsideration aConsideration)
> >  {
> > +  MOZ_ASSERT(aOther);
> 
> I don't really see the reason to change this from MOZ_RELEASE_ASSERT after
> the fixes in this patch, could you explain it?

Before my patch, I was interested in catching XPCOM consumers (especially extensions) that were passing null to subsumes or equals. Now that we're explicitly handling that, I don't see any special reason why we need release-mode checking on this particular assertion.

Comment 13

[Bobby Holley (:bholley)]

(In reply to Bobby Holley (:bholley) from comment #12)
> (In reply to Gabor Krizsanits [:krizsa :gabor] from comment #11)
> > If I get it correctly this is not really a sec crit then just a null deref,
> > no?

Correct.

Comment 14

[Bobby Holley (:bholley)]

Thanks gabor for reviews, and dmajor for the insight!

Comment 15

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ce014d3475af

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/ce014d3475af

Comment 17

[(Away)]

Stats on the 0603 nightly look good. Do you still want to [leave open]?

Comment 18

[Bobby Holley (:bholley)]

(In reply to David Major [:dmajor] from comment #17)
> Stats on the 0603 nightly look good. Do you still want to [leave open]?

Thanks for following up.