Crash [@ js::GetObjectClass] or [@ js::ComputeImplicitThis]

VERIFIED FIXED in Firefox 17

Status

()

Core
JavaScript Engine
--
critical
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: gkw, Assigned: luke)

Tracking

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

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

Firefox Tracking Flags

(firefox15 unaffected, firefox16 unaffected, firefox17+ verified, firefox18+ verified, firefox-esr10 unaffected)

Details

(Whiteboard: [jsbugmon:update], crash signature)

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

5 years ago
Created attachment 666187 [details]
stack

Function("\
    __defineGetter__(\"x\", function() {\
        delete this[\"x\"];\
    });\
    x()\
")()

crashes js debug and opt shell on m-c changeset c09a0c022b2e without any CLI arguments at js::GetObjectClass

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   102943:57c1c330e85f
user:        Luke Wagner
date:        Fri Aug 17 18:09:43 2012 -0700
summary:     Bug 774915 - don't use the property cache for dynamic name lookup (r=bhackett)

Inspection of the registers shows to likely be a null deref (%rax and %rcx in %pc are null or near-null), but setting s-s just to be safe.
(Reporter)

Comment 1

5 years ago
Similar testcases crash at js::ComputeImplicitThis.
Crash Signature: [@ js::GetObjectClass] → [@ js::GetObjectClass] [@ js::ComputeImplicitThis]
Summary: Crash [@ js::GetObjectClass] → Crash [@ js::GetObjectClass] or [@ js::ComputeImplicitThis]
Crash Signature: [@ js::GetObjectClass] [@ js::ComputeImplicitThis] → [@ js::GetObjectClass] [@ js::ComputeImplicitThis]
(Assignee)

Comment 2

5 years ago
Ahh, IMPLICITTHIS can, as this example demonstrates, miss, which returns a null 'scope' hence the (safe) NULL deref.  IMPLICITTHIS should use the 'global'-defaulting LookupNameForSet variant (which now needs to be renamed).
Group: core-security
Crash Signature: [@ js::GetObjectClass] [@ js::ComputeImplicitThis] → [@ js::GetObjectClass] [@ js::ComputeImplicitThis]
(Assignee)

Comment 3

5 years ago
Created attachment 666741 [details] [diff] [review]
fix and test

Simple fix: most of the patch is renaming LookupNameForSet, only real change is JSOP_IMPLICITTHIS.
Assignee: general → luke
Status: NEW → ASSIGNED
Attachment #666741 - Flags: review?(jwalden+bmo)

Comment 4

5 years ago
We don't track all regressions for a specific release. This isn't a high volume crash or a security issue, so what's the user impact here?
(Assignee)

Comment 5

5 years ago
Created attachment 666754 [details] [diff] [review]
fix and test

Oops, need the symmetric fix in the JM stub call.  IM doesn't handle IMPLICITTHIS.
Attachment #666741 - Attachment is obsolete: true
Attachment #666741 - Flags: review?(jwalden+bmo)
Attachment #666754 - Flags: review?(jwalden+bmo)
(Assignee)

Comment 6

5 years ago
(In reply to Alex Keybl [:akeybl] from comment #4)
> We don't track all regressions for a specific release. This isn't a high
> volume crash or a security issue, so what's the user impact here?

If a site triggers the condition (which seems relatively rare, but the web is vast), we'll get a null deref.  It's hard to tell what the actual crash volume is; ComputeImplicitThis is an inline function which means that crashes will likely show up as js::Interpret in release builds.
Crash Signature: [@ js::GetObjectClass] [@ js::ComputeImplicitThis] → [@ js::GetObjectClass] [@ js::ComputeImplicitThis]
Comment on attachment 666754 [details] [diff] [review]
fix and test

Review of attachment 666754 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jit-test/tests/basic/testImplicitThisMiss.js
@@ +1,3 @@
> +// |jit-test| error:TypeError
> +var f = Function("\
> +    __defineGetter__(\"x\", function() {\

Object.defineProperty(this, "x", { configurable: true, get: function() { ... } });

@@ +1,4 @@
> +// |jit-test| error:TypeError
> +var f = Function("\
> +    __defineGetter__(\"x\", function() {\
> +        delete this[\"x\"];\

Use ' for readability in these lines?  Or maybe convert this whole thing into a function expression, not a runtime-compiled function?  Would read a whole lot better that way...
Attachment #666754 - Flags: review?(jwalden+bmo) → review+
(Assignee)

Comment 8

5 years ago
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #7)
> Or maybe convert this whole thing into a function expression, 

Function is necessary to generate JSOP_IMPLICITTHIS.
Crash Signature: [@ js::GetObjectClass] [@ js::ComputeImplicitThis] → [@ js::GetObjectClass] [@ js::ComputeImplicitThis]
(Assignee)

Comment 9

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/8b5a786ee47b
Crash Signature: [@ js::GetObjectClass] [@ js::ComputeImplicitThis] → [@ js::GetObjectClass] [@ js::ComputeImplicitThis]
https://hg.mozilla.org/mozilla-central/rev/8b5a786ee47b
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla18

Updated

5 years ago
Crash Signature: [@ js::GetObjectClass] [@ js::ComputeImplicitThis] → [@ js::GetObjectClass] [@ js::ComputeImplicitThis]
status-firefox18: affected → fixed
Tracking for FF17, but if this doesn't get fixed while 17 is on Aurora and has a non-zero risk fix, we'll likely untrack due to lack of data.
tracking-firefox17: ? → +
tracking-firefox18: ? → +
(Assignee)

Comment 12

5 years ago
Comment on attachment 666754 [details] [diff] [review]
fix and test

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 774915
User impact if declined: null deref crash
Testing completed (on m-c, etc.): m-c
Risk to taking this patch (and alternatives if risky): seems pretty low, just restoring accidentally-changed previous behavior in a rare op
Attachment #666754 - Flags: approval-mozilla-aurora?
(Reporter)

Comment 13

5 years ago
Release drivers, ping re: approval-mozilla-aurora?
Comment on attachment 666754 [details] [diff] [review]
fix and test

 Approving for aurora . Please land before monday oct 8th merge.
Attachment #666754 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(Assignee)

Comment 15

5 years ago
https://hg.mozilla.org/releases/mozilla-aurora/rev/28ef082de058
status-firefox17: affected → fixed
Keywords: verifyme
No new crashes appear in Socorro in the last month.
status-firefox17: fixed → verified
In the last month, no new crashes appear in Socorro.
status-firefox18: fixed → verified

Updated

5 years ago
Status: RESOLVED → VERIFIED
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.