a code review (erights/trivopt)
eri...@gmail.com
gvn review --project https://google-caja.googlecode.com/svn erights/trivopt@2787
Alternatively, to review the latest snapshot of this change
branch, run
gvn --project https://google-caja.googlecode.com/svn review erights/trivopt
to review the following change:
*erights/trivopt@2787 | erights | 2008-10-27 10:59:38 -0800 (Mon, 27 Oct 2008)
Description:
Picking some low hanging performance fruit.
In Cajita, <expr1>[+<expr2>] now translates to <texpr1>[+<texpr2>]
rather than ___.readPub(<texpr1>, <texpr2>) since numbers are
necessarily whitelisted.
asSimpleFunc() is now responsible for freezing its
argument. Translation of calls to declared function names changed to
no longer directly call primFreeze on function, since asSimpleFunc()
will do that.
Some calls to asFirstClass() on fastpaths, where failure would have
been a symptom of a bug needing to be fixed anyway, and where we have
no such known bugs, have been commented out. (It would be nice to have
a statically switchable assert mechanism so we can leave such
diagnostics in the code but not pay for those that are turned off.)
Some inlining and rearranging of code to help fastpaths.
Changed representation of frozenness from having FROZEN___ as an own
property -- which required a hasOwnProperty check --- to having a
FROZEN___ property that points at this same object. An inherited
FROZEN___ fails this check quickly.
Once a simple-function is frozen, its inherited primitive call and
apply methods become whitelisted as canCall.
Some "typeOf()"s reverted to "typeof"s where it's safe to do so.
cajita.hasOwnPropertyOf() and cajita.readOwn() added to speed up some
Valija bottlenecks.
Affected Paths:
M //trunk/src/com/google/caja/cajita.js
M //trunk/src/com/google/caja/parser/quasiliteral/CajitaRewriter.java
M //trunk/src/com/google/caja/parser/quasiliteral/PermitTemplate.java
M //trunk/src/com/google/caja/valija-cajita.js
This is a semiautomated message from "gvn mail". See
<http://code.google.com/p/gvn/> to learn more.
Index: src/com/google/caja/cajita.js
===================================================================
--- src/com/google/caja/cajita.js (//trunk/src/com/google/caja/cajita.js@2777)
+++ src/com/google/caja/cajita.js (//changes/erights/trivopt/trunk/src/com/google/caja/cajita.js@2787)
@@ -492,7 +492,7 @@
}
obj = Object(obj);
var result;
- if (hasOwnProp(obj, 'proto___')) {
+ if (myOriginalHOP.call(obj, 'proto___')) {
var proto = obj.proto___;
if (proto === null) { return (void 0); }
if (isPrototypical(proto)) {
@@ -501,7 +501,7 @@
result = directConstructor(proto);
}
} else {
- if (!hasOwnProp(obj, 'constructor')) {
+ if (!myOriginalHOP.call(obj, 'constructor')) {
// TODO(erights): Detect whether this is a valid constructor
// property in the sense that result is a proper answer. If
// not, at least give a sensible error, which will be hard to
@@ -617,21 +617,20 @@
* obj.
* <p>
* The status of being frozen is not inherited. If A inherits from
- * B (i.e., if A's prototype is B), then (we hope) B must be
- * frozen regardless, but A may or may not be frozen.
+ * B (i.e., if A's prototype is B), A and B each may or may not be
+ * frozen independently. (Though if B is prototypical, then it must
+ * be frozen.)
* <p>
* If typeof <tt>obj</tt> is neither 'object' nor 'function', then
* it's currently considered frozen.
*/
function isFrozen(obj) {
- var t = typeof obj;
- if (t !== 'object' && t !== 'function') {
- return true;
- }
- if (obj === null) { return true; }
+ if (!obj) { return true; }
// TODO(erights): Object(<primitive>) wrappers should also be
// considered frozen.
- return hasOwnProp(obj, 'FROZEN___');
+ if (obj.FROZEN___ === obj) { return true; }
+ var t = typeof obj;
+ return t !== 'object' && t !== 'function';
}
/**
@@ -670,7 +669,7 @@
}
for (var i = 0; i < badFlags.length; i++) {
var flag = badFlags[i];
- if (hasOwnProp(obj, flag)) {
+ if (myOriginalHOP.call(obj, flag)) {
if (!(delete obj[flag])) {
fail('internal: failed delete: ', debugReference(obj), '.', flag);
}
@@ -688,8 +687,12 @@
obj[flag] = false;
}
}
- obj.FROZEN___ = true;
+ obj.FROZEN___ = obj;
if (typeOf(obj) === 'function') {
+ if (isSimpleFunc(obj)) {
+ grantCall(obj, 'call');
+ grantCall(obj, 'apply');
+ }
// Do last to avoid possible infinite recursion.
if (obj.prototype) { primFreeze(obj.prototype); }
}
@@ -894,13 +897,13 @@
////////////////////////////////////////////////////////////////////////
function isCtor(constr) {
- return (typeOf(constr) === 'function') && !! constr.CONSTRUCTOR___;
+ return constr && !! constr.CONSTRUCTOR___;
}
function isSimpleFunc(fun) {
- return (typeOf(fun) === 'function') && !! fun.SIMPLEFUNC___;
+ return fun && !! fun.SIMPLEFUNC___;
}
function isXo4aFunc(func) {
- return (typeOf(func) === 'function') && !! func.XO4A___;
+ return func && !! func.XO4A___;
}
/**
@@ -966,16 +969,16 @@
}
var result = {
call: simpleFrozenFunc(function(self, var_args) {
- if (self === null || self === undefined) { self = USELESS; }
+ if (self === null || self === void 0) { self = USELESS; }
return xfunc.apply(self, Array.slice(arguments, 1));
}),
apply: simpleFrozenFunc(function(self, args) {
- if (self === null || self === undefined) { self = USELESS; }
+ if (self === null || self === void 0) { self = USELESS; }
return xfunc.apply(self, args);
}),
bind: simpleFrozenFunc(function(self, var_args) {
var args = arguments;
- if (self === null || self === undefined) {
+ if (self === null || self === void 0) {
self = USELESS;
args = [self].concat(Array.slice(args, 1));
}
@@ -1060,10 +1063,21 @@
return primFreeze(asCtorOnly(constr));
}
- /** Only simple functions can be called as simple functions */
+ /**
+ * Only simple functions can be called as simple functions.
+ * <p>
+ * It is now <tt>asSimpleFunc</tt>'s responsibility to
+ * <tt>primFreeze(fun)</tt>.
+ */
function asSimpleFunc(fun) {
- if (isSimpleFunc(fun)) {
- return fun;
+// if (isSimpleFunc(fun)) { // inlined below
+ if (fun && fun.SIMPLEFUNC___) {
+ // fastpath shortcut
+ if (fun.FROZEN___ === fun) {
+ return fun;
+ } else {
+ return primFreeze(fun);
+ }
}
enforceType(fun, 'function');
if (isCtor(fun)) {
@@ -1089,7 +1103,7 @@
// <tt>Date</tt>, <tt>RegExp</tt>, and <tt>Error</tt>. (Not
// sure about <tt>Array</tt>.) We should understand these as
// well before introducing a proper solution.
- return fun;
+ return primFreeze(fun);
}
fail("Constructors can't be called as simple functions: ", fun);
}
@@ -1190,18 +1204,27 @@
*/
function canReadPub(obj, name) {
name = String(name);
- if (endsWith__.test(name)) { return false; }
if (obj === null) { return false; }
if (obj === void 0) { return false; }
- if (canRead(obj, name)) { return true; }
+ if (obj[name + '_canRead___']) { return true; }
+ if (endsWith__.test(name)) { return false; }
if (name === 'toString') { return false; }
if (!isJSONContainer(obj)) { return false; }
- if (!hasOwnProp(obj, name)) { return false; }
+ if (!myOriginalHOP.call(obj, name)) { return false; }
fastpathRead(obj, name);
return true;
}
/**
+ *
+ */
+ function hasOwnPropertyOf(obj, name) {
+ if (typeof name === 'number') { return hasOwnProp(obj, name); }
+ name = String(name);
+ return canReadPub(obj, name) && myOriginalHOP.call(obj, name);
+ }
+
+ /**
* Implements <tt>name in obj</tt>
*/
function inPub(name, obj) {
@@ -1234,7 +1257,7 @@
* instead.
*/
function readPub(obj, name) {
- if ((typeof name) === 'number') {
+ if (typeof name === 'number') {
if (typeof obj === 'string') {
// In partial anticipation of ES3.1.
// TODO(erights): Once ES3.1 settles, revisit this and
@@ -1252,6 +1275,31 @@
}
/**
+ * If <tt>obj</tt> is a non-function object with readable
+ * own property <tt>name</tt>, return <tt>obj[name]</tt>, else
+ * <tt>pumpkin</tt>.
+ * <p>
+ * Provides a fastpath for Valija's <tt>read()</tt> function
+ * <tt>$v.r()</tt>.
+ */
+ function readOwn(obj, name, pumpkin) {
+ if (typeof obj !== 'object' || !obj) { return pumpkin; }
+ if (typeof name === 'number') {
+ if (myOriginalHOP.call(obj, name)) { return obj[name]; }
+ return pumpkin;
+ }
+ name = String(name);
+ if (!myOriginalHOP.call(obj, name)) { return pumpkin; }
+ // inline remaining relevant cases from canReadPub
+ if (obj[name + '_canRead___']) { return obj[name]; }
+ if (endsWith__.test(name)) { return pumpkin; }
+ if (name === 'toString') { return pumpkin; }
+ if (!isJSONContainer(obj)) { return pumpkin; }
+ fastpathRead(obj, name);
+ return obj[name];
+ }
+
+ /**
* Ensure that all the permitsUsed starting at result are forever
* safe to allow without runtime checks.
*/
@@ -1287,7 +1335,7 @@
*/
function readImport(module_imports, name, opt_permitsUsed) {
var result;
- if ((typeof name) === 'number') {
+ if (typeof name === 'number') {
result = module_imports[name];
} else {
result = readPub(module_imports, name);
@@ -1331,11 +1379,13 @@
* canReadProp. Otherwise according to whitelisting.
*/
function canEnumPub(obj, name) {
+ if (obj === null) { return false; }
+ if (obj === void 0) { return false; }
name = String(name);
+ if (obj[name + '_canEnum___']) { return true; }
if (endsWith__.test(name)) { return false; }
- if (canEnum(obj, name)) { return true; }
if (!isJSONContainer(obj)) { return false; }
- if (!hasOwnProp(obj, name)) { return false; }
+ if (!myOriginalHOP.call(obj, name)) { return false; }
fastpathEnumOnly(obj, name);
if (name === 'toString') { return true; }
fastpathRead(obj, name);
@@ -1347,7 +1397,7 @@
*/
function canEnumOwn(obj, name) {
name = String(name);
- return hasOwnProp(obj, name) && canEnumPub(obj, name);
+ return canEnumPub(obj, name) && myOriginalHOP.call(obj, name);
}
/**
@@ -1464,12 +1514,12 @@
* which we can memoize.
*/
function canCallPub(obj, name) {
- name = String(name);
- if (endsWith__.test(name)) { return false; }
if (obj === null) { return false; }
if (obj === void 0) { return false; }
+ name = String(name);
if (canCall(obj, name)) { return true; }
if (!canReadPub(obj, name)) { return false; }
+ if (endsWith__.test(name)) { return false; }
if (name === 'toString') { return false; }
var func = obj[name];
if (!isSimpleFunc(func) && !isXo4aFunc(func)) {
@@ -1484,9 +1534,8 @@
*/
function callPub(obj, name, args) {
name = String(name);
- if (canCallPub(obj, name)) {
- var meth = obj[name];
- return meth.apply(obj, args);
+ if ((obj && obj[name + '_canCall___']) || canCallPub(obj, name)) {
+ return obj[name].apply(obj, args);
}
if (obj.handleCall___) { return obj.handleCall___(name, args); }
fail('not callable:', debugReference(obj), '.', name);
@@ -1509,8 +1558,8 @@
*/
function canSetPub(obj, name) {
name = String(name);
- if (endsWith__.test(name)) { return false; }
if (canSet(obj, name)) { return true; }
+ if (endsWith__.test(name)) { return false; }
if (name === 'valueOf') { return false; }
if (name === 'toString') { return false; }
return !isFrozen(obj) && isJSONContainer(obj);
@@ -1518,10 +1567,18 @@
/** A client of obj attempts to assign to one of its properties. */
function setPub(obj, name, val) {
+// val = asFirstClass(val);
+ if (typeof name === 'number' &&
+ obj instanceof Array &&
+ obj.FROZEN___ !== obj) {
+ return obj[name] = val;
+ }
name = String(name);
- if (canSetPub(obj, name)) {
+ if (obj && obj[name + '_canSet___']) {
+ return obj[name] = val;
+ } else if (canSetPub(obj, name)) {
fastpathSet(obj, name);
- return obj[name] = asFirstClass(val);
+ return obj[name] = val;
} else {
return obj.handleSet___(name, val);
}
@@ -2004,8 +2061,7 @@
]);
grantRead(Object.prototype, 'length');
handleGeneric(Object.prototype, 'hasOwnProperty', function(name){
- name = String(name);
- return canReadPub(this, name) && hasOwnProp(this, name);
+ return hasOwnPropertyOf(this, name);
});
handleGeneric(Object.prototype, 'propertyIsEnumerable', function(name) {
name = String(name);
@@ -2474,7 +2530,7 @@
function initializeMap(list) {
var result = {};
for (var i = 0; i < list.length; i+=2) {
- setPub(result, list[i], list[i+1]);
+ setPub(result, list[i], asFirstClass(list[i+1]));
}
return result;
}
@@ -2703,7 +2759,7 @@
for (var k in obj) {
if (hasOwnProp(obj, k)) {
if (implicit && !endsWith__.test(k)) {
- if (!hasOwnProp(seen, k)) {
+ if (!myOriginalHOP.call(seen, k)) {
seen[k] = true;
result.push(k);
}
@@ -2711,7 +2767,7 @@
var match = Attribute.exec(k);
if (match !== null) {
var base = match[1];
- if (!hasOwnProp(seen, base)) {
+ if (!myOriginalHOP.call(seen, base)) {
seen[base] = true;
result.push(base);
}
@@ -2784,6 +2840,8 @@
// Accessing properties
canReadPub: canReadPub, readPub: readPub,
+ hasOwnPropertyOf: hasOwnPropertyOf,
+ readOwn: readOwn,
canEnumPub: canEnumPub,
canEnumOwn: canEnumOwn,
canInnocentEnum: canInnocentEnum,
Index: src/com/google/caja/parser/quasiliteral/CajitaRewriter.java
===================================================================
--- src/com/google/caja/parser/quasiliteral/CajitaRewriter.java (//trunk/src/com/google/caja/parser/quasiliteral/CajitaRewriter.java@2777)
+++ src/com/google/caja/parser/quasiliteral/CajitaRewriter.java (//changes/erights/trivopt/trunk/src/com/google/caja/parser/quasiliteral/CajitaRewriter.java@2787)
@@ -925,6 +925,26 @@
new Rule() {
@Override
@RuleDescription(
+ name="readNumPublic",
+ synopsis="",
+ reason="",
+ matches="@o[+@s]",
+ substitutes="@o[+@s]")
+ public ParseTreeNode fire(ParseTreeNode node, Scope scope, MessageQueue mq) {
+ Map<String, ParseTreeNode> bindings = match(node);
+ if (bindings != null) {
+ return QuasiBuilder.substV(
+ "@o[+@s]",
+ "o", expand(bindings.get("o"), scope, mq),
+ "s", expand(bindings.get("s"), scope, mq));
+ }
+ return NONE;
+ }
+ },
+
+ new Rule() {
+ @Override
+ @RuleDescription(
name="readIndexPublic",
synopsis="",
reason="",
@@ -1697,6 +1717,27 @@
new Rule() {
@Override
@RuleDescription(
+ name="callDeclaredFunc",
+ synopsis="When calling a declared function name, leave the freezing to asSimpleFunc.",
+ reason="",
+ matches="@fname(@as*)",
+ substitutes="___.asSimpleFunc(@fname)(@as*)")
+ public ParseTreeNode fire(ParseTreeNode node, Scope scope, MessageQueue mq) {
+ Map<String, ParseTreeNode> bindings = match(node);
+ if (bindings != null &&
+ scope.isDeclaredFunctionReference(bindings.get("fname"))) {
+ return QuasiBuilder.substV(
+ "___.asSimpleFunc(@fname)(@as*)",
+ "fname", bindings.get("fname"),
+ "as", expandAll(bindings.get("as"), scope, mq));
+ }
+ return NONE;
+ }
+ },
+
+ new Rule() {
+ @Override
+ @RuleDescription(
name="callFunc",
synopsis="",
reason="",
Index: src/com/google/caja/parser/quasiliteral/PermitTemplate.java
===================================================================
--- src/com/google/caja/parser/quasiliteral/PermitTemplate.java (//trunk/src/com/google/caja/parser/quasiliteral/PermitTemplate.java@2777)
+++ src/com/google/caja/parser/quasiliteral/PermitTemplate.java (//changes/erights/trivopt/trunk/src/com/google/caja/parser/quasiliteral/PermitTemplate.java@2787)
@@ -80,6 +80,8 @@
"snapshot", CanCall,
"canReadPub", CanCall,
"readPub", CanCall,
+ "hasOwnPropertyOf", CanCall,
+ "readOwn", CanCall,
"canEnumPub", CanCall,
"canEnumOwn", CanCall,
"canInnocentEnum", CanCall,
Index: src/com/google/caja/valija-cajita.js
===================================================================
--- src/com/google/caja/valija-cajita.js (//trunk/src/com/google/caja/valija-cajita.js@2777)
+++ src/com/google/caja/valija-cajita.js (//changes/erights/trivopt/trunk/src/com/google/caja/valija-cajita.js@2787)
@@ -231,23 +231,21 @@
}
}
- function hasOwnProp(obj, name) {
- return {}.hasOwnProperty.call(obj, name);
- }
+ var pumpkin = {};
/**
* Handle Valija <tt><i>obj</i>[<i>name</i>]</tt>.
*/
function read(obj, name) {
+ var result = cajita.readOwn(obj, name, pumpkin);
+ if (result !== pumpkin) { return result; }
+
if (typeof obj === 'function') {
return getShadow(obj)[name];
}
- if (obj === null || obj === undefined) {
+ if (obj === null || obj === void 0) {
throw new TypeError('Cannot read property "' + name + '" from ' + obj);
}
- if (hasOwnProp(obj, name)) {
- return obj[name];
- }
// BUG TODO(erights): figure out why things break when the
// following line (which really shouldn't be there) is deleted.
@@ -379,6 +377,8 @@
}
function readOuter(name) {
+ var result = cajita.readOwn(own, name, pumpkin);
+ if (result !== pumpkin) { return result; }
if (canReadRev(name, outers)) {
return read(outers, name);
} else {
David-Sarah Hopwood
> Picking some low hanging performance fruit.
>
> In Cajita, <expr1>[+<expr2>] now translates to <texpr1>[+<texpr2>]
> rather than ___.readPub(<texpr1>, <texpr2>) since numbers are
> necessarily whitelisted.
Note that it is easy to extend this to other expressions that always
produce a number:
<NumericLiteral>
<expr> ('++' | '--')
('++' | '--') <expr>
<NumericUnaryOp> <expr>
<expr> <NumericBinaryOp> <expr>
<NumericUnaryOp> : one of
+ - ~
<NumericBinaryOp> : one of
* / % - << >> >>> & ^ |
(Binary '+' is unfortunately not a <NumericBinaryOp>, because it can return
a string. Jacaranda does allow direct use of binary '+' expressions as an
index in some cases, but that fruit hangs from a somewhat higher branch.)
--
David-Sarah Hopwood
David-Sarah Hopwood
> Note that it is easy to extend this to other expressions that always
If they don't throw an exception, that is -- but throwing an exception
causes no problem here.
--
David-Sarah Hopwood
Mike Samuel
David-Sarah Hopwood
> In
> http://code.google.com/p/google-caja/source/browse/changes/mikesamuel/array-optimization/trunk/src/com/google/caja/parser/quasiliteral/opt/ArrayIndexOptimization.java?spec=svn2776&r=2776the
> isNumericOrUndef() is true for the operators ( unary+, unary-, ~, ++,
> --, &, |, ^, /, *, %, -, <<, >>, >>>, void ) and recurses to the last two
> operands to ( &&, ||, ?: ) and to the last operand to the comma operator.
This looks correct. (And thanks, the comma operator case was missing in the
Jacaranda 0.3 spec.)
"IDENTITY" is a misleading operator name for unary '+', though, since it
is not the identity function when applied to a non-number value.
--
David-Sarah Hopwood
Mike Samuel
--
David-Sarah Hopwood
Mark S. Miller
--
Cheers,
--MarkM
Mike Samuel
David-Sarah Hopwood
>> is not the identity function when applied to a non-number value.
>
> yep. Any ideas for a better name?
UNARY_PLUS.
--
David-Sarah Hopwood
David-Sarah Hopwood
> <http://www.axisofstevil.com/Images/transmogrifier.gif>
I could do with one of those!
Mark S. Miller <eri...@google.com> wrote:
> How about TO_NUMBER?
That also works for me.
--
David-Sarah Hopwood
Mike Samuel
mike
Mike Samuel
David-Sarah Hopwood
> x++ and x-- do not always produce numbers on IE6&IE7 though they do always
> assign a number to their LeftHandSideExpression operand.
Really? That would definitely be a bug. ECMA-262 sections 11.4.4 and 11.4.5
are clear that the value used in the assignment attempt, Result(4), is the
same value that is returned. This should always be a number because it is
specified as being a result of 'ToNumber(GetValue(x)) + 1' or
'ToNumber(GetValue(x)) - 1'.
(PutValue might silently fail to assign the value, but that doesn't matter
here.)
Do you have a specific test case that demonstrates this behaviour of IE?
If IE7 doesn't implement this correctly, then I'll have to remove ++ and
-- from the operators that produce an exposed expression (which can be used
as an array index) in Jacaranda 0.4. Which is irritating, because 'foo[i++]'
is a commonly used idiom.
(I don't care about IE6 because that browser is unsecurable.)
--
David-Sarah Hopwood
Mike Samuel
Really? That would definitely be a bug. ECMA-262 sections 11.4.4 and 11.4.5
Mike Samuel wrote:
> x++ and x-- do not always produce numbers on IE6&IE7 though they do always
> assign a number to their LeftHandSideExpression operand.
are clear that the value used in the assignment attempt, Result(4), is the
same value that is returned. This should always be a number because it is
specified as being a result of 'ToNumber(GetValue(x)) + 1' or
'ToNumber(GetValue(x)) - 1'.
(PutValue might silently fail to assign the value, but that doesn't matter
here.)
Do you have a specific test case that demonstrates this behaviour of IE?
See the pending browser-expectations CL.
http://code.google.com/p/google-caja/source/browse/changes/mikesamuel/browser-expectations/trunk/tests/com/google/caja/browser-expectations.html
David-Sarah Hopwood
> 2008/10/31 David-Sarah Hopwood <david....@industrial-designers.co.uk>
>
>> Mike Samuel wrote:
>>> x++ and x-- do not always produce numbers on IE6&IE7 though they do
>> always
>>> assign a number to their LeftHandSideExpression operand.
>> Really? That would definitely be a bug. ECMA-262 sections 11.4.4 and 11.4.5
>> are clear that the value used in the assignment attempt, Result(4), is the
>> same value that is returned. This should always be a number because it is
>> specified as being a result of 'ToNumber(GetValue(x)) + 1' or
>> 'ToNumber(GetValue(x)) - 1'.
>>
>> (PutValue might silently fail to assign the value, but that doesn't matter
>> here.)
>>
>> Do you have a specific test case that demonstrates this behaviour of IE?
>
> See the pending browser-expectations CL.
> http://code.google.com/p/google-caja/source/browse/changes/mikesamuel/browser-expectations/trunk/tests/com/google/caja/browser-expectations.html
I cannot reproduce this on IE version 7.0.6001.18000 on Vista under
squarefree shell.
I define
function assertEquals(s, x, y) { print((x === y ? "PASS:" : "FAIL:")+s); }
and then paste in the body of function testNumericOperators(), and
every test prints PASS.
(I also checked FF3.0.3 as a control, that prints PASS for every test as
well.)
Is there a known exact version of IE6 or 7 for which this test fails?
--
David-Sarah Hopwood
Mike Samuel
In
checkOperator(function (x) { return ++x; });
checkOperator(function (x) { return --x; });
checkOperator(function (x) { return x -= 1; });
// x++ and y++ do not yield a number on IE6 & IE7, but they should assign
// a number to their operand.
the comment is meant to indicate that
checkOperator(function (x) { return x++; });
checkOperator(function (x) { return x--; });
are missing from that file.
If you add those back, you should be able to repeat the problem.
As a short test, try this
typeof ((function (x) { return x++; })('foo'))
David-Sarah Hopwood
> // x++ and y++ do not yield a number on IE6 & IE7, but they should
The comment should say x++ and x--.
> the comment is meant to indicate that
> checkOperator(function (x) { return x++; });
> checkOperator(function (x) { return x--; });
> are missing from that file.
>
> If you add those back, you should be able to repeat the problem.
Thanks.
function whatis(v) { print(typeof(v)+' '+v); }
(function() { var x = 'foo'; whatis(x++); })();
//string foo
(function(x) { whatis(x++); })('foo');
//string foo
var y = 'foo'; whatis(y++);
//number NaN
(function() { z = 'foo'; whatis(z++); })();
//number NaN
(function() { var o = {x:'foo'}; whatis(o.x++); })();
//number NaN
Seems to be a misoptimization of ++/-- for local variables and parameters.
This results in a fatal security bug in Jacaranda 0.3 on IE:
(function() {
var c = 'constructor';
var F = (function(){})[c++]; // Function constructor
F('alert("toast")')();
})();
In Jacaranda 0.4, postincrement/postdecrement will be disallowed in
index expressions for modules that are not marked as depending on ES3.1.
(If the browser implements ES3.1 then it will not have this bug, and an
ES3.1-dependent module will refuse to run on affected versions of IE.)
[I wrote earlier:
> ECMA-262 sections 11.4.4 and 11.4.5 are clear that the value used in the
> assignment attempt, Result(4), is the same value that is returned.
That's for preincrement/predecrement. But sections 11.3.1 and 11.3.2 are
equally clear that postincrement/postdecrement should always evaluate to
a result of ToNumber, so this is still definitely a bug in IE.]
--
David-Sarah Hopwood
David-Sarah Hopwood
> function whatis(v) { print(typeof(v)+' '+v); }
>
> (function() { var x = 'foo'; whatis(x++); })();
> //string foo
>
> (function(x) { whatis(x++); })('foo');
> //string foo
> Seems to be a misoptimization of ++/-- for local variables and parameters.
>
> This results in a fatal security bug in Jacaranda 0.3 on IE:
>
> (function() {
> var c = 'constructor';
> var F = (function(){})[c++]; // Function constructor
> F('alert("toast")')();
> })();
Note that the changeset
<http://code.google.com/p/google-caja/source/browse/changes/mikesamuel/array-optimization/trunk/src/com/google/caja/parser/quasiliteral/opt/ArrayIndexOptimization.java?spec=svn2819&r=2819>
treats POST_DECREMENT and POST_INCREMENT as number-producing operators, and
therefore has the same security bug, I think. It would be possible and
correct to rewrite 'a[x++]' to 'a[+(x++)]', though.
In any case, this needs to be documented in the Attack Vectors section of
the wiki.
--
David-Sarah Hopwood
David-Sarah Hopwood
> function whatis(v) { print(typeof(v)+' '+v); }
>
> (function() { var x = 'foo'; whatis(x++); })();
> //string foo
>
> (function(x) { whatis(x++); })('foo');
> //string foo
>
> var y = 'foo'; whatis(y++);
> //number NaN
>
> (function() { z = 'foo'; whatis(z++); })();
> //number NaN
>
> (function() { var o = {x:'foo'}; whatis(o.x++); })();
> //number NaN
>
> Seems to be a misoptimization of ++/-- for local variables and parameters.
... and only if they are local to the current function, not an enclosing
function:
(function() {var x = 'foo'; (function() { whatis(x++); })();})();
//number NaN
> In any case, this needs to be documented in the Attack Vectors section of
> the wiki.
<http://code.google.com/p/google-caja/wiki/PostIncrementAndDecrementCanReturnNonNumber>
--
David-Sarah Hopwood
Mike Samuel
[...]
David-Sarah Hopwood wrote:
> function whatis(v) { print(typeof(v)+' '+v); }
>
> (function() { var x = 'foo'; whatis(x++); })();
> //string foo
>
> (function(x) { whatis(x++); })('foo');
> //string foo
> Seems to be a misoptimization of ++/-- for local variables and parameters.Note that the changeset
>
> This results in a fatal security bug in Jacaranda 0.3 on IE:
>
> (function() {
> var c = 'constructor';
> var F = (function(){})[c++]; // Function constructor
> F('alert("toast")')();
> })();
<http://code.google.com/p/google-caja/source/browse/changes/mikesamuel/array-optimization/trunk/src/com/google/caja/parser/quasiliteral/opt/ArrayIndexOptimization.java?spec=svn2819&r=2819>
treats POST_DECREMENT and POST_INCREMENT as number-producing operators, and
therefore has the same security bug, I think. It would be possible and
correct to rewrite 'a[x++]' to 'a[+(x++)]', though.
Yep. Thanks for reminding me.
In any case, this needs to be documented in the Attack Vectors section of
the wiki.
--
David-Sarah Hopwood
Mike Samuel
2008/11/1 David-Sarah Hopwood <david....@industrial-designers.co.uk>[...]
David-Sarah Hopwood wrote:
> function whatis(v) { print(typeof(v)+' '+v); }
>
> (function() { var x = 'foo'; whatis(x++); })();
> //string foo
>
> (function(x) { whatis(x++); })('foo');
> //string foo
> Seems to be a misoptimization of ++/-- for local variables and parameters.Note that the changeset
>
> This results in a fatal security bug in Jacaranda 0.3 on IE:
>
> (function() {
> var c = 'constructor';
> var F = (function(){})[c++]; // Function constructor
> F('alert("toast")')();
> })();
<http://code.google.com/p/google-caja/source/browse/changes/mikesamuel/array-optimization/trunk/src/com/google/caja/parser/quasiliteral/opt/ArrayIndexOptimization.java?spec=svn2819&r=2819>
treats POST_DECREMENT and POST_INCREMENT as number-producing operators, and
therefore has the same security bug, I think. It would be possible and
correct to rewrite 'a[x++]' to 'a[+(x++)]', though.
Yep. Thanks for reminding me.
This should be fixed in the most recent snapshot of mikesamuel/array-optimization. I removed the post-{inc,dec}rement ops from ArrayIndexOptimization.isNumericOperator. Since they are guaranteed to assign a numeric value even if they don't yield one, I checked the code that determines whether a variable is always null or undefined, and didn't have to change anything, but did add a note about why the branch that handles the post-{inc,dec} operators doesn't need to do anything.
See http://code.google.com/p/google-caja/source/browse/changes/mikesamuel/array-optimization/trunk/src/com/google/caja/parser/quasiliteral/opt/ArrayIndexOptimization.java lines 147 and 273.
David-Sarah Hopwood
> http://code.google.com/p/google-caja/source/browse/changes/mikesamuel/array-optimization/trunk/src/com/google/caja/parser/quasiliteral/opt/ArrayIndexOptimization.java
* <li>That a variable that is only ever assigned a numeric value, when used
* as an object property name before initialization matches the name
* {@code NaN} instead of the name {@code undefined}. This is documented
* as a gotcha in the cajoler spec, and can be seen in the following code:
* <pre>
* var o = { 0: 'a', NaN: 'b', undefined: 'c' };
* var i;
* alert(a[i]);
* i = 0;
* alert(a[i]);
* </pre>
The two instances of 'a[i]' should be 'o[i]'. Also the wording is unclear:
it should say that the correct result is to alert 'c' then 'a', but the
optimized code alerts 'b' than 'a' as a result of the insertion of
unary '+' in optBindings (I think).
Otherwise I can't see any obvious remaining errors.
--
David-Sarah Hopwood