Closed Bug 886850 Opened 7 years ago Closed 6 years ago

Assertion failure: [barrier verifier] Unmarked edge: method, at gc/Verifier.cpp:572 with setObjectMetadataCallback

Categories

(Core :: JavaScript Engine, defect, critical)

x86_64
Linux
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla26
Tracking Status
firefox23 --- unaffected
firefox24 - wontfix
firefox25 + wontfix
firefox26 - fixed
firefox-esr17 --- unaffected
firefox-esr24 26+ fixed
b2g18 --- unaffected
b2g-v1.1hd --- unaffected
b2g-v1.2 --- fixed

People

(Reporter: decoder, Assigned: bhackett)

References

(Blocks 1 open bug)

Details

(Keywords: assertion, sec-high, testcase, Whiteboard: [fuzzblocker] [jsbugmon:update][adv-main26+][adv-esr24.2+])

Attachments

(2 files, 3 obsolete files)

The following testcase asserts on mozilla-central revision bc569033125a (no options required):


evaluate("\
for ( var time = 0, year = 1969; year >= 0; year-- )\
  time -= TimeInYear(year);\
function TimeInYear( y ) {}\
gczeal(4);\
", { noScriptRval : true });
evaluate("setObjectMetadataCallback();", { noScriptRval : true });
Whiteboard: [jsbugmon:update,bisect]
Attachment #767268 - Attachment is obsolete: true
Brian, this is causing lots of signatures, can you look into this?
Flags: needinfo?(bhackett1024)
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update,bisect][fuzzblocker]
Can you get a blame cset?  setObjectMetadataCallback doesn't do anything interesting when it isn't passed any arguments.
Flags: needinfo?(bhackett1024)
Whiteboard: [jsbugmon:update,bisect][fuzzblocker] → [fuzzblocker] [jsbugmon:update]
JSBugMon: Bisection requested, result:
autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   http://hg.mozilla.org/mozilla-central/rev/be1399f8f973
user:        Brian Hackett
date:        Thu May 30 17:37:22 2013 -0600
summary:     Bug 850026 - Allow metadata objects to be associated with JS objects, and add a hook for attaching metadata to newly created objects, r=luke.

This iteration took 1.042 seconds to run.
Setting needinfo on Brian, although this has probably a different cause just like the other bugs with setObjectMetadataCallback. This generates a lot of signatures though so we need to fix it.
Flags: needinfo?(bhackett1024)
Attached patch patch (obsolete) — Splinter Review
Calling ReleaseAllJITCode seems to be missing some pre barrier somewhere.  Since this is only ever used by debugging tools it seems fine to just let any in progress incremental GC finish.
Assignee: general → bhackett1024
Attachment #781824 - Flags: review?(wmccloskey)
Flags: needinfo?(bhackett1024)
Comment on attachment 781824 [details] [diff] [review]
patch

I thought about this a little and I'm not sure it's right. The basic problem is that JSScript has pointers to IonScripts and BaselineScripts. Those aren't GC things, but they have trace methods, and those trace methods trace GC things. So we need to be sure that, any time we overwrite one of these fields, we call the trace function.

This happens correctly in Invalidate, but it seems like there are other places where we don't have a barrier. For example, nukeTypes calls FinishInvalidation, and that doesn't seem to have a barrier. Also, there's a weird barrier inside InvalidateActivation that's only on the method_ field; I don't understand why it's there.

Anyway, I don't understand this code well enough to review. Can you look it over, Jan? As long as ReleaseAllJITCode is the only problem, I think the patch is fine. I'm just worried about other cases.
Attachment #781824 - Flags: review?(wmccloskey) → review?(jdemooij)
Yeah I agree with comment 8; a more targeted fix would be good if possible.
Attachment #781824 - Flags: review?(jdemooij)
Marking critical per comment 8.
Keywords: sec-critical
Attachment #773175 - Attachment is obsolete: true
Attached patch patchSplinter Review
Per comment 8, write barriers around ion/baseline scripts are a mess.
Attachment #781824 - Attachment is obsolete: true
Attachment #784627 - Flags: review?(jdemooij)
Comment on attachment 784627 [details] [diff] [review]
patch

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

Nice, thanks!

