Closed Bug 789812 Opened 10 years ago Closed 10 years ago

Freeze ParallelArray objects; rewrap .shape property


(Core :: JavaScript Engine, defect)

Not set



Tracking Status
firefox16 --- unaffected
firefox17 + fixed
firefox18 --- fixed
firefox-esr10 --- unaffected


(Reporter: shu, Assigned: shu)


(Keywords: regression, sec-critical, Whiteboard: [qa-][adv-track-main17-])


(1 file, 2 obsolete files)

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.
Assignee: general → shu
Attached patch fix and testcases (obsolete) — Splinter Review
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.
Attachment #659586 - Flags: review?(dmandelin)
Flags: in-testsuite+
Attached patch fix and testcase v2 (obsolete) — Splinter Review
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?
Attachment #659586 - Attachment is obsolete: true
Attachment #659586 - Flags: review?(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.
Attachment #659612 - Flags: review?(jwalden+bmo)
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.
Attachment #659612 - Flags: review?(jwalden+bmo) → review+
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.
Attachment #659612 - Attachment is obsolete: true
Attachment #663465 - Flags: review+
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
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:
Attachment #663465 - Flags: approval-mozilla-aurora?
Keywords: regression
Attachment #663465 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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 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.
I had landed this already; it slipped my mind to link the commit:

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.
Whiteboard: [qa-]
Whiteboard: [qa-] → [qa-][adv-track-main17-]
Group: core-security
You need to log in before you can comment on or make changes to this bug.