test262 icon indicating copy to clipboard operation
test262 copied to clipboard

Compatibility with Hardened JavaScript

Open phoddie opened this issue 1 year ago • 6 comments

This PR proposes changes to existing test262 tests to allow them to pass under Hardened JavaScript (see Secure ECMAScript proposal and Hardened JavaScript). Moddable uses Hardened JavaScript for JavaScript runtimes on resource constrained embedded devices, including those targeted by ECMA-419.

The changes fall into four groups:

  1. Replace use of new Date() with new Date(1970). Scripts running inside a Compartment cannot retrieve the current time, so new Date() throws but new Date(1970) succeeds. Very few tests need the current time, but instead simply need a Date instance.
  2. Use Object.defineProperty instead of setting existing built-in properties directly, such as toString and toValue. In Hardened JavaScript, prototypes of built-in objects are frozen. Consequently, setting properties of an instance that exist on the prototype throw (Hardened JavaScript is always in strict mode).
  3. Eliminate use of Math.random(). Scripts running inside a Compartment cannot generate random numbers. One test identified so far uses Math.random() in a way that can easily be replaced with a counter.
  4. Narrow the scope of exception tests. Consider the following
assert.throws(TypeError, () => {
  var s1 = new Date();
  s1.toString = Boolean.prototype.toString;
  s1.toString();
});

This test passes, but only because new Date() fails by throwing a TypeError. If the invocation of the Date constructor is resolved by (1) above, then the assignment to toString fails as per (2) above. The script should be modified as below to ensure that assert.throws only tests the intended statement, s1.toString(). The modified script tests the intended functionality and passes under Hardened JavaScript

var s1 = new Date(1970);
Object.defineProperty(s1, "toString", {
  value: Boolean.prototype.toString
});
assert.throws(TypeError, () => {
  s1.toString();
});

This is an initial PR to begin the process of adapting test262 for use with Hardened JavaScript. Further changes are expected, with the vast majority likely to fall into the four groups described above.

Thank you to @gibson042, @kriskowal, and @erights for their advice on this work.

phoddie avatar May 18 '24 02:05 phoddie

Using new Date(1970) is a bit confusing, because it can easily be misinterpreted as creating a Date object for the year 1970, even though it's actually creating a Date object 1970 milliseconds from the epoch January 1, 1970. Can this be replaced with new Date(0)?

anba avatar May 18 '24 08:05 anba

var s1 = new Date(1970);
Object.defineProperty(s1, "toString", {
  value: Boolean.prototype.toString
});
assert.throws(TypeError, () => {
  s1.toString();
});

145913a3d9648075bb4e9b912a98735b8dcb99c2 also uses writable: true in all property descriptors. Is specifying writable: true needed for the tests or can be it be omitted?

anba avatar May 18 '24 08:05 anba

I believe it’s necessary because the things it’s shadowing are accessors in a locked-down realm.

ljharb avatar May 18 '24 13:05 ljharb

@anba, thanks for the review.

Using new Date(1970) is a bit confusing, because it can easily be misinterpreted as creating a Date object for the year 1970, even though it's actually creating a Date object 1970 milliseconds from the epoch January 1, 1970. Can this be replaced with new Date(0)?

I wanted an indication of the Date constructor changes made for Hardened JavaScript and chose 1970. Admittedly, it could be misleading. Is there another value that could be used for that purpose? If not, I can change them to 0.

https://github.com/tc39/test262/commit/145913a3d9648075bb4e9b912a98735b8dcb99c2 also uses writable: true in all property descriptors. Is specifying writable: true needed for the tests or can be it be omitted?

Setting writable: true is not necessary for the tests in that commit, since the property is not written after the call to defineProperty. I imagine some other tests might need it to be writable. Would you prefer to remove the writable: true in these?

phoddie avatar May 18 '24 19:05 phoddie

I wanted an indication of the Date constructor changes made for Hardened JavaScript and chose 1970. Admittedly, it could be misleading. Is there another value that could be used for that purpose? If not, I can change them to 0.

The exact value doesn't matter to me, it's just important for me that there's no confusion about the milliseconds value which is passed to Date.

Setting writable: true is not necessary for the tests in that commit, since the property is not written after the call to defineProperty. I imagine some other tests might need it to be writable. Would you prefer to remove the writable: true in these?

I'd prefer to either:

  • Omit writable: true and only use value, e.g. Object.defineProperty(obj, prop, {value: 123}).
  • Or use a fully populated property descriptor, e.g. Object.defineProperty(obj, prop, {value: 123, writable: true, enumerable: true, configurable: true}).

When only writable: true is used, it indicates (at least to me) that the property needs to support write-access, but doesn't support deletion.

anba avatar May 28 '24 14:05 anba

@anba – I've changed new Date(1970) to new Date(0) everywhere. That should invite the fewest questions moving forward.

For Object.defineProperty let's try your rule: either a minimal property descriptor with value alone or a fully populated property descriptor. For this PR, the minimal property descriptor is always enough. (I added four more files that use defineProperty in this update).

phoddie avatar May 31 '24 22:05 phoddie

This has a formal approval and a couple of additional approvals. Can we merge it or was anyone else planning to review it?

ptomato avatar Jul 03 '24 15:07 ptomato

This will need a rebase; also, which tests remain that do exercise new Date(), and Math.random()? If those still exist then I’m fine with this landing.

ljharb avatar Jul 03 '24 22:07 ljharb

This will need a rebase

Done.

also, which tests remain that do exercise new Date(), and Math.random()? If those still exist then I’m fine with this landing.

This PR does not modify or remove any explicit test of new Date or Math.random(). It eliminates their use in tests of other language features.

phoddie avatar Jul 04 '24 14:07 phoddie