The default bug view has changed. See this FAQ.

Unbalanced js_Enter/LeaveLocalRootScope in js_ErrorToException

VERIFIED FIXED

Status

()

Core
JavaScript Engine
VERIFIED FIXED
11 years ago
10 years ago

People

(Reporter: Igor Bukanov, Assigned: Igor Bukanov)

Tracking

({verified1.8.0.9, verified1.8.1.1})

Trunk
verified1.8.0.9, verified1.8.1.1
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.8.1.1 +
blocking1.8.0.9 +
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:moderate])

Attachments

(3 attachments, 2 obsolete attachments)

(Assignee)

Description

11 years ago
The code in js_ErrorToException from jsexn.c contains:

    /* Protect the newly-created strings below from nesting GCs. */
    ok = js_EnterLocalRootScope(cx);
    if (!ok)
        goto out;
...
  out:
    js_LeaveLocalRootScope(cx);

That is, it calls js_LeaveLocalRootScope even when the corresponding js_EnterLocalRootScope fails.

Since on tight memory it can be used to remove local root protection, I mark the bug as security-related.
(Assignee)

Comment 1

11 years ago
Created attachment 243641 [details] [diff] [review]
Fix v1

The patch changes js_ErrorToException to use temp roots instead of EnterLocalRoot which works since JS_PUSH_TEMP_ROOT cannot fail.

Effectively the patch changes js_ErrorToException to the AVIARY_1_0_1_01252004_BRANCH version since there EnterLocalRoot is not available so the fix for bug 312278 used JS_PUSH_TEMP_ROOT. The only difference compared with AVIARY_1_0_1_01252004_BRANCH is that the patch is slightly paranoid and uses 4 temp roots instead of 2 to ensure the same rooting properties as EnterLocalRoot provides.
Attachment #243641 - Flags: review?(brendan)
(Assignee)

Updated

11 years ago
Flags: blocking1.8.1.1?
Flags: blocking1.8.0.9?
(Assignee)

Updated

11 years ago
Attachment #243641 - Flags: approval1.8.1.1?
Comment on attachment 243641 [details] [diff] [review]
Fix v1

Argh.

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

Comment 3

11 years ago
Created attachment 243713 [details] [diff] [review]
Fix v2

Compared with v1, this patch moves JS_SetPendingException after the exception object initialization code. In this way an Out-of-Memory error during the initialization would not be catchable by scripts.

Previously the early call to JS_SetPendingException was done to root the exception. But this is not necessary since the object is rooted in any case.
Attachment #243641 - Attachment is obsolete: true
Attachment #243713 - Flags: review?(brendan)
Attachment #243713 - Flags: approval1.8.1.1?
Attachment #243641 - Flags: approval1.8.1.1?
Comment on attachment 243713 [details] [diff] [review]
Fix v2

I should have seen that.  Nice reordering of errorString's decl too.

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

Comment 5

11 years ago
I committed the patch from comment 3 to the trunk:

Checking in jsexn.c;
/cvsroot/mozilla/js/src/jsexn.c,v  <--  jsexn.c
new revision: 3.79; previous revision: 3.78
done
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
(Assignee)

Comment 6

11 years ago
Created attachment 243728 [details] [diff] [review]
Fix v3

Patch to commit with comments fixed.
Attachment #243713 - Attachment is obsolete: true
Attachment #243728 - Flags: review+
Attachment #243713 - Flags: approval1.8.1.1?
(Assignee)

Comment 7

