Closed Bug 988416 Opened 6 years ago Closed 6 years ago

std_Object_defineProperty is mostly not usable in self-hosted code

Categories

(Core :: JavaScript: Internationalization API, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla31

People

(Reporter: Waldo, Assigned: till)

Details

Attachments

(3 files, 1 obsolete file)

The issue is that Object.defineProperty(obj, propname, desc) does validation on desc.  Specifically, if desc is this:

  var desc = { value: 17, writable: false, configurable: true, enumerable: true };

but script has done

  Object.prototype.get = 42;

then Object.defineProperty validation of the descriptor will fail, because the descriptor has bits to appear to be both a data descriptor and an accessor descriptor.

One workaround is to create the descriptor using std_Object_create(null).  But really an intrinsic for adding/creating data properties is more desirable.  So we should probably do that instead, here, as I don't think there's super-huge rush to immediately immediately fix the correctness issue here.
Test:

  Object.prototype.get = 5;
  try
  {
    new Intl.Collator().resolvedOptions();
    print("PASS");
  }
  catch (e)
  {
    print("FAIL");
  }
As discussed. I moved some of the #defines from Utilities.js, the next patch will contain those for property descriptor attributes.
Attachment #8398764 - Flags: review?(jwalden+bmo)
Assignee: nobody → till
Status: NEW → ASSIGNED
Introduces _DefineValueProperty, removes std_Object_defineProperty and replaces all uses of the latter with the former. Plus test from comment 1.
Attachment #8398765 - Flags: review?(jwalden+bmo)
Comment on attachment 8398764 [details] [diff] [review]
Part 1: Extract self-hosting #defines into builtin/SelfHostingDefines.h

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

::: js/src/builtin/SelfHostingDefines.h
@@ +17,5 @@
> +/* Assertions */
> +#ifdef DEBUG
> +#define assert(b, info) if (!(b)) AssertionFailed(info)
> +#else
> +#define assert(b, info)

Conceivably /* elided assertion */ might be nice here

@@ +20,5 @@
> +#else
> +#define assert(b, info)
> +#endif
> +
> +/* Safe versions of ARRAY.push(ELEMENT) */

If these existed beforehand, okay.  But given that the semantics of Array.prototype.push are -- according to the spec, if not to our implementation, as I recall -- supposed to invoke prototypal setters, I'm not sure this is the right primitive to expose, nor the right description of behavior to use.  Followup bug.
Attachment #8398764 - Flags: review?(jwalden+bmo) → review+
Comment on attachment 8398765 [details] [diff] [review]
Part 2: Add safe _DefineValueProperty self-hosting intrinsic.

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

::: js/src/builtin/SelfHostingDefines.h
@@ +29,5 @@
>  
> +// Property descriptor attributes.
> +#define ATTR_ENUMERABLE     1
> +#define ATTR_CONFIGURABLE   2
> +#define ATTR_WRITABLE       4

0x1, 0x2, 0x4 might make clearer this is a bit field.

I would rather see the exact flags passed in at each site, specified explicitly, without implied defaults.  That is, I'd rather see (ENUMERABLE | CONFIGURABLE | NONWRITABLE) or so at each location.  I'm not sure about the best interface for this -- just adding NON* versions set to 0x0 for this would work, but there's no code enforcement of the policy, so I suspect it'd fall by the side over time.  Having an ATTRS(enum, config, writ) helper that combines the flags together and requires passing all of them is prone to argument mis-ordering.  You have any ideas here, that are better than adding NON* defines and doing our best to enforce use of them at review time?

::: js/src/jit-test/tests/self-hosting/define-value-property.js
@@ +4,5 @@
> +    new Intl.Collator().resolvedOptions();
> +  }
> +  catch (e)
> +  {
> +    assertTrue(false, "Mustn't be reached");

Might as well simplify to just:

Object.prototype.get = 5;
new Intl.Collator().resolvedOptions();

The try-catch was just to make for nicer success/failure observations in the bug.  But a thrown exception here is plenty good for indicating there's a bug.

::: js/src/jsobj.cpp
@@ +943,5 @@
>      if (!desc || !desc->initialize(cx, descriptor))
>          return false;
>  
> +    printf("attr: %d\n", desc->attributes());
> +

Doubtful this is wanted any more.  :-)