::: js/src/jsscriptinlines.h
@@ +162,5 @@
>      enclosingScopeOrOriginalFunction_ = fun;
>  }
>  
> +inline void
> +JSScript::setIonScript(js::ion::IonScript *ionScript) {

Style nit: brace on its own line here and below.
Attachment #784627 - Flags: review?(jdemooij) → review+
Target Milestone: --- → mozilla24
Wrong bug, sorry.
Target Milestone: mozilla24 → ---
Comment on attachment 784627 [details] [diff] [review]
patch

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

I don't think that what this bug is fixing is actually exploitable.  We could miss a pre barrier under some extremely difficult to trigger conditions (not easily controllable OOM).

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?

All Ion branches probably.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

They should be easy, but I don't really know.

How likely is this patch to cause regressions; how much testing does it need?

Unlikely.
Attachment #784627 - Flags: sec-approval?
Comment on attachment 784627 [details] [diff] [review]
patch

sec-approval+ for trunk. 

I'm neutral on whether we need this on aurora or beta but you could nominate it for both and check it in there assuming branch approvals are given.
Attachment #784627 - Flags: sec-approval? → sec-approval+
Oops, thanks for reverting that flag bugzilla.
https://hg.mozilla.org/mozilla-central/rev/6292baed5ec3
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
Flags: in-testsuite?
Status: RESOLVED → VERIFIED
JSBugMon: This bug has been automatically verified fixed.
Let's take this up to Aurora, since its necessity on Beta is debatable.
Never got uplift to Aurora, so untracking completely.
At some point, either now in 25 or next time in 26, we need this on ESR24. If we fix it now we also need it on beta-25.
Flags: needinfo?(jdemooij)
Moving the ESR24 tracking to the Firefox 26 release since Alex won't fixed this for Firefox 25.
I'm getting this compilation failure (narrowed down to this bug) in both gcc 4.6.4 and 4.8.2:

/Volumes/BruceDeuce/src/mozilla-26b/js/src/jsscriptinlines.h: In member function 'void JSScript::setBaselineScript(js::jit::BaselineScript*)':
/Volumes/BruceDeuce/src/mozilla-26b/js/src/jsscriptinlines.h:128:9: error: incomplete type 'js::jit::BaselineScript' used in nested name specifier
         js::jit::BaselineScript::writeBarrierPre(tenuredZone(), baseline);
         ^
This is still not fixed in ESR24 two weeks before release. Can we change this, please?
Whiteboard: [fuzzblocker] [jsbugmon:update] → [fuzzblocker] [jsbugmon:update][adv-main26+]
(In reply to Al Billings [:abillings] from comment #26)
> This is still not fixed in ESR24 two weeks before release. Can we change
> this, please?

Setting needinfo from bhackett.
Flags: needinfo?(bhackett1024)
Comment on attachment 784627 [details] [diff] [review]
patch

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: 
Fix Landed on Version:
Risk to taking this patch (and alternatives if risky): 
String or UUID changes made by this patch: 

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.
Attachment #784627 - Flags: approval-mozilla-esr24?
This was never my bug.
Flags: needinfo?(bhackett1024)
Clearing needinfo. Brian requested approval.
Flags: needinfo?(jdemooij)
Attachment #784627 - Flags: approval-mozilla-esr24? → approval-mozilla-esr24+
This is approved by Alex. Can someone check it in?
Since this is sec-high, the test shouldn't land until six weeks after this release (or in seven weeks) so avoid pwning ourselves.
This hit bustage that I didn't know how to resolve and couldn't get help from #jsapi on. Backed out.
https://hg.mozilla.org/releases/mozilla-esr24/rev/df993b197a7e

https://tbpl.mozilla.org/php/getParsedLog.php?id=31333197&tree=Mozilla-Esr24
Flags: needinfo?(choller) → needinfo?(bhackett1024)
This is the same problem I reported in comment 25. If this helps analysis, for certain files the way to get it to compile was to add #include "jsscript.h" and remove #include "jit/BaselineJIT.h" but obviously this breaks down for compiling stuff in jit/. I eventually had to backout the patch.
The problem looks like Assembler-x86.h unnecessarily including jsscriptinlines.h, creating an include cycle.

https://hg.mozilla.org/releases/mozilla-esr24/rev/a8653b21c44b
Flags: needinfo?(bhackett1024)
Whiteboard: [fuzzblocker] [jsbugmon:update][adv-main26+] → [fuzzblocker] [jsbugmon:update][adv-main26+][adv-esr24.2+]
Group: core-security
You need to log in before you can comment on or make changes to this bug.