Missed SAVE_SP_AND_PC in JSOP_YIELD

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
10 years ago
10 years ago

People

(Reporter: Igor Bukanov, Assigned: Igor Bukanov)

Tracking

({fixed1.8.1.13})

unspecified
fixed1.8.1.13
Points:
---
Bug Flags:
blocking1.9 +
blocking1.8.1.13 +
wanted1.8.1.x +
in-testsuite -
in-litmus -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:moderate])

Attachments

(3 attachments)

(Assignee)

Description

10 years ago
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

10 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

10 years ago
Created attachment 303171 [details] [diff] [review]
v1

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 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

10 years ago
Flags: blocking1.9? → blocking1.9+
(Assignee)

Comment 4

10 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
Last Resolved: 10 years ago
Resolution: --- → FIXED
Flags: blocking1.8.1.13? → blocking1.8.1.13+
Whiteboard: [sg:moderate]

Updated

10 years ago
Flags: in-testsuite-
Flags: in-litmus-
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

10 years ago
Created attachment 306590 [details] [diff] [review]
fix for 1.8

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

10 years ago
Created attachment 306591 [details]
diff between trunk and branch versions
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?
Whiteboard: [sg:moderate] needs 1.8 patch → [sg:moderate]
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

10 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

10 years ago
Keywords: fixed1.8.1.13
(Assignee)

Comment 11

10 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.
Group: security
You need to log in before you can comment on or make changes to this bug.