::: js/src/vm/SelfHosting.cpp
@@ +445,5 @@
> +
> +    if (args.length() != 4) {
> +        JS_ReportError(cx, "Incorrect number of arguments, exactly 4 required");
> +        return false;
> +    }

MOZ_ASSERT this, tests will exercise and ensure this.

@@ +453,5 @@
> +
> +    RootedObject obj(cx, &args[0].toObject());
> +    if (obj->is<ProxyObject>()) {
> +        JS_ReportError(cx, "_DefineValueProperty can't be used on proxies");
> +        return false;

Technically this might be enforceable at code-writing time, but it does seem slightly more prone to mistake, so an extra check seems fair enough.
Attachment #8398765 - Flags: review?(jwalden+bmo) → review+
Asking for re-review, because the patch changed substantially.

I introduced NON* versions for all defineProperty attributes and gave them individual flags. _DefineValueProperty asserts that, for all attributes, either the YES-version xor the NON*-version is set.

I also null'd Object.defineProperty at the beginning of Utilities.js so that it is extremely unlikely that someone accidentally re-introduces std_Object_defineProperty.

If you think this setup is good, I'll apply it to defineProperties and Object.create, next.

Try-servering here: https://tbpl.mozilla.org/?tree=Try&rev=476d6d483229
Attachment #8402194 - Flags: review?(jwalden+bmo)
Attachment #8398765 - Attachment is obsolete: true
Comment on attachment 8402194 [details] [diff] [review]
Part 2: Add safe _DefineValueProperty self-hosting intrinsic. v2

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

Yeah, this seems fine/better.

::: js/src/builtin/Utilities.js
@@ +24,5 @@
>  */
>  
>  #include "SelfHostingDefines.h"
>  
> +// Remove built-in functions that're unsafe to use.

"Remove unsafe builtin functions" is slightly more concise, avoids a slightly awkward/somewhat-ungrammatical contraction.

::: js/src/vm/SelfHosting.cpp
@@ +462,5 @@
> +
> +    unsigned resolvedAttributes = JSPROP_PERMANENT | JSPROP_READONLY;
> +
> +    MOZ_ASSERT((attributes & ATTR_ENUMERABLE && !(attributes & ATTR_NONENUMERABLE)) ||
> +               attributes & ATTR_NONENUMERABLE,

bool(attributes & ATTR_ENUMERABLE) != bool(attributes & ATTR_NONENUMERABLE) seems slightly more readable.  (Or ! instead of bool if you want to be concise, but I think I like bool better.)

@@ +468,5 @@
> +    if (attributes & ATTR_ENUMERABLE)
> +        resolvedAttributes |= JSPROP_ENUMERATE;
> +
> +    MOZ_ASSERT((attributes & ATTR_CONFIGURABLE && !(attributes & ATTR_NONCONFIGURABLE)) ||
> +               attributes & ATTR_NONCONFIGURABLE,

Same form here.

@@ +475,5 @@
> +    if (attributes & ATTR_CONFIGURABLE)
> +        resolvedAttributes &= ~JSPROP_PERMANENT;
> +
> +    MOZ_ASSERT((attributes & ATTR_WRITABLE && !(attributes & ATTR_NONWRITABLE)) ||
> +               attributes & ATTR_NONWRITABLE,

Same form here.
Attachment #8402194 - Flags: review?(jwalden+bmo) → review+
https://hg.mozilla.org/mozilla-central/rev/35134eeaf242
https://hg.mozilla.org/mozilla-central/rev/fb38cd386f8c
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
Comment on attachment 8406190 [details] [diff] [review]
Test for the Intl property

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

r=me
Attachment #8406190 - Flags: review?(till) → review+
You need to log in before you can comment on or make changes to this bug.