Bugzilla

Comment 4

•

6 years ago

(In reply to Sebastian Hengst [:aryx] (needinfo on intermittent or backout) from comment #3)
I'm not convinced either of my patches in that push would have caused this.  However, feel free to back them out if you think it would help track this down.

It looks like we're crashing because one of nsXPCWrappedJS::mJSObj and mJSObjGlobal holds a stale pointer into a chunk that has been freed.  I checked and both of these are traced from the TraceJS() method.

Jan, I was looking at the changes for bug 1478359.  There are a couple of places where we don't treat mJSObj and mJSObjGlobal in the same way - for example where we expose mJSObj but not mJSObjGlobal.  In general mJSObj will reference mJSObjGlobal, but is it possible for mJSObj to be a dead wrapper and if so do you think this could cause problems?

Flags: needinfo?(jdemooij)

Jan de Mooij [:jandem]

Assignee

Comment 5

•

6 years ago

(In reply to Jon Coppeard (:jonco) from comment #4)
> In general mJSObj will
> reference mJSObjGlobal, but is it possible for mJSObj to be a dead wrapper
> and if so do you think this could cause problems?

Root wrappers (nsXPCWrappedJS::IsRootWrapper) always store an unwrapped mJSObj. I think most of the GC complexity like UpdateObjectPointerAfterGC is limited to root wrappers.

Non-root wrappers always reference the root wrapper as mRoot but may have a CCW as mJSObj.

We should null out mJSObjGlobal whenever we null out mJSObj etc. It would be interesting to know whether the nsXPCWrappedJS here is a root wrapper.

Comment 6

•

6 years ago

If it helps shed any light on this, we also started hitting this on Beta today.
https://treeherder.mozilla.org/logviewer.html#?job_id=212631860&repo=mozilla-beta

status-firefox64: --- → affected

status-firefox65: --- → affected

Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)

Updated

•

6 years ago

Keywords: regression, sec-high

Updated

•

6 years ago

Comment 10

•

6 years ago

Attached patch bug1508102-wrapped-js-cc — Details — Splinter Review

This patch takes account mJSObjGlobal in the CC traverse and CanSkip() methods.

Normally this field will be the global of mJSObj and this isn't necessary (I think?) but it's possible that mJSObj is a dead wrapper and mJSObjGlobal is now unrelated to it (and I've confirmed that this does happen).

I don't know if this will fix the problem... Andrew what do you think?

Flags: needinfo?(jcoppeard)

Attachment #9026689 - Flags: review?(continuation)

Comment 11

•

6 years ago

Comment on attachment 9026689 [details] [diff] [review]
bug1508102-wrapped-js-cc

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

Hmm looks like a reasonable change. I don't know if it will fix this or not.

Attachment #9026689 - Flags: review?(continuation) → review+

Comment 12

•

6 years ago

Comment on attachment 9026689 [details] [diff] [review]
bug1508102-wrapped-js-cc

[Security Approval Request]

How easily could an exploit be constructed based on the patch?: Very difficult.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: No

Which older supported branches are affected by this flaw?: 

If not all supported branches, which bug introduced the flaw?: Bug 1478359

Do you have backports for the affected branches?: No

If not, how different, hard to create, and risky will they be?: Trival.

How likely is this patch to cause regressions; how much testing does it need?: I'd say very unlikely.

Attachment #9026689 - Flags: sec-approval?

Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)

Comment 13

•

6 years ago

Does this affect ESR60 and Firefox 63? My guess is that it does not based on bug 1478359.

Flags: needinfo?(jcoppeard)

Updated

•

6 years ago

Comment 14

•

6 years ago

(In reply to Al Billings [:abillings] from comment #13)
It looks like that bug landed in 63, so I'd say that is affected but esr60 is not.

Flags: needinfo?(jcoppeard)

Updated

•

6 years ago

Blocks: 1509986

Liz Henry (:lizzard) (relman/hg->git project)

Comment 15

•

6 years ago

Liz, can we take this for 64? I know it is late.

Flags: needinfo?(lhenry)

Comment 16

•

6 years ago

Yes we still can.

Flags: needinfo?(lhenry)

Liz Henry (:lizzard) (relman/hg->git project)

Comment 17

•

6 years ago

Jon can you also request uplift to beta? Thanks! I'm just assigning you to the bug since you wrote the patch, hope that's ok.

Assignee: nobody → jcoppeard

status-firefox63: --- → wontfix

status-firefox-esr60: --- → unaffected

Jan de Mooij [:jandem]

Assignee

Comment 18

•

6 years ago

Thanks for fixing this. Not sure how I missed these spots.

Flags: needinfo?(jdemooij)

Comment 20

•

6 years ago

Comment on attachment 9026689 [details] [diff] [review]
bug1508102-wrapped-js-cc

sec-approval+

Attachment #9026689 - Flags: sec-approval? → sec-approval+

Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)

Comment 21

•

6 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/930054aa138bd32f51bbe2ec84e07267859afa6e

Keywords: leave-open

Comment 22

•

6 years ago

https://hg.mozilla.org/mozilla-central/rev/930054aa138b

Status: NEW → RESOLVED

Closed: 6 years ago

Resolution: --- → FIXED

Updated

•

6 years ago

Status: RESOLVED → REOPENED

Resolution: FIXED → ---

Randell Jesup [:jesup] (needinfo me)

Comment 24

•

6 years ago

It looks like this is still happening over in bug 1501413 and bug 1503226? Any more ideas, Jon? Thanks.

Flags: needinfo?(jcoppeard)

Comment 25

•

6 years ago

Should we hide those bugs as sec issues?  There's a downside to that since they're intermittent failures in automation...

Flags: needinfo?(dveditz)

Comment 26

•

6 years ago

I'm wondering whether this is related to bug 1397297.

Smaug, is it possible for a cycle collected object to be scheduled for deletion and for the deletion to happen after an intervening GC, without the object being traced in that GC?  If so that could explain this.

Updated

•

6 years ago

Flags: needinfo?(jcoppeard) → needinfo?(bugs)

Olli Pettay [:smaug][bugs@pettay.fi]

Updated

•

5 years ago

status-firefox64: affected → wontfix

status-firefox66: --- → affected

Comment 31

•

5 years ago

(In reply to Jon Coppeard (:jonco) from comment #26)
> I'm wondering whether this is related to bug 1397297.
> 
> Smaug, is it possible for a cycle collected object to be scheduled for
> deletion and for the deletion to happen after an intervening GC, without the
> object being traced in that GC?  If so that could explain this.

Sorry about delay.

bug 1397297 shouldn't really cause that, since it just make already asynchronous operation even more async, and if some C++ is holding a ref to JS, it should be in JSHolders hashtable, and those
objects are iterated in CycleCollectedJSRuntime::TraceNativeGrayRoots.

But https://searchfox.org/mozilla-central/rev/13788edbabb04d004e4a1ceff41d4de68a8320a2/js/xpconnect/src/XPCWrappedJS.cpp#286,288,295 looks worrisome to me. Why is it ok to call RemoveFromRootSet(); but not clear mJSObj and mJSObjGlobal?
https://searchfox.org/mozilla-central/rev/13788edbabb04d004e4a1ceff41d4de68a8320a2/js/xpconnect/src/XPCJSRuntime.cpp#657,664-666 needs to still work.
jonco, does that look suspicious to you?

Flags: needinfo?(bugs) → needinfo?(jcoppeard)

:Gijs (he/him)

Comment 32

•

5 years ago

FWIW, bug 1514778 references a test that's already in the tree that fails reproducibly with this crash on our mac opt infra when run with --verify , if that's helpful in tracking this down.

:Gijs (he/him)

Updated

•

5 years ago

Blocks: 1514778

Comment 34

•

5 years ago

(In reply to Olli Pettay [:smaug] (high review load) from comment #31)
> if some C++ is holding a ref to
> JS, it should be in JSHolders hashtable, and those
> objects are iterated in CycleCollectedJSRuntime::TraceNativeGrayRoots.

Ah ok, it's not that then.

> But
> https://searchfox.org/mozilla-central/rev/
> 13788edbabb04d004e4a1ceff41d4de68a8320a2/js/xpconnect/src/XPCWrappedJS.
> cpp#286,288,295 looks worrisome to me. Why is it ok to call
> RemoveFromRootSet(); but not clear mJSObj and mJSObjGlobal?

These still get updated/swept by nsXPCWrappedJS::UpdateObjectPointerAfterGC when the ref count is 1 AIUI.  I tried clearing them anyway but this broke a ton of xpcshell tests.

Olli Pettay [:smaug][bugs@pettay.fi]

Updated

•

5 years ago

Flags: needinfo?(jcoppeard)

Comment 35

•

5 years ago

Hmm, so is UpdateObjectPointerAfterGC not called then in some case?

Comment 36

•

5 years ago

(In reply to Olli Pettay [:smaug] (away-ish Dec 21-30) from comment #35)
Well it should always be called when the ref count == 1 (from JSObject2WrappedJSMap::UpdateWeakPointersAfterGC).

BTW I think jandem is working on a patch that removes mJSObjGlobal so this problem may go away.

Comment 37

•

5 years ago

(In reply to Jon Coppeard (:jonco) from comment #36)
> BTW I think jandem is working on a patch that removes mJSObjGlobal so this
> problem may go away.

According to Aryx, we did indeed see a big drop-off in crashes after bug 1480121 landed on 21-Dec. Jan, is that something we could sanely backport?

Flags: needinfo?(jdemooij)

Jan de Mooij [:jandem]

Assignee

Comment 38

•

5 years ago

(In reply to Ryan VanderMeulen [:RyanVM] from comment #37)
> According to Aryx, we did indeed see a big drop-off in crashes after bug
> 1480121 landed on 21-Dec. Jan, is that something we could sanely backport?

Sure, I posted a patch in bug 1480121.

Flags: needinfo?(jdemooij)

Updated

•

5 years ago

Depends on: 1480121

Cornel Ionce [:noni] [Hubs QA]

Comment 39

•

5 years ago

Fixed by bug 1480121.

Assignee: jcoppeard → jdemooij

Group: javascript-core-security → core-security-release

Status: REOPENED → RESOLVED

Closed: 6 years ago → 5 years ago

status-firefox65: affected → fixed

status-firefox66: affected → fixed

Flags: needinfo?(dveditz)

Keywords: leave-open

Priority: P5 → P2

Resolution: --- → FIXED

Target Milestone: --- → mozilla66

Updated

•

5 years ago

Flags: qe-verify-

Whiteboard: [post-critsmash-triage]