Problematic branch callback calls from the last ditch GC

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
11 years ago
11 years ago

People

(Reporter: Igor Bukanov, Assigned: Igor Bukanov)

Tracking

({fixed1.8.0.7, fixed1.8.1})

Trunk
fixed1.8.0.7, fixed1.8.1
Points:
---
Bug Flags:
blocking1.8.1 +
blocking1.8.0.7 +
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:critical][need testcase])

Attachments

(2 attachments)

(Assignee)

Description

11 years ago
The last ditch GC must preserve cx->newborn array and keep all atoms. But as far as I can see calling branch callback can break this since there is very little restrictions on what the callback can do. In particular, it can call and JS_MaybeGC() or even JS_GC() wiping out all newborn arrays. 

In particular, the branch callback in the browser, http://lxr.mozilla.org/seamonkey/source/dom/src/base/nsJSEnvironment.cpp#654 ,  calls JS_MaybeGC and potentially install the debugger. The former would GC all newborns with WAY_TOO_MUCH_GC defined (when JS_MaybeGC is just JS_GC) and the later AFAICS can allocate GC things (this is the reason I mark the bug as a security problem).

This has to be addressed somehow. The simplest and IMO the best option would be just to disable the callback on the last ditch GC. Instead explicit calls to the callback should be issued from other points where it is safe to call the callback.
Yes, this is just a big bad bug, courtesy of my big block head.  The bug I sought to fix was a kind of D.O.S testcase where there weren't enough branches in scripted code for the old DOM branch callback monitor, which counted branches, to catch the expensive native branches or other native operations.  See bug 203278 comment 27.  Of course, we've changed the DOM branch callback to monitor real time so this is all unmotivated now.

Let's turn it off, quickly.  If it helps to clean up control flow, rip out anything that distorted GC-locking in order to "rotate" the branch callback down from the allocator to the end of the collector.

Sorry for the mess.

/be
Should be fixed on branches too.

/be
Flags: blocking1.8.1?
Flags: blocking1.8.0.7?

Updated

11 years ago
Flags: blocking1.8.1? → blocking1.8.1+
(Assignee)

Comment 3

11 years ago
Created attachment 231895 [details] [diff] [review]
Fix
Assignee: general → igor.bukanov
Status: NEW → ASSIGNED
Attachment #231895 - Flags: review?(brendan)
Comment on attachment 231895 [details] [diff] [review]
Fix

Nice -- nominating.  The 1.8.0 branch will need a different patch.

/be
Attachment #231895 - Flags: review?(brendan)
Attachment #231895 - Flags: review+
Attachment #231895 - Flags: approval1.8.1?
Attachment #231895 - Flags: approval1.8.0.7?

Comment 5

11 years ago
Comment on attachment 231895 [details] [diff] [review]
Fix

a=schrep for drivers.
Attachment #231895 - Flags: approval1.8.1? → approval1.8.1+
(Assignee)

Comment 6

11 years ago
I committed to the trunk the patch from comment 3.
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
(Assignee)

Comment 7

11 years ago
I committed the patch from comment 3 to MOZILLA_1_8_BRANCH
Keywords: fixed1.8.1

Updated

11 years ago
Flags: in-testsuite-
Flags: blocking1.8.0.7? → blocking1.8.0.7+
(Assignee)

Comment 8

11 years ago
Created attachment 232787 [details] [diff] [review]
Fix for 1.8.0

A version for 1.8.0 is much simpler compared with trunk/1.8.1 cases. On 1.8.0 branch the code to call the callback was not moved outside GC lock just to be removed completely. So this patch just erases the callback checking/calling in js_NewGCThing.
Attachment #232787 - Flags: approval1.8.0.7?
(Assignee)

Updated

11 years ago
Attachment #231895 - Flags: approval1.8.0.7?
(Assignee)

Comment 9

11 years ago
Note that on the trunk/1.8.1 branch the problem was much more serious due to close hooks that can be triggered to run from the branch callback invoked under js_NewGCThing.

In fact that was the reason for the rval changes in array_extra in the patch for bug 322135. There rval2 could be GC-ed through the branch callback when IndexToId  creates a string for a large index. Since the patch was touching the same code,  I tried to make rval2 rooted even without the changes for this bug committed. But I should reported it as a separated bug and not wait for orange tinderboxes with a mistake in the implementation hidden among other changes.
Comment on attachment 232787 [details] [diff] [review]
Fix for 1.8.0

approved for 1.8.0 branch, a=dveditz for drivers
Attachment #232787 - Flags: approval1.8.0.7? → approval1.8.0.7+
(Assignee)

Comment 11

11 years ago
I committed the patch from comment 8 to MOZILLA_1_8_0_BRANCH
Keywords: fixed1.8.0.7

Updated

11 years ago
Whiteboard: [need testcase]
Whiteboard: [need testcase] → [sg:critical][need testcase]

Comment 12

11 years ago
... so this is a 1.8 regression, right?
(Assignee)

Comment 13

11 years ago
(In reply to comment #12)
> ... so this is a 1.8 regression, right?

No. It is just that earlier branches require more "cooperation" with the engine to expose the bug, like, for example, periodically calling JS_GC() in the branch callback.

Comment 14

11 years ago
hmmm ... the code *after* applying this patch on 1.8.0 branch looks equal to the existing code in 1.0.x ... I don't see any branchCallback invocation there. What is the equivalent on 1.0.x? Any hints?
(Assignee)

Comment 15

11 years ago
(In reply to comment #14)
> hmmm ... the code *after* applying this patch on 1.8.0 branch looks equal to
> the existing code in 1.0.x ... I don't see any branchCallback invocation there.
> What is the equivalent on 1.0.x? Any hints?

Aha, that was indeed a regression that happens in >= 1.8*. I misread that 1.8 as  1.8.1. So no bug in Firefox 1.0.*.

(In reply to comment #15)
> Aha, that was indeed a regression that happens in >= 1.8*. I misread that 1.8
> as  1.8.1. So no bug in Firefox 1.0.*.

Or in Firefox 1.5.0.x (based on 1.8.0.x).

/be
Group: security
You need to log in before you can comment on or make changes to this bug.