TM: Heap corruption with gczeal, yield

VERIFIED FIXED

Status

()

Core
JavaScript Engine
--
critical
VERIFIED FIXED
9 years ago
3 years ago

People

(Reporter: Jesse Ruderman, Assigned: gal)

Tracking

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

Trunk
x86
Mac OS X
crash, testcase
Points:
---
Bug Flags:
in-testsuite ?
in-litmus -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:critical])

Attachments

(4 attachments, 1 obsolete attachment)

(Reporter)

Description

9 years ago
Created attachment 341515 [details]
testcase

$ ~/tracemonkey/js/src/Darwin_DBG.OBJ/js -j a5.js
js(40337) malloc: *** error for object 0x302f90: incorrect checksum for freed object - object was probably modified after being freed.
*** set a breakpoint in malloc_error_break to debug
Crash [@ szone_free] attempting to access 0xfffff804

$ valgrind ~/tracemonkey/js/src/Darwin_DBG.OBJ/js -j a5.js
Invalid write of size 4 at nanojit::Fragment::blacklist
Address was freed during GC js_FlushJITCache
(Reporter)

Comment 1

9 years ago
Created attachment 341516 [details]
valgrind output

14 invalid reads, 2 invalid writes.
(Reporter)

Updated

9 years ago
Whiteboard: [sg:critical]
(Reporter)

Comment 2

9 years ago
Still happens on tracemonkey branch after nanojit2 landing.
Assignee: general → danderson
Created attachment 344384 [details] [diff] [review]
proposed fix

If js_Interpret sees that the JIT cache was flushed, it sets a "safe delete" flag in addition to the deep abort flag, which causes it to not read from its fragment.
Attachment #344384 - Flags: review?(brendan)
(Whoops, ignore that erroneous "pendingFlush")
Comment on attachment 344384 [details] [diff] [review]
proposed fix

>diff -r 35c34996d80e js/src/jscntxt.h
>--- a/js/src/jscntxt.h	Tue Oct 21 15:21:23 2008 -0700
>+++ b/js/src/jscntxt.h	Wed Oct 22 15:30:57 2008 -0700
>@@ -141,6 +141,8 @@ typedef struct JSTraceMonitor {
>     JSFragmentCacheEntry    fcache[JS_FRAGMENT_CACHE_SIZE];
>     jsval                   *recoveryDoublePool;
>     jsval                   *recoveryDoublePoolPtr;
>+    uint32                  cacheId;
>+    JSBool                  pendingFlush;

Deadwood as noted.

>+    uint32 traceMonCacheId = JS_TRACE_MONITOR(cx).cacheId;

Suggest jitCacheId as name (to match js_FlushJITCache).

>+        if (fragment->root == fragment

isRootFragment should be equivalent and faster to test?

>+    if (f == NULL) {

Nit: !f as noted.

>+    tm->cacheId++;

cacheId is a fine name, some slight pref for cacheGen (generation) to match other generation-number-based cache flushing in SpiderMonkey.

r=me with above addressed.

/be
Attachment #344384 - Flags: review?(brendan) → review+
Pushed changeset: http://hg.mozilla.org/tracemonkey/rev/dcf3b1f789e8
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
(Assignee)

Comment 7

9 years ago
Created attachment 345187 [details] [diff] [review]
improved patch
Assignee: danderson → gal
Attachment #344384 - Attachment is obsolete: true
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Updated

9 years ago
Attachment #345187 - Flags: review?(danderson)
(Assignee)

Updated

9 years ago
Attachment #345187 - Flags: review?(brendan)
Comment on attachment 345187 [details] [diff] [review]
improved patch

Easy come, easy go! Srsly, do we have a test for the bug that was fixed by the jitCacheGen patch?

/be
Attachment #345187 - Flags: review?(brendan) → review+
Attachment #345187 - Flags: review?(danderson) → review+
(Assignee)

Comment 9

9 years ago
http://hg.mozilla.org/tracemonkey/rev/fca419901097
Status: REOPENED → RESOLVED
Last Resolved: 9 years ago9 years ago
Resolution: --- → FIXED

Comment 10

9 years ago
Created attachment 348000 [details]
js1_7/extensions/regress-458288.js

Updated

9 years ago
Flags: in-testsuite+
Flags: in-litmus-

Comment 11

9 years ago
verified fixed mozilla-central, tracemonkey
Status: RESOLVED → VERIFIED

Comment 12

8 years ago
when this bug is opened, the test should be checked in.
Flags: in-testsuite+ → in-testsuite?
Group: core-security
You need to log in before you can comment on or make changes to this bug.