Closed Bug 846984 Opened 7 years ago Closed 7 years ago

Crash [@ js_NewStringCopyN] with a threadsafe js shell

Categories

(Core :: JavaScript Engine, defect, critical)

defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla22
Tracking Status
firefox19 --- unaffected
firefox20 --- unaffected
firefox21 + fixed
firefox22 + fixed
firefox-esr17 --- unaffected
b2g18 + affected
b2g18-v1.0.0 --- wontfix
b2g18-v1.0.1 --- affected

People

(Reporter: gkw, Assigned: Benjamin)

References

(Blocks 1 open bug)

Details

(Keywords: crash, regression, testcase, Whiteboard: [jsbugmon:])

Crash Data

Attachments

(2 files)

Attached file debug and opt stacks
for (var y = 0; y < 999999; y++) {
    evalcx("print(x);function x(...x){}", newGlobal(''))
}

crashes js debug and opt shell on m-c changeset 3362afba690e without any CLI arguments at js_NewStringCopyN
I just verified that this seems to only occur with threadsafe js shells.
Summary: Crash [@ js_NewStringCopyN] → Crash [@ js_NewStringCopyN] with a threadsafe js shell
And I also verified that 32-bit and 64-bit Windows shells are affected, but Linux and Mac shells do not seem to be affected.
Hardware: x86 → All
Summary: Crash [@ js_NewStringCopyN] with a threadsafe js shell → Crash [@ js_NewStringCopyN] with a threadsafe js shell on Windows
Whiteboard: [jsbugmon:update] → [jsbugmon:]
JSBugMon: Cannot process bug: Unable to automatically reproduce, please track manually.
A faster testcase:

for (var y = 0; y < 9999; y++) {
    evalcx("print(x);function x(...x){}", newGlobal(''))
}
Even faster one:

for (var y = 0; y < 999; y++) {
    evalcx("print(x);function x(...x){}", newGlobal(''))
}


autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   121521:79ebe1a5d8f4
user:        Ms2ger
date:        Tue Feb 12 11:14:01 2013 +0100
summary:     Bug 837176 - Simplify code flow in CheckSideEffects; r=jorendorff

Ms2ger, is bug 837176 a likely regressor?
I would have hoped not... Jason?
(In reply to :Ms2ger from comment #6)
> I would have hoped not... Jason?

I'm not so sure anymore either. :-/ It is an unstable testcase. I changed the for loop to 99999:

for (var y = 0; y < 99999; y++) {
    evalcx("print(x);function x(...x){}", newGlobal(''))
}

and have restarted autoBisect. More tomorrow.
I have managed to get a second possible regressor from the testcase in comment 7:

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   121159:71f14579265f
user:        Benjamin Peterson
date:        Thu Feb 07 09:29:22 2013 -0500
summary:     Bug 836515 - Allow source compression to run while executing the script. r=jorendorff

jorendorff, you're probably the best person to evaluate the regression ranges.
Flags: needinfo?(jorendorff)
It's more likely related to 71f14579265f. Unfortunately, I don't have Windows, so I probably won't be able to debug this.
Flags: needinfo?(jorendorff)
Flags: needinfo?(jorendorff)
Blocks: 836515
No longer blocks: 837176
Assuming sec-critical until otherwise shown. (there are some indications from the stack that it might just be a null deref, or not.. so let's have someone look at this first)
Keywords: sec-critical
Assignee: general → jorendorff
Adjusting flags and nominating for tracking on b2g, apparently bug 836515 landed on b2g as a performance improvement.
Attachment #724238 - Flags: review?(jorendorff)
Attachment #724238 - Flags: feedback?(gary)
Comment on attachment 724238 [details] [diff] [review]
tweak some defines

This seems to fix the crash for me.
Attachment #724238 - Flags: feedback?(gary) → feedback+
Comment on attachment 724238 [details] [diff] [review]
tweak some defines

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

This is fine, but if you think it's less error-prone to make all the compression *and* threading code conditional on having both USE_ZLIB and JS_THREADSAFE (thus supporting only two variants rather than four), please do that instead.
Attachment #724238 - Flags: review?(jorendorff) → review+
Flags: needinfo?(jorendorff)
Definitely not security-sensitive.
Group: core-security
Post-developer analysis, I guess this no longer needs to land on b2g, but this will be nice to have on the Aurora 21 branch though.
Flags: needinfo?(lsblakk)
I don't think it needs to land on anything except central unless it blocks fuzzing. The patch is simple, but the bug doesn't afflict the browser.
(In reply to :Benjamin Peterson from comment #17)
> I don't think it needs to land on anything except central unless it blocks
> fuzzing. The patch is simple, but the bug doesn't afflict the browser.

Yes, this blocks fuzzing on Windows (which is how I found this bug in the first place) on the Aurora 21 branch. I think they accept fuzzing blockers when requesting for landing approval.
I could make this trigger (via another unreliably large testcase) on Linux as well, but it was a lot more unreliable.

I can somewhat confirm that the patch fixes this issue, too, on Linux.
Assignee: jorendorff → benjamin
Status: NEW → ASSIGNED
Keywords: sec-critical
OS: Windows 7 → All
Summary: Crash [@ js_NewStringCopyN] with a threadsafe js shell on Windows → Crash [@ js_NewStringCopyN] with a threadsafe js shell
Comment on attachment 724238 [details] [diff] [review]
tweak some defines

[Approval Request Comment]
Blocks fuzzing on auroa. Very simple patch, which doesn't change browser.
Attachment #724238 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/bd8615bc2be5
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
Attachment #724238 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.