11 years ago
(In reply to comment #6)
> Created an attachment (id=243728) [edit]
> Fix v3
> 
> Patch to commit with comments fixed.
> 

Please ignore this: wrong bug!
(Assignee)

Updated

11 years ago
Attachment #243728 - Attachment is obsolete: true
(Assignee)

Updated

11 years ago
Attachment #243713 - Attachment is obsolete: false

Updated

11 years ago
Flags: in-testsuite-
Is "fix v2" ready for the braches, or will we need a merged/ported version? Please ask for "approval?" on the appropriate patch.
Flags: blocking1.8.1.1?
Flags: blocking1.8.1.1+
Flags: blocking1.8.0.9?
Flags: blocking1.8.0.9+
(Assignee)

Comment 9

11 years ago
(In reply to comment #8)
> Is "fix v2" ready for the braches, or will we need a merged/ported version?
> Please ask for "approval?" on the appropriate patch.

The patch would apply to 1.8.1 branch as is if one applies the patch from bug 347248 first. 1.8.0 on the other hand requires some porting.
Blocks: 355044
Depends on: 347248
(Assignee)

Updated

11 years ago
Attachment #243713 - Flags: approval1.8.1.1?
(Assignee)

Comment 10

11 years ago
Created attachment 244842 [details] [diff] [review]
Fix v2 for 1.8.0 branch

Non-trivial porting requires an extra review.
Attachment #244842 - Flags: review?(brendan)
(Assignee)

Comment 11

11 years ago
Created attachment 244843 [details] [diff] [review]
diff between Fix v2 and Fix v2 for 1.8.0
(Assignee)

Updated

11 years ago
Attachment #244842 - Flags: approval1.8.0.9?
Comment on attachment 244842 [details] [diff] [review]
Fix v2 for 1.8.0 branch

Looks good -- too bad JS_ARRAY_LENGTH is not on the 1.8.0 branch.

/be
Attachment #244842 - Flags: review?(brendan) → review+
Calling this "sg:moderate" because the exploitable conditions seem hard to set up.

I'm happy taking these patches, but given the tight schedule for 2.0.0.1 (1.8.1.1) we're uncomfortable taking bug 347248 along with it. Can the two bugs be done separately, or are there extra reviews or testing that can reassure us about bug 347248?
Whiteboard: [sg:moderate]
(Assignee)

Comment 14

11 years ago
(In reply to comment #13)
> Calling this "sg:moderate" because the exploitable conditions seem hard to set
> up.
> 
> I'm happy taking these patches, but given the tight schedule for 2.0.0.1
> (1.8.1.1) we're uncomfortable taking bug 347248 along with it. Can the two bugs
> be done separately, 

Yes: in fact the patch 1.8.0 is almost the patch for 1.8.1 as well if 347248 would not be taken in.

> or are there extra reviews or testing that can reassure us
> about bug 347248?

Well, the optimization patch affects the frequently executed code path. 

Yes, in the initial form I introduced a regression there, but it was very quickly pointed-out due-to this frequently executed property. Moreover, the regression caused to consider the patch few more times by me and Brendan adding the confidence on top of the fact that the patch exists on the trunk with all regression fixed for more then 2 months.
(In reply to comment #13)
> Calling this "sg:moderate" because the exploitable conditions seem hard to set
> up.
> 
> I'm happy taking these patches, but given the tight schedule for 2.0.0.1
> (1.8.1.1) we're uncomfortable taking bug 347248 along with it. Can the two bugs
> be done separately, or are there extra reviews or testing that can reassure us
> about bug 347248?

Bug 347248 is well-baked.  Recommend you take it and not make yet another variation ("almost the same as 1.8.0" is not the same) to engineer and support.  Take the known quantity.

/be
Comment on attachment 243713 [details] [diff] [review]
Fix v2

approved for 1.8/1.8.0 branch, a=dveditz for drivers
Attachment #243713 - Flags: approval1.8.1.1? → approval1.8.1.1+
Attachment #244842 - Flags: approval1.8.0.9? → approval1.8.0.9+
(Assignee)

Comment 17

11 years ago
I committed the patch from comment 3 to MOZILLA_1_8_BRANCH:

Checking in jsexn.c;
/cvsroot/mozilla/js/src/jsexn.c,v  <--  jsexn.c
new revision: 3.50.4.11; previous revision: 3.50.4.10
done
Keywords: fixed1.8.1.1
(Assignee)

Comment 18

11 years ago
I committed the patch from comment 10 to MOZILLA_1_8_0_BRANCH:

Checking in jsexn.c;
/cvsroot/mozilla/js/src/jsexn.c,v  <--  jsexn.c
new revision: 3.50.4.4.2.4; previous revision: 3.50.4.4.2.3
done
Keywords: fixed1.8.0.9

Comment 19

11 years ago
verified fixed 1.8.0.9, 1.8.1.1, 1.9 by code inspection.
Status: RESOLVED → VERIFIED
Keywords: fixed1.8.0.9, fixed1.8.1.1 → verified1.8.0.9, verified1.8.1.1
Group: security

Comment 20

10 years ago
... not applicable to 1.7 branch !?
(Assignee)

Comment 21

10 years ago
(In reply to comment #20)
> ... not applicable to 1.7 branch !?

No, js_EnterLocalRootScope API was introduced for 1.8.* and later. To root temporaries 1.7 branch uses JS_PUSH_TEMP_ROOT API and the corresponding line never fails.   
You need to log in before you can comment on or make changes to this bug.