789812 - Freeze ParallelArray objects; rewrap .shape property

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Shu-yu Guo [:shu]]

ParallelArray objects should be frozen in the Object.isFrozen sense.

Further, the .shape property on objects is currently writable by users and needs to be rewrapped every access. This is a glaring security hole, marking s-s.

Comment 1

[Shu-yu Guo [:shu]]

Attachment: fix and testcases

Until Waldo implements something to deal with all properties uniformly between native/non-native objects, this code is a bit hacky to duplicate behavior of frozen objects.

Makes .length and .shape "own" properties instead of dangling them as getters off the prototype.

Copy the .shape array everytime it's accessed.

Comment 2

[Shu-yu Guo [:shu]]

Attachment: fix and testcase v2

Forgot to reuse the type of copied .shape array.

dmandelin, would you be willing to review this or this freeze semantics kind of stuff is better off for Waldo?

Comment 3

[David Mandelin [:dmandelin]]

(In reply to Shu-yu Guo [:shu] from comment #2)
> Created attachment 659612 [details] [diff] [review]
> fix and testcase v2
> 
> Forgot to reuse the type of copied .shape array.
> 
> dmandelin, would you be willing to review this or this freeze semantics kind
> of stuff is better off for Waldo?

Yes, Jeff would be a better reviewer.

Comment 4

[Jeff Walden [:Waldo]]

Comment on attachment 659612 [details] [diff] [review]
fix and testcase v2

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

I know nothing at all about ParallelArray semantics, so I'm making some assumptions about what's being implemented here, and how it fits in with the rest of the implementation, and about the semantics of the existing implementation.  It looks reasonable from a pure-ECMAScript-semantics angle, but if you want any double-checking of ParallelArray-centric semantics, you'll need to find someone else for it.

::: js/src/builtin/ParallelArray.cpp
@@ +214,5 @@
>  
> +static JSBool
> +EmptyDimensions(JSContext *cx, unsigned argc, Value *vp)
> +{
> +    JSObject *empty = NewDenseArrayWithType(cx, 1);

Use a root here, please.

@@ +1772,5 @@
>      }
>  
> +    if (name == cx->runtime->atomState.shapeAtom) {
> +        RootedObject dimArray(cx, as(obj)->dimensionArray());
> +        JSObject *copy = NewDenseCopiedArray(cx, dimArray->getDenseArrayInitializedLength(),

Root this too.

@@ +1812,5 @@
> +        return true;
> +    }
> +
> +    vp.setUndefined();
> +    return true;

*present = false; here.  I'm not sure if it's part of the contract that *present has unspecified value initially, but really we should just assume that -- there's no reason to tempt fate on this, especially since the property/elements change should make this hook go away.

@@ +1958,5 @@
> +    if (IdIsLengthOrShapeAtom(cx, id) || IdIsInBoundsIndex(cx, obj, id)) {
> +        if (strict)
> +            return obj->reportNotConfigurable(cx, id);
> +        if (cx->hasStrictOption())
> +            return obj->reportNotConfigurable(cx, id, JSREPORT_STRICT | JSREPORT_WARNING);

This doesn't look right.  If strict you correctly throw (although I dislike the implication of throwing-ness, optional arguments controlling behavior so significantly here, sigh).  If there's only a strict option, tho, you shouldn't be stopping execution (at least not if the werror option is unset).  Check for !reportNotConfigurable and return false in that case, but if it doesn't fail, you should fall through here.

@@ +2000,5 @@
>                                 AutoIdVector *props)
>  {
>      RootedParallelArrayObject source(cx, as(obj));
>  
> +    if (flags & JSITER_HIDDEN &&

Throw some parentheses around this bitwise-and, please.

Comment 5

[Shu-yu Guo [:shu]]

Attachment: fix and testcase v3

Reverted decision to make .shape and .length magical [own] properties.

Carrying the r+ as frozen semantics are not changed, and unfortunately I myself am the only person who can review ParallelArray semantics changes.

Comment 6

[Shu-yu Guo [:shu]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3b92cb5e3d65

https://tbpl.mozilla.org/?tree=Try&rev=c71268892db5

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3b92cb5e3d65

Comment 8

[Shu-yu Guo [:shu]]

Comment on attachment 663465 [details] [diff] [review]
fix and testcase v3

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 778559
User impact if declined: Crash/read of uninitialized memory
Testing completed (on m-c, etc.): locally and on m-c
Risk to taking this patch (and alternatives if risky): None, localized to a non-publicized and unused-in-the-wild API
String or UUID changes made by this patch:

Comment 9

[Justin Wood (:Callek)]

despite lack of sec-approval+ since this landed on m-c dveditz agreed there was no need to await a landing -- however:

$ hg transplant -s http://hg.mozilla.org/mozilla-central 3b92cb5e3d65
searching for changes
applying 3b92cb5e3d65
patching file js/src/builtin/ParallelArray.cpp
Hunk #1 succeeded at 227 with fuzz 1 (offset 0 lines).
Hunk #2 FAILED at 826
Hunk #3 succeeded at 1054 with fuzz 2 (offset 11 lines).
Hunk #4 FAILED at 1445
Hunk #5 FAILED at 1522
Hunk #6 FAILED at 1604
Hunk #7 FAILED at 1652
Hunk #8 FAILED at 1664
Hunk #9 FAILED at 1673
Hunk #10 FAILED at 1682
Hunk #11 FAILED at 1711
Hunk #13 FAILED at 1767
Hunk #14 FAILED at 1811
Hunk #15 FAILED at 1836
Hunk #16 FAILED at 1902
13 out of 16 hunks FAILED -- saving rejects to file js/src/builtin/ParallelArray.cpp.rej
patching file js/src/builtin/ParallelArray.h
Hunk #1 FAILED at 378
1 out of 1 hunks FAILED -- saving rejects to file js/src/builtin/ParallelArray.h.rej
patching file js/src/jit-test/tests/parallelarray/length-3.js
Hunk #1 FAILED at 0
1 out of 1 hunks FAILED -- saving rejects to file js/src/jit-test/tests/parallelarray/length-3.js.rej
patching file js/src/jit-test/tests/parallelarray/shape-2.js
Hunk #1 FAILED at 6
1 out of 1 hunks FAILED -- saving rejects to file js/src/jit-test/tests/parallelarray/shape-2.js.rej
file js/src/jit-test/tests/parallelarray/shape-4.js already exists
1 out of 1 hunks FAILED -- saving rejects to file js/src/jit-test/tests/parallelarray/shape-4.js.rej
file js/src/jit-test/tests/parallelarray/surfaces-3.js already exists
1 out of 1 hunks FAILED -- saving rejects to file js/src/jit-test/tests/parallelarray/surfaces-3.js.rej
patching file js/src/js.msg
Hunk #1 FAILED at 353
1 out of 1 hunks FAILED -- saving rejects to file js/src/js.msg.rej
patch failed to apply
abort: Fix up the merge and run hg transplant --continue

I'm not about to try untangling the .rej's right now.

Comment 10

[Shu-yu Guo [:shu]]

I had landed this already; it slipped my mind to link the commit:

https://hg.mozilla.org/releases/mozilla-aurora/rev/106950

But was I supposed to get sec-approval+? I've been only asking for a? and landing patches for s-s ParallelArray patches that needed to go to aurora.