Closed Bug 1170311 Opened 5 years ago Closed 5 years ago

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

Categories

(Core :: Security: CAPS, defect, critical)

41 Branch
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox41 --- fixed

People

(Reporter: mats, Unassigned)

References

Details

(Keywords: crash, regression)

Crash Data

Attachments

(1 file)

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.
Flags: needinfo?(bobbyholley)
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.
Flags: needinfo?(bobbyholley) → needinfo?(mats)
Sorry, it's not my crash, I just saw that this signature is new in crash data.
Flags: needinfo?(mats)
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.
Whiteboard: [leave open]
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)
(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.
Flags: needinfo?(dmajor)
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.
Flags: needinfo?(dmajor)
(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?
(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.
(In the case of EXCEPTION_BREAKPOINT, the crashing address is actually the code address of the int 3)
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?
Attachment #8613742 - Flags: review?(gkrizsanits) → review+
(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.
(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.
Thanks gabor for reviews, and dmajor for the insight!
Stats on the 0603 nightly look good. Do you still want to [leave open]?
Flags: needinfo?(bobbyholley)
(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.
Status: NEW → RESOLVED
Closed: 5 years ago
Flags: needinfo?(bobbyholley)
Resolution: --- → FIXED
Whiteboard: [leave open]
Target Milestone: --- → mozilla41
You need to log in before you can comment on or make changes to this bug.