Closed
Bug 417377
Opened 16 years ago
Closed 16 years ago
Missed SAVE_SP_AND_PC in JSOP_YIELD
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
People
(Reporter: igor, Assigned: igor)
Details
(Keywords: fixed1.8.1.13, Whiteboard: [sg:moderate])
Attachments
(3 files)
3.62 KB,
patch
|
brendan
:
review+
brendan
:
approval1.9+
|
Details | Diff | Splinter Review |
3.63 KB,
patch
|
brendan
:
review+
dveditz
:
approval1.8.1.13+
|
Details | Diff | Splinter Review |
3.93 KB,
text/plain
|
Details |
Here is the JSOP_YIELD case from js_Interpret: BEGIN_CASE(JSOP_YIELD) ASSERT_NOT_THROWING(cx); if (FRAME_TO_GENERATOR(fp)->state == JSGEN_CLOSING) { js_ReportValueError(cx, JSMSG_BAD_GENERATOR_YIELD, JSDVG_SEARCH_STACK, fp->argv[-2], NULL); ok = JS_FALSE; goto out; } fp->rval = FETCH_OPND(-1); fp->flags |= JSFRAME_YIELDING; pc += JSOP_YIELD_LENGTH; SAVE_SP_AND_PC(fp); Here SAVE_SP_AND_PC(fp) is not called before reporting an error. As such a freshly allocated GC thing on the stack may become unrooted leading to a GC hazard if a GC is triggered twice during js_ReportValueError.
Flags: blocking1.9?
Flags: blocking1.8.1.13?
Assignee | ||
Comment 1•16 years ago
|
||
(In reply to comment #0) > Here SAVE_SP_AND_PC(fp) is not called before reporting an error. As such a > freshly allocated GC thing on the stack may become unrooted leading to a GC > hazard if a GC is triggered twice during js_ReportValueError. This is wrong. The hazard can only happen if the collected value is accessed. With the current it is only possible when a debugger installs a call handler and the handler executes a GC. Only in this case the stack slot with a collected during js_ReportValueError value can be accessed.
Assignee | ||
Comment 2•16 years ago
|
||
There is another missing SAVE_SP_AND_PC in JSOP_ARRAYPUSH with the same consequences: the hazard is possible only if a debugger installs a handler that runs a GC. Plus in the patch I removed default: case in the getter switch to save the code and less time to check that SAVE_SP_AND_PC is not missed there.
Attachment #303171 -
Flags: review?(brendan)
Comment 3•16 years ago
|
||
Comment on attachment 303171 [details] [diff] [review] v1 Looks good, thanks. /be
Attachment #303171 -
Flags: review?(brendan)
Attachment #303171 -
Flags: review+
Attachment #303171 -
Flags: approval1.9+
Updated•16 years ago
|
Flags: blocking1.9? → blocking1.9+
Assignee | ||
Comment 4•16 years ago
|
||
I checked in the patch from comment 2 to the trunk: http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&branch=HEAD&cvsroot=%2Fcvsroot&date=explicit&mindate=1203003360&maxdate=1203003362& who=igor%25mir2.org
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Updated•16 years ago
|
Flags: blocking1.8.1.13? → blocking1.8.1.13+
Whiteboard: [sg:moderate]
Updated•16 years ago
|
Flags: in-testsuite-
Flags: in-litmus-
Comment 5•16 years ago
|
||
Is there a chance of getting a 1.8 patch by the March 7 code-freeze? If not I suppose we can wait another cycle for a "moderate" bug.
Flags: wanted1.8.1.x+
Whiteboard: [sg:moderate] → [sg:moderate] needs 1.8 patch
Assignee | ||
Comment 6•16 years ago
|
||
Compared with the trunk version the version for 1.8 the patch does not need to fix trunk-only JSOP_ARRAYPUSH. In addition there is no ASSERT_SAVED_SP_AND_PC on the branch so the patch backports it.
Attachment #306590 -
Flags: review?(brendan)
Assignee | ||
Comment 7•16 years ago
|
||
Comment 8•16 years ago
|
||
Comment on attachment 306590 [details] [diff] [review] fix for 1.8 >- case JSOP_INITELEM: >+ default: >+ JS_ASSERT(op2 == JSOP_INITELEM); FYI, see bug 356378 and bug 380432. This patch is good, but we want further fixing of one or both of the above. /be
Attachment #306590 -
Flags: review?(brendan)
Attachment #306590 -
Flags: review+
Attachment #306590 -
Flags: approval1.8.1.13?
Updated•16 years ago
|
Whiteboard: [sg:moderate] needs 1.8 patch → [sg:moderate]
Comment 9•16 years ago
|
||
Comment on attachment 306590 [details] [diff] [review] fix for 1.8 approved for 1.8.1.13, a=dveditz for release-drivers
Attachment #306590 -
Flags: approval1.8.1.13? → approval1.8.1.13+
Assignee | ||
Comment 10•16 years ago
|
||
I checked in the patch from comment 6 to the trunk: Checking in jsinterp.c; /cvsroot/mozilla/js/src/jsinterp.c,v <-- jsinterp.c new revision: 3.181.2.97; previous revision: 3.181.2.96 done
Assignee | ||
Updated•16 years ago
|
Keywords: fixed1.8.1.13
Assignee | ||
Comment 11•16 years ago
|
||
(In reply to comment #10) > I checked in the patch from comment 6 to the trunk: I meant MOZILLA_1_8_BRANCH, not trunk.
Updated•16 years ago
|
Group: security
You need to log in
before you can comment on or make changes to this bug.
Description
•