googlearchive / caja

Caja is a tool for safely embedding third party HTML, CSS and JavaScript in your website.
Apache License 2.0
1.13k stars 114 forks source link

Typeof rewriting is broken around 'this' #1629

Closed kpreid closed 9 years ago

kpreid commented 9 years ago

Original issue 1629 created by metaweta on 2013-01-24T02:18:02.000Z:

What steps will reproduce the problem?

<script> function A() {} A.prototype.x = 1; A.prototype.xType = function() { return typeof this.x; } var a = new A; cajaVM.log(a.xType()); </script>

What is the expected output? What do you see instead? Expect 'number', see 'undefined'

Please use labels and text to provide additional information.

The typeof operator gets rewritten to

A.prototype.xType = function () { return function () { try { return typeof this.x; } catch (e) { return 'undefined'; } }(); };

Here, the function is invoked with 'this' bound to undefined. The rewrite should be something like

A.prototype.xType = function () { return function (self) { try { return typeof self.x; } catch (e) { return 'undefined'; } }(this); };

kpreid commented 9 years ago

Comment #1 originally posted by metaweta on 2013-01-24T02:25:18.000Z:

Better: typeof this.x => (function () { try { return typeof this.x; } catch (e) { return 'undefined'; } }).call(this);

kpreid commented 9 years ago

Comment #2 originally posted by metaweta on 2013-01-24T02:31:37.000Z:

Jasmine tickles this bug: every call to expect() is failing.

kpreid commented 9 years ago

Comment #3 originally posted by erights on 2013-01-24T02:59:38.000Z:

I don't understand. Are we talking ES5? On the playground in ES53, I get 'number' as expected.

On ES5, why is this typeof getting rewritten at all? Its operand is an expression other than a simple variable name. This case should already be fine without rewriting. I think the right fix is that

typeof varname

should be rewritten to

typeof global.varname

(for however global is spelled in the rewritten context). And all other typeofs should not be rewritten.

kpreid commented 9 years ago

Comment #4 originally posted by jasvir on 2013-01-24T03:03:48.000Z:

I was rewriting the argument of typeof irrespective of the argument. CL to follow shortly.

kpreid commented 9 years ago

Comment #5 originally posted by jasvir@google.com on 2013-01-24T03:06:00.000Z:

Just clarifying, erights - we can't rewrite arbitrary typeof varname to global.varname right? Only if typeof varname invokes the poisonpill.

kpreid commented 9 years ago

Comment #6 originally posted by erights on 2013-01-24T03:17:59.000Z:

Kinda. Good catch. But the rule should be that varname should be rewritten if and only if varname is free in that context, irrespective of whether it would access a pseudo-global property of that name. Note that the latter issue, which determines whether it would inappropriately throw, isn't statically known and so can't be the basis for rewriting.

kpreid commented 9 years ago

Comment #7 originally posted by jasvir on 2013-01-24T03:37:53.000Z:

I intend to rewrite :

typeof var

to:

(function(var) { try { return typeof var; } catch (e) { if e instanceof ReferenceError return 'undefined'; throw e; } }).call(var)

In what circumstances is that incorrect?

kpreid commented 9 years ago

Comment #8 originally posted by erights on 2013-01-24T05:01:38.000Z:

If you're trying to avoid scope analysis, then your pattern is almost correct. As written, "}).call(var)" will throw ReferenceError before calling, defeating the point. But a slight variation:

(function() { try { return typeof var; } catch (e) { if e instanceof ReferenceError return 'undefined'; throw e; } }).call()

should be correct. As long as the "var" identifier in question is not "arguments" or "this".

kpreid commented 9 years ago

Comment #9 originally posted by jasvir on 2013-01-24T05:02:19.000Z:

Note I am doing this transform to avoid the cost of scope analysis.

kpreid commented 9 years ago

Comment #10 originally posted by erights on 2013-01-24T05:07:48.000Z:

My "}).call()" should of course just be "})()"

kpreid commented 9 years ago

Comment #11 originally posted by jasvir on 2013-01-30T17:23:13.000Z:

@5233