Crash [@ JSC::ExecutablePool::~ExecutablePool] or [@ delete_<JSC::ExecutablePool>]

VERIFIED FIXED in Firefox 10

Status

()

defect
--
critical
VERIFIED FIXED
8 years ago
6 years ago

People

(Reporter: gkw, Assigned: cdleary)

Tracking

(Blocks 1 bug, {crash, regression, testcase})

Trunk
mozilla11
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox9 unaffected, firefox10 verified, firefox11 verified, firefox-esr10 unaffected, status1.9.2 unaffected)

Details

(Whiteboard: [sg:critical][qa!], crash signature)

Attachments

(7 attachments)

Reporter

Description

8 years ago
Posted file stacks
function g(code) {
    eval("(function(){" + code + "})")()
}
g("evalcx(\"(/a/g,function(){})\")");
g("'a'.replace(/a/g,gc)")

crashes js debug shell on m-c changeset eb84780783ed without any CLI arguments at JSC::ExecutablePool::~ExecutablePool and js opt shell at delete_<JSC::ExecutablePool>

s-s just to be safe even though this seems to be a null crash, scary function delete_ is on the opt shell stack.

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   79966:366d80e91816
user:        Chris Leary
date:        Mon Nov 07 13:42:31 2011 -0800
summary:     Bug 634654 - Add RegExpPrivate cache. (r=Waldo)
Reporter

Comment 1

8 years ago
Happens on Mac too.
Crash Signature: [@ JSC::ExecutablePool::~ExecutablePool] [@ delete_<JSC::ExecutablePool>]
OS: Linux → All
Hardware: x86 → All
Reporter

Updated

8 years ago
Attachment #574416 - Attachment description: stack → Mac Valgrind opt stacks - shows Invalid free() / delete / delete[] / realloc()
Very nice find. I think I have to back my patches out of Aurora because of this gaffe. Thread data level cache outlives the jaegerCompartment. I totally forgot about that constraint because I didn't have to touch the ExecutableAllocator acquisition path.

That change would be too much to land on Aurora. Backout diffs forthcoming, which will negate the aurora approval flag on those other RegExp bugs. I'll also create a fix for Nightly.
Oh, and I have to back out the lazification change as well because backing out the regexp cache regresses our V8-regexp score.
It's a simple fix, but we'll want to observe this on nightlies instead of blindly applying to Aurora.
Assignee: general → cdleary
Status: NEW → ASSIGNED
Attachment #574497 - Flags: review?(luke)
Note to self: add the wonderful test before committing!
Reversed in this patch (newest to oldest):

366d80e91816: RegExpPrivate cache
2da44633e0bb: Eagerly compile clone source
d50bd6d5b097: Style followup
61dd23c012ee: Lazy regexps

Interdiffs to follow for ease of review.
Attachment #574508 - Flags: approval-mozilla-aurora?
(Oh, and I confirmed that the v8-regexp score was restored to its original value.)
I couldn't interdiff this one successfully, so I've attached the conflict hunk for comparison with the folded patch. Sorry these are kind of boring, but I figured I'd err on the side of documenting / getting easy reviews on things.
Attachment #574514 - Flags: review?(luke)
Comment on attachment 574497 [details] [diff] [review]
Hoist executable allocator to thread data level.

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

::: js/src/vm/RegExpObject-inl.h
@@ +380,5 @@
>  #ifdef JS_METHODJIT
>      if (isJITRuntimeEnabled(cx) && !yarrPattern.m_containsBackreferences) {
> +        JSC::ExecutableAllocator *execAlloc = cx->threadData()->getOrCreateExecutableAllocator();
> +        if (!execAlloc) {
> +            js_ReportOutOfMemory(cx);

Instead of storing the rt in the threadData and having to report oom at the callsite, can you just have a JSContext* parameter to these functions?
Attachment #574497 - Flags: review?(luke) → review+

Updated

8 years ago
Attachment #574512 - Flags: review?(luke) → review+

Updated

8 years ago
Attachment #574513 - Flags: review?(luke) → review+

Updated

8 years ago
Attachment #574514 - Flags: review?(luke) → review+
(In reply to Luke Wagner [:luke] from comment #12)
> Instead of storing the rt in the threadData and having to report oom at the
> callsite, can you just have a JSContext* parameter to these functions?

Good idea! I still need to store rt in the ThreadData for deallocation in the destructor, though.
Adding Kairo so he can see the relevant backout patch. (Kairo, you were asking about it in bug 701761 comment 10.)
Fix for trunk: https://hg.mozilla.org/integration/mozilla-inbound/rev/ebc6ef282df4

Note: will land the testcase after we back out on Aurora.
Whiteboard: js-triage-needed

Comment 17

8 years ago
Comment on attachment 574508 [details] [diff] [review]
Back out laziness and related patches; folded.

[triage comment]
Approved for aurora. If this needs to be landed for Fx9, please request beta as well.
Attachment #574508 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Take two, landing on m-i for FF11 (nightly). Shell build didn't do threadsafe.

https://hg.mozilla.org/integration/mozilla-inbound/rev/6157fc56d02c
(In reply to Chris Leary [:cdleary] from comment #18)
> Take two, landing on m-i for FF11 (nightly). Shell build didn't do
> threadsafe.
> 
> https://hg.mozilla.org/integration/mozilla-inbound/rev/6157fc56d02c

its also now on mozilla-central -> https://hg.mozilla.org/mozilla-central/rev/6157fc56d02c
Assuming exploitable based on comment 2
Whiteboard: [sg:critical]
Backout pushed to aurora: https://hg.mozilla.org/releases/mozilla-aurora/rev/3f725329f26d
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
Now that we've landed on aurora I've landed the test on inbound:

https://hg.mozilla.org/integration/mozilla-inbound/rev/cee431a0f087
I think the right status for backout is |fixed|...
Whiteboard: [sg:critical] → [sg:critical][qa+]
Target Milestone: --- → mozilla11
Verified fixed for using Firefox 10.0 and 11.0 debug js-shells.
Whiteboard: [sg:critical][qa+] → [sg:critical][qa!]
Reporter

Comment 27

7 years ago
(In reply to Anthony Hughes, Mozilla QA (irc: ashughes) from comment #26)
> Verified fixed for using Firefox 10.0 and 11.0 debug js-shells.

Also setting to VERIFIED.
Status: RESOLVED → VERIFIED
Group: core-security
A testcase for this bug was automatically identified at js/src/jit-test/tests/basic/bug702426-regexp-gc.js.
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.