8.35 KB, application/octet-stream
2.35 KB, application/octet-stream
31.14 KB, application/pdf
7.25 KB, patch
|Details | Diff | Splinter Review|
My patch for adding semaphores, my testcase, and the pseudo code for the feasible schedule will come soon.
For the old code in js-1.5, I need 3 threads and 4 semphores to expose this bug order (please refer to the bug 133773 page for more details). However, in latest source code in hg, I just need 2 threads and 2 semaphores. It seems to me that the latest code are more vulnerable to this bug.
Assume only two threads are setup, so after thread T2 remove its context from list, the list becomes empty, so T2 will try to destroy the hash table. However, while T2 is destroying the hash table, the garbage collection of T1 has not finished yet.
The source code version in hg repository is up to 2009-9-8. Especially, jscntxt.cpp is Aug 20, and jsgc.cpp is Aug 24. hg log jscntxt.cpp changeset: 31911:371c845dfd5d user: Jason Orendorff <email@example.com> date: Thu Aug 20 14:13:21 2009 -0500 summary: Bug 511418 - static-analysis error in jsobj.cpp:4257: cannot access JS_REQUIRES_STACK variable JSContext::fp and another trivial error in jstracer.cpp. r=gal. hg log jsgc.cpp changeset: 31939:7b7f9ae673cf user: David Anderson <firstname.lastname@example.org> date: Mon Aug 24 17:09:44 2009 -0700 summary: Removed JSStackFrame::callee (bug 512029, r=brendan).
Dear Developers, Is my description clear? If there is any another information I need to provide, please tell me. Thanks, Heming
Heming, thanks for all the work -- I have not had time to look yet, way too busy with other commitments. I'm hoping Igor has time. /be
Assignee: general → igor
Status: UNCONFIRMED → NEW
Ever confirmed: true
The fix just removes the call to FinishRuntimeScriptState and this bug race from js_DestroyContext. The same job is done when JS_DestroyContext calls js_FreeRuntimeScriptState.
The new version moves clearance of rt->builfinFunctions into js_GC as doing that in js_DestroyContext has the same race as this bug and theoretically could lead to a shutdown leak. The patch also removes useless js_UnlockGCThing from js_FinishNumberState as NaN and +-oo are long explicitly marked.
(In reply to comment #8) > Created an attachment (id=402557) [details] > fix v1 > > The fix just removes the call to FinishRuntimeScriptState and this bug race > from js_DestroyContext. The same job is done when JS_DestroyContext calls > js_FreeRuntimeScriptState. JS_DestroyRuntime calls js_FreeRuntimeScriptState, rather -- which leaves the space tied up during zero-contexts-active-on-runtime intervals, which was what the over-complicated code patched by this bug's patch was trying to avoid. Isn't there a race-free way to save that space, by analogy to the clearing of rt->builtinFunctions in the patch, by calling js_FreeRuntimeScriptState from js_GC? /be
(In reply to comment #10) > JS_DestroyRuntime calls js_FreeRuntimeScriptState, rather -- which leaves the > space tied up during zero-contexts-active-on-runtime intervals, which was what > the over-complicated code patched by this bug's patch was trying to avoid. The entries from rt->scriptFilenameTable are removed when they are swept using JS_HashTableEnumerateEntries. That function shrinks the hashtable when necessary leaving just MINBUCKETS or 16 entries in the hasharray when all entries are removed. Thus for an embedding that is frequently in the zero context state, the space that the patch waste is minuscule. Moreover, for such embedding the patch would avoid recreation of the table after compiling a new script. > > Isn't there a race-free way to save that space, by analogy to the clearing of > rt->builtinFunctions in the patch, by calling js_FreeRuntimeScriptState from > js_GC? This is what I did in the initial version of the patch until I realized that that code is not necessary due to the above reasoning.
Comment on attachment 402564 [details] [diff] [review] fix v2 Ok, maybe a comment about the space held being tiny is appropriate. r=me, thanks. /be
Attachment #402564 - Flags: review?(brendan) → review+
http://hg.mozilla.org/tracemonkey/rev/3fc33a44b5ef I nominate this for branches as this bug race could lead to accessing freed memory in some embedding or with Firefox addons. Note that in the browser itself the race cannot happen as thread workers are shutdown strictly before the last context is destroyed.
Whiteboard: [ccbr] → [ccbr] fixed-in-tracemonkey
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
(In reply to comment #6) > Dear Developers, > Is my description clear? If there is any another information I need to > provide, please tell me. > > Thanks, > Heming Heming, I am a PhD student from UCF and my research involves techniques for debugging. I would like to study this bug, but I am new to Mozilla. Can you please help me reproduce the bug. Here is what I have so far: - I checked out an old version of the code from 9-8-2009. $ hg clone http://hg.mozilla.org/releases/comm-1.9.1/ $ python client.py --comm-rev=b7f6edb75fa --mozilla-rev=dd9b10ed7eeb checkout - Then I applied your patch with the semaphores and finally, I compiled mozilla. - However, I am not able to compile your test program: test-js.cpp Can u please tell me how to compile and run that, in order to trigger the bug. Thank you very much, Martin
Hi, I figured out how to reproduce the bug. Thanks, Martin
You need to log in before you can comment on or make changes to this bug.