ES5/3 compatibility fixes for native accessor properties and Error inheritance. (issue 247900043 by kpreid@google.com)

[re...@codereview-hr.appspotmail.com]

Reviewers: MarkM, metaweta,

Description:
* For Chrome 44 <https://github.com/google/caja/issues/1967>, avoid
doing "O[P] = O[P]" in the case where it would be a noop by ES3
rules, which seems to be sufficient for the issue.
* Found while testing: markFunc did nothing if the function was
already marked. Since the Error subclass constructors now inherit
from the Error constructor per ES6 (and in Chrome), this would cause
the subclasses to not get new___ configured, so 'new EvalError(...)'
would act as 'new Error(...)'. markFunc now looks for an own property.

Please review this at https://codereview.appspot.com/247900043/

Affected files (+12, -4 lines):
M src/com/google/caja/es53.js


Index: src/com/google/caja/es53.js
diff --git a/src/com/google/caja/es53.js b/src/com/google/caja/es53.js
index
5765d0885a723bd11e8f849a117ab4fd92229b8a..1f5e009e7814b15e4c8bb3de03595220d4d6a8b7
100644
--- a/src/com/google/caja/es53.js
+++ b/src/com/google/caja/es53.js
@@ -718,7 +718,7 @@ var ___, cajaVM, safeJSON, WeakMap, ArrayLike, Proxy;
* whitelisted properties of {@code this}).
*/
function markFunc(fn, name) {
- if (fn.ok___) { return fn; }
+ if (fn.ok___ && fn.hasOwnProperty('ok___')) { return fn; }
if (!isFunction(fn)) {
notFunction(fn);
}
@@ -2464,7 +2464,12 @@ var ___, cajaVM, safeJSON, WeakMap, ArrayLike, Proxy;
// Desc. If the value of an attribute field of Desc is
// absent, the attribute of the newly created property is
// set to its default value.
- O[P] = Desc.configurable ? void 0 : O[P];
+
+ if (Desc.configurable) {
+ O[P] = void 0;
+ } else if (!O.hasOwnProperty(P)) {
+ O[P] = O[P];
+ }
O[P + '_v___'] = false;
O[P + '_w___'] = O[P + '_gw___'] = false;
O[P + '_e___'] = Desc.enumerable ? O : false;
@@ -2624,8 +2629,11 @@ var ___, cajaVM, safeJSON, WeakMap, ArrayLike, Proxy;
O[P + '_gw___'] = Desc.writable ? O : false;
} else {
// Create the property if it's not there so that JSON.stringify
- // can see the property.
- O[P] = O[P];
+ // can see the property. But don't do this unless necessary in
case of
+ // native ES5 accessors.
+ if (!O.hasOwnProperty(P)) {
+ O[P] = O[P];
+ }
O[P + '_v___'] = false;
O[P + '_gw___'] = false;
O[P + '_g___'] = Desc.get;

[Kevin Reid]

Mike Stay, I'm asking you to take a look at this because it's touching ES5/3 and I don't fully understand the code as it exists. In particular, is the change to markFunc safe?

The ok___ test was introduced in r4822 <https://github.com/google/caja/commit/8cae568538b1b6a5eaca5ee886eb2a250a5fe606> but the description and review don't say what exactly that part of the change is intended to protect.

Is anything going to go wrong from now allowing markFunc on a function inheriting an already marked function?

[eri...@gmail.com]

https://codereview.appspot.com/247900043/diff/1/src/com/google/caja/es53.js
File src/com/google/caja/es53.js (right):

https://codereview.appspot.com/247900043/diff/1/src/com/google/caja/es53.js#newcode721
src/com/google/caja/es53.js:721: if (fn.ok___ &&

fn.hasOwnProperty('ok___')) { return fn; }

I no longer remember specifically how ok___ was set up. But if it is
like a lot of ES5/3's other markings, then its value is already the
object that it is own on, precisely to make such own property checks
cheap. So if we do want this to be an own property check, and if ok___
does work this way, then was can do simply

if (fn.ok___ === fn) {

As for whether we want this to be an own property check, I also am
curious what Mike Stay has to say. Mike?

https://codereview.appspot.com/247900043/diff/1/src/com/google/caja/es53.js#newcode2633
src/com/google/caja/es53.js:2633: // native ES5 accessors.
Have we actually encountered this as an issue? How? If ES5/3 co-exists
with native accessors, I suspect we have other problems as well. It was
written assuming not.

https://codereview.appspot.com/247900043/

[kpr...@google.com]

https://codereview.appspot.com/247900043/diff/1/src/com/google/caja/es53.js
File src/com/google/caja/es53.js (right):

https://codereview.appspot.com/247900043/diff/1/src/com/google/caja/es53.js#newcode721
src/com/google/caja/es53.js:721: if (fn.ok___ &&
fn.hasOwnProperty('ok___')) { return fn; }

On 2015/06/09 22:40:31, MarkM wrote:
> I no longer remember specifically how ok___ was set up. But if it is
like a lot
> of ES5/3's other markings, then its value is already the object that
it is own
> on, precisely to make such own property checks cheap.

It isn't; it's a boolean. If you look at the review of the original
addition of this line, <https://codereview.appspot.com/5873043>, we
expected that markFunc would never be called with inheritance like this.

But perhaps as you point out we _should_ make it into (fn.ok___ === fn)
in these cases. I hope Mike can help clarify the issues here.

https://codereview.appspot.com/247900043/diff/1/src/com/google/caja/es53.js#newcode2633
src/com/google/caja/es53.js:2633: // native ES5 accessors.

On 2015/06/09 22:40:31, MarkM wrote:
> Have we actually encountered this as an issue? How?

Yes, that's what https://github.com/google/caja/issues/1967 is. Chrome
44 has Function.prototype.arguments and Function.prototype.caller
accessors which throw.

I agree that there might be other problems lurking, but we'll have to
fix them as we see them, unless you're volunteering to do the complete
analysis. Hopefully we can kill ES5/3 for real before another one pops
up.

https://codereview.appspot.com/247900043/

[eri...@google.com]

https://codereview.appspot.com/247900043/diff/1/src/com/google/caja/es53.js
File src/com/google/caja/es53.js (right):

https://codereview.appspot.com/247900043/diff/1/src/com/google/caja/es53.js#newcode2633
src/com/google/caja/es53.js:2633: // native ES5 accessors.

On 2015/06/09 22:54:57, kpreid_google wrote:
> On 2015/06/09 22:40:31, MarkM wrote:
> > Have we actually encountered this as an issue? How?

> Yes, that's what https://github.com/google/caja/issues/1967 is. Chrome
44 has
> Function.prototype.arguments and Function.prototype.caller accessors
which
> throw.

But we don't whitelist these, do we? Can this be an issue for properties
we don't whitelist?

https://codereview.appspot.com/247900043/

[meta...@gmail.com]

https://codereview.appspot.com/247900043/diff/1/src/com/google/caja/es53.js
File src/com/google/caja/es53.js (right):

https://codereview.appspot.com/247900043/diff/1/src/com/google/caja/es53.js#newcode721
src/com/google/caja/es53.js:721: if (fn.ok___ &&
fn.hasOwnProperty('ok___')) { return fn; }

fn.ok___ is supposed to mark a function as being non-toxic. It depends
on an invariant that all toxic functions inherit directly from
Function.prototype. If it's possible that f.prototype is not
Function.prototype for some toxic function, we should switch to setting
fn.ok___ = fn on line 731 and elsewhere, and test for that here.

https://codereview.appspot.com/247900043/diff/1/src/com/google/caja/es53.js#newcode2633
src/com/google/caja/es53.js:2633: // native ES5 accessors.

FF has had setters forever, so this is potentially a problem if objects
with setters are being tamed. The code below looks right to me.

https://codereview.appspot.com/247900043/

[kpr...@google.com]

https://codereview.appspot.com/247900043/diff/1/src/com/google/caja/es53.js
File src/com/google/caja/es53.js (right):

https://codereview.appspot.com/247900043/diff/1/src/com/google/caja/es53.js#newcode721
src/com/google/caja/es53.js:721: if (fn.ok___ &&
fn.hasOwnProperty('ok___')) { return fn; }

On 2015/06/09 23:18:18, metaweta wrote:
> …, we should switch to setting fn.ok___ = fn on line 731 and

elsewhere,
> and test for that here.

Done.

https://codereview.appspot.com/247900043/diff/1/src/com/google/caja/es53.js#newcode2633
src/com/google/caja/es53.js:2633: // native ES5 accessors.

On 2015/06/09 22:59:22, Mark S. Miller wrote:
> On 2015/06/09 22:54:57, kpreid_google wrote:
> > Yes, that's what https://github.com/google/caja/issues/1967 is.
Chrome 44 has
> > Function.prototype.arguments and Function.prototype.caller accessors
which
> > throw.

> But we don't whitelist these, do we? Can this be an issue for
properties we
> don't whitelist?

We poison them; look for
Function.prototype.DefineOwnProperty___('caller', ...)

https://codereview.appspot.com/247900043/

[meta...@gmail.com]

LGTM

https://codereview.appspot.com/247900043/

[eri...@gmail.com]

LGTM too,


https://codereview.appspot.com/247900043/