Closed Bug 911707 Opened 6 years ago Closed 6 years ago

Assertion failure: arr->lengthIsWritable() (setter shouldn't be called if property is non-writable), at jsarray.cpp

Categories

(Core :: JavaScript Engine, defect, critical)

x86
macOS
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla26
Tracking Status
firefox25 --- unaffected
firefox26 --- wontfix
firefox27 --- fixed
firefox-esr17 --- unaffected
firefox-esr24 --- unaffected
b2g18 --- unaffected

People

(Reporter: gkw, Assigned: efaust)

References

(Blocks 1 open bug)

Details

(4 keywords, Whiteboard: [jsbugmon:update,testComment=3,origRev=1179318fb5aa][adv-main27+])

Attachments

(4 files)

Attached file stack
try {
    x = [];
    y = [];
    f = (function() {
        y.length = 6
    });
    Object.freeze(y);
    f();
    Object.prototype.watch.call(x, "length", (function() {
        return function() {
            f()
        }
    })());
    Array.prototype.pop.call(x)
} catch (e) {}
Array.prototype.push.call(x, "")

asserts js debug threadsafe shell on m-c changeset 1179318fb5aa with --ion-eager --ion-inlining=off at Assertion failure: arr->lengthIsWritable() (setter shouldn't be called if property is non-writable), at jsarray.cpp

My configure flags are:
LD=ld CROSS_COMPILE=1 CXX="clang++ -Qunused-arguments -arch i386" RANLIB=ranlib CC="clang -Qunused-arguments -arch i386" AS=$CC AR=ar STRIP="strip -x -S" HOST_CC="clang -Qunused-arguments" HOST_CXX="clang++ -Qunused-arguments" sh ./configure --target=i386-apple-darwin9.2.0 --enable-macos-target=10.5 --enable-optimize --enable-debug --enable-profiling --enable-gczeal --enable-debug-symbols --enable-methodjit --enable-type-inference --disable-tests --enable-threadsafe --with-ccache <some NSPR compile flags, or use --with-system-nspr>

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   http://hg.mozilla.org/mozilla-central/rev/c1ccfd8f31bf
user:        Eric Faust
date:        Fri Aug 30 18:50:36 2013 -0700
summary:     Bug 824393 - Part 0: Open SetPropertyIC to cases with uncertain TI. (r=bhackett)

(s-s because the stack is short and strange) Eric, is bug 824393 probably related?
Flags: needinfo?(efaustbmo)
Whiteboard: [jsbugmon:update] → [jsbugmon:]
JSBugMon: Cannot process bug: Unable to automatically reproduce, please track manually.
Easier test that should also reproduce better (m-c rev 1179318fb5aa, options --fuzzing-safe --ion-eager):


x = [ "CNY", "TWD", "invalid" ];
Object.freeze(x).map(function() {
    x.length = 6
})
Whiteboard: [jsbugmon:] → [jsbugmon:update,bisect,testComment=3,origRev=1179318fb5aa]
Whiteboard: [jsbugmon:update,bisect,testComment=3,origRev=1179318fb5aa] → [jsbugmon:update,testComment=3,origRev=1179318fb5aa]
JSBugMon: Bisection requested, result:
autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   http://hg.mozilla.org/mozilla-central/rev/c1ccfd8f31bf
user:        Eric Faust
date:        Fri Aug 30 18:50:36 2013 -0700
summary:     Bug 824393 - Part 0: Open SetPropertyIC to cases with uncertain TI. (r=bhackett)

This iteration took 330.328 seconds to run.
Attached patch FixSplinter Review
Yeah, OK. Don't IC unwritable PropertyOp setter shapes. Even though it should be relevant, we mirror the state that is in that flag, so it's the easiest way to check.
Assignee: general → efaustbmo
Status: NEW → ASSIGNED
Attachment #799015 - Flags: review?(jwalden+bmo)
Flags: needinfo?(efaustbmo)
Comment on attachment 799015 [details] [diff] [review]
Fix

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

::: js/src/jit/IonCaches.cpp
@@ +2054,5 @@
>  
>      if (shape->hasSetterValue())
>          return false;
>  
> +    // Despite the vehement claims of Shape.h that writable() is only

I don't actually see these claims, only a commented-out assertion in Shape::writable() that belies them.  :-)
Attachment #799015 - Flags: review?(jwalden+bmo) → review+
How bad is this, security-wise, Eric?
Best to play it safe here, I think. Jeff and I agree that while the actual assertion that is failing is not a big deal, there could be other PropertyOp setters that are not as carefully written. I don't know what that translates to in terms of flags? moderate is my best guess.

Not hugely bad on the path taken, but with other potential consequences. Suggest we keep it closed until this lands.
Keywords: sec-moderate
Whiteboard: [jsbugmon:update,testComment=3,origRev=1179318fb5aa] → [jsbugmon:update,testComment=3,origRev=1179318fb5aa,ignore]
JSBugMon: The testcase found in this bug no longer reproduces (tried revision 680c89c76100).
This landed on m-c:

http://hg.mozilla.org/mozilla-central/rev/dcd4c1b60d7f
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [jsbugmon:update,testComment=3,origRev=1179318fb5aa,ignore] → [jsbugmon:update,testComment=3,origRev=1179318fb5aa]
Target Milestone: --- → mozilla26
Status: RESOLVED → VERIFIED
JSBugMon: This bug has been automatically verified fixed.
Whiteboard: [jsbugmon:update,testComment=3,origRev=1179318fb5aa] → [jsbugmon:update,testComment=3,origRev=1179318fb5aa][adv-main27+]
Group: core-security
You need to log in before you can comment on or make changes to this bug.