js_InvokeConstructor with clampReturn==JS_TRUE and user-controlled ctor is a gc hazard

RESOLVED FIXED

Status

()

Core
JavaScript Engine
--
critical
RESOLVED FIXED
7 years ago
4 years ago

People

(Reporter: Waldo, Assigned: Waldo)

Tracking

(Blocks: 1 bug, {assertion, regression, testcase})

Trunk
assertion, regression, testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(blocking2.0 final+, status1.9.2 .9-fixed, status1.9.1 .12-fixed)

Details

(Whiteboard: [sg:critical?][critsmash:investigating])

Attachments

(2 attachments)

See bug 567577 comment 9 and bug 567577 comment 10.

I *think* we either need to add a tvr for the noted default-return object in js_InvokeConstructor or figure out whether __noSuchMethod__ functionality can ever be invoked in a constructing situation.  The latter is probably hard, and the former is probably easy, which suggests probably the former is what we should do.

This also shows up in JS_New on trunk (but conservative stack scanning saves us) and in fun_applyConstructor (Narcissus-only, so I think we don't care), but neither of these cases needs a change.
Cc'ing mrbkap who was involved in bug 453462, and gal who took over at some point.

/be

Updated

7 years ago
Whiteboard: [sg:critical?]

Comment 2

7 years ago
this is still biting on branches, even if we get a conservative scanner on trunk, right?
Yes.
The patch in bug 567577 fixes this, I think.

Comment 5

7 years ago
(In reply to comment #4)
> The patch in bug 567577 fixes this, I think.

"Can we get a little more confidence than 'I Think'?" -- Damon Sicore

Comment 6

7 years ago
Waldo filed it. Waldo should decide. As far as I understand the bug report scanning the stack will find this root, so this is safe now. Waldo?
Comment 4 is correct with respect to trunk, even with conservative stack scanning turned off.  We may not want to apply the patch there to branches exactly as is, however, as it does force clamping, which would be a web-visible change.  Just adding the AutoObjectRooter (JSAutoTempValueRooter on branches) should be enough for branches, without compatibility concerns.

Comment 8

7 years ago
Waldo, could you please get a branch patch going?
Assignee: general → jwalden+bmo
Sure.
Status: NEW → ASSIGNED

Updated

7 years ago
Whiteboard: [sg:critical?] → [sg:critical?][critsmash:investigating]

Updated

7 years ago
status1.9.1: --- → ?
status1.9.2: --- → ?
Any news on the branch patch?
Created attachment 460709 [details] [diff] [review]
Branch patch
Attachment #460709 - Flags: review?(dvander)
Attachment #460709 - Flags: review?(dvander) → review+
Attachment #460709 - Flags: approval1.9.2.9?
Attachment #460709 - Flags: approval1.9.1.12?

Comment 12

7 years ago
Comment on attachment 460709 [details] [diff] [review]
Branch patch

a=LegNeato for 1.9.2.9 and 1.9.1.12. Please land on mozilla-1.9.2 default and mozilla-1.9.1 default.
Attachment #460709 - Flags: approval1.9.2.9?
Attachment #460709 - Flags: approval1.9.2.9+
Attachment #460709 - Flags: approval1.9.1.12?
Attachment #460709 - Flags: approval1.9.1.12+
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/c54b6c62214d
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/95e8deb0774b
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
status1.9.1: ? → .12-fixed
status1.9.2: ? → .9-fixed
Resolution: --- → FIXED
Created attachment 471885 [details] [diff] [review]
Patch for 1.9.0
Attachment #471885 - Flags: review?
Attachment #471885 - Flags: review? → review?(dvander)
Attachment #471885 - Flags: review?(dvander) → review+
Group: core-security
https://hg.mozilla.org/mozilla-central/rev/edd1edc19f46
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.