Closed Bug 824863 Opened 7 years ago Closed 7 years ago

Incorrect behavior on emscripten "stack_byval" test

Categories

(Core :: JavaScript Engine, defect)

x86_64
All
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla20
Tracking Status
firefox17 --- unaffected
firefox18 --- unaffected
firefox19 + fixed
firefox20 + fixed

People

(Reporter: azakai, Assigned: jandem)

References

Details

(Keywords: csectype-wildptr, regression, sec-critical, Whiteboard: [adv-main19-])

Attachments

(3 files)

Attached file testcase
Running the attached benchmark in node produces the right result,

sum:9780*

but in latest spidermonkey an exception is thrown that should not be. In the exception, line 2425 is interesting, that's where in the compiled main() that the problem is shown.

Trying to debug this, I added print() statements near it (print('zz ' + $_2); right before it for example). Adding the print statements changes the behavior, sometimes no error is thrown, sometimes a different error is thrown.

I saw this when running the full emscripten test suite for the first time on x86_64, so perhaps this is specific to there.
Attached file sqlite testcase
Here is another failing case, as before, works in v8 but fails in spidermonkey. This is sqlite, so it is much much bigger. Hopefully this is the same issue as the previous testcase.
Group: core-security
Attached patch PatchSplinter Review
64-bit only regalloc bug in JM's ionCompileHelper. Calling stubcc.syncExitAndJump() before calling frame.allocReg() is wrong because the sync path may store a bogus value. We've had a ton of these bugs and imo we should have added asserts to catch them a long time ago, but overkill to do it at this point.
Assignee: general → jdemooij
Status: NEW → ASSIGNED
Attachment #696534 - Flags: review?(bhackett1024)
Blocks: 785494
(The patch fixes both testcases attached to this bug.)
Attachment #696534 - Flags: review?(bhackett1024) → review+
Thanks for the quick response!

Btw, should I mark correctness bugs as security sensitive in the future?
https://hg.mozilla.org/mozilla-central/rev/5b3f586806b1
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
(In reply to Alon Zakai (:azakai) from comment #5)
>
> Btw, should I mark correctness bugs as security sensitive in the future?

Based on bugs filed by our fuzzer people, I don't think that's necessary. (In this case I marked it as s-s, but it's not easy to exploit and does not affect the beta and release channels.)

And btw it would be great if you could keep running the Emscripten test suite regularly with various SM builds, it has found similar bugs in the past ;)
Comment on attachment 696534 [details] [diff] [review]
Patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #):
Bug 785494

User impact if declined:
Crashes and correctness bugs on 64-bit builds.

Testing completed (on m-c, etc.):
Landed on m-c a few days ago.

Risk to taking this patch (and alternatives if risky):
Very low risk and does not affect 32-bit builds (x86 and ARM) at all.

String or UUID changes made by this patch:
None
Attachment #696534 - Flags: approval-mozilla-aurora?
(In reply to Jan de Mooij [:jandem] from comment #7)
> And btw it would be great if you could keep running the Emscripten test
> suite regularly with various SM builds, it has found similar bugs in the
> past ;)

I run the full test suite before any push to emscripten master, so quite frequently :) What happened here is that I had never run it on x86_64, only x86, so I noticed some new stuff. I'll be running it on both systems in the future most likely now that I have 2 machines, one of each architecture.
(In reply to Alon Zakai (:azakai) from comment #9)
> I run the full test suite before any push to emscripten master, so quite
> frequently :) What happened here is that I had never run it on x86_64, only
> x86, so I noticed some new stuff. I'll be running it on both systems in the
> future most likely now that I have 2 machines, one of each architecture.

Awesome! :)
Do we think this is an sg:crit for Mac/Linux 64-bit builds?
(In reply to Alex Keybl [:akeybl] from comment #11)
> Do we think this is an sg:crit for Mac/Linux 64-bit builds?

It's possible to clobber a JSObject pointer with a bogus pointer somewhere into JSScript or IonScript. JIT code could then mess with these objects and that will be scary. So yeah, I think we should treat this as an sg:crit.

Also note that this broke 2 Emscripten tests and I can imagine it breaking other websites.
Comment on attachment 696534 [details] [diff] [review]
Patch

Approving for Aurora 19. Please land before Monday to make the next merge (or consider this approval applicable to Beta 19 as well).
Attachment #696534 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Whiteboard: [adv-main19-]
Group: core-security
You need to log in before you can comment on or make changes to this bug.