Issue 10711045: SES: Repair v8 Array methods getting implicit this=window. (Closed)
|
Created:
12 years, 6 months ago by kpreid2 Modified:
12 years, 4 months ago CC:
caja-discuss-undisclosed_googlegroups.com, MarkM, felix8a, ihab.awad, Jasvir, kpreid2, metaweta, MikeSamuel Base URL:
http://google-caja.googlecode.com/svn/trunk/ Visibility:
Public. |
DescriptionFixes <http://code.google.com/p/v8/issues/detail?id=2758>.
Array.prototype.{pop, push, shift, unshift, slice, splice, concat} are
wrapped if they are not already wrapped by other repairs.
Future work after this security patch is deployed:
* In principle, the bug might get partially fixed, and we should
thus have an independent test and repair for each method to avoid
unnecessary wrappers and the subtest() gimmick; I have refrained
from adding such novel things to repairES5's patterns for now.
* es53-test-scan-guest should be extended to detect this category of
problems.
@r5550
Patch Set 1 #
Total comments: 16
Patch Set 2 : SES: Repair v8 Array methods getting implicit this=window. #Patch Set 3 : SES: Repair v8 Array methods getting implicit this=window. #
Total comments: 2
Patch Set 4 : SES: Repair v8 Array methods getting implicit this=window. #MessagesTotal messages: 10
https://codereview.appspot.com/10711045/diff/1/src/com/google/caja/ses/repair... File src/com/google/caja/ses/repairES5.js (right): https://codereview.appspot.com/10711045/diff/1/src/com/google/caja/ses/repair... src/com/google/caja/ses/repairES5.js:831: var origLength = global.length; What if global.length doesn't exist or is not a number? https://codereview.appspot.com/10711045/diff/1/src/com/google/caja/ses/repair... src/com/google/caja/ses/repairES5.js:833: var desc = Object.getOwnPropertyDescriptor(global, name); I would name this variable opt_desc to emphasize that it is not unusual for it to be undefined (void 0). https://codereview.appspot.com/10711045/diff/1/src/com/google/caja/ses/repair... src/com/google/caja/ses/repairES5.js:838: var minSaveLength = (isFinite(origLength) ? origLength : 0) + 2; I don't understand the purpose of minSaveLength and found. https://codereview.appspot.com/10711045/diff/1/src/com/google/caja/ses/repair... src/com/google/caja/ses/repairES5.js:845: function subtest(name, args, failPredicate) { Nice. https://codereview.appspot.com/10711045/diff/1/src/com/google/caja/ses/repair... src/com/google/caja/ses/repairES5.js:862: // not possible on Firefox (which doesn't have this bug anyway) In that case, perhaps "return false;" here? https://codereview.appspot.com/10711045/diff/1/src/com/google/caja/ses/repair... src/com/google/caja/ses/repairES5.js:4050: urls: ['http://code.google.com/p/v8/issues/detail?id=2758'], Please put the Caja bug here as well. https://codereview.appspot.com/10711045/diff/1/src/com/google/caja/ses/repair... src/com/google/caja/ses/repairES5.js:4052: tests: [] // TODO(erights): Add to test262 Please make this TODO someone else's.
Sign in to reply to this message.
Fixes <http://code.google.com/p/v8/issues/detail?id=2758>. Array.prototype.{pop, push, shift, unshift, slice, splice, concat} are wrapped if they are not already wrapped by other repairs. Future work after this security patch is deployed: * In principle, the bug might get partially fixed, and we should thus have an independent test and repair for each method to avoid unnecessary wrappers and the subtest() gimmick; I have refrained from adding such novel things to repairES5's patterns for now. * es53-test-scan-guest should be extended to detect this category of problems.
Sign in to reply to this message.
https://codereview.appspot.com/10711045/diff/1/src/com/google/caja/ses/repair... File src/com/google/caja/ses/repairES5.js (right): https://codereview.appspot.com/10711045/diff/1/src/com/google/caja/ses/repair... src/com/google/caja/ses/repairES5.js:831: var origLength = global.length; On 2013/07/03 21:37:31, MarkM wrote: > What if global.length doesn't exist or is not a number? Then we treat it like 0; see minSaveLength. I've now inlined origLength to clarify. https://codereview.appspot.com/10711045/diff/1/src/com/google/caja/ses/repair... src/com/google/caja/ses/repairES5.js:833: var desc = Object.getOwnPropertyDescriptor(global, name); On 2013/07/03 21:37:31, MarkM wrote: > I would name this variable opt_desc to emphasize that it is not unusual for it > to be undefined (void 0). Done. Also made the return value explicitly boolean. https://codereview.appspot.com/10711045/diff/1/src/com/google/caja/ses/repair... src/com/google/caja/ses/repairES5.js:838: var minSaveLength = (isFinite(origLength) ? origLength : 0) + 2; On 2013/07/03 21:37:31, MarkM wrote: > I don't understand the purpose of minSaveLength and found. We want to record the state of all properties in the set [length, 0, 1, 2, ...] up to the highest index that might be of interest. Indexes which are of interest are those which - are < length (handled by minSaveLength), - actually exist (handled by found), or - are possibly mutated by our tests (handled by the + 2). Added a comment. https://codereview.appspot.com/10711045/diff/1/src/com/google/caja/ses/repair... src/com/google/caja/ses/repairES5.js:845: function subtest(name, args, failPredicate) { On 2013/07/03 21:37:31, MarkM wrote: > Nice. What is not nice about it is that if one of the repairs fails, this will just return false without indicating which part is the problem. Ideally, this would be one test/repair per property, with shared implementation code — or the repairES5 architecture would have a way to be more specific than 'return false'. https://codereview.appspot.com/10711045/diff/1/src/com/google/caja/ses/repair... src/com/google/caja/ses/repairES5.js:862: // not possible on Firefox (which doesn't have this bug anyway) On 2013/07/03 21:37:31, MarkM wrote: > In that case, perhaps "return false;" here? Paranoia. If Firefox gains something like this bug, then at least some of these tests are still useful. Added a comment. https://codereview.appspot.com/10711045/diff/1/src/com/google/caja/ses/repair... src/com/google/caja/ses/repairES5.js:4050: urls: ['http://code.google.com/p/v8/issues/detail?id=2758'], On 2013/07/03 21:37:31, MarkM wrote: > Please put the Caja bug here as well. Done. https://codereview.appspot.com/10711045/diff/1/src/com/google/caja/ses/repair... src/com/google/caja/ses/repairES5.js:4052: tests: [] // TODO(erights): Add to test262 On 2013/07/03 21:37:31, MarkM wrote: > Please make this TODO someone else's. Done.
Sign in to reply to this message.
lgtm++ and thanks for the quick fix! https://codereview.appspot.com/10711045/diff/1/src/com/google/caja/ses/repair... File src/com/google/caja/ses/repairES5.js (right): https://codereview.appspot.com/10711045/diff/1/src/com/google/caja/ses/repair... src/com/google/caja/ses/repairES5.js:889: if (status !== false) { return status; } Is there a way to turn this into one long Boolean expression so as to avoid the error-prone repetition of "if (status !== false)" conditionals?
Sign in to reply to this message.
Fixes <http://code.google.com/p/v8/issues/detail?id=2758>. Array.prototype.{pop, push, shift, unshift, slice, splice, concat} are wrapped if they are not already wrapped by other repairs. Future work after this security patch is deployed: * In principle, the bug might get partially fixed, and we should thus have an independent test and repair for each method to avoid unnecessary wrappers and the subtest() gimmick; I have refrained from adding such novel things to repairES5's patterns for now. * es53-test-scan-guest should be extended to detect this category of problems.
Sign in to reply to this message.
https://codereview.appspot.com/10711045/diff/1/src/com/google/caja/ses/repair... File src/com/google/caja/ses/repairES5.js (right): https://codereview.appspot.com/10711045/diff/1/src/com/google/caja/ses/repair... src/com/google/caja/ses/repairES5.js:889: if (status !== false) { return status; } On 2013/07/03 23:02:08, ihab.awad wrote: > Is there a way to turn this into one long Boolean expression so as to > avoid the error-prone repetition of "if (status !== false)" > conditionals? Done.
Sign in to reply to this message.
LGTM given typeof guarding isFinite https://codereview.appspot.com/10711045/diff/10001/src/com/google/caja/ses/re... File src/com/google/caja/ses/repairES5.js (right): https://codereview.appspot.com/10711045/diff/10001/src/com/google/caja/ses/re... src/com/google/caja/ses/repairES5.js:841: var minSaveLength = (isFinite(global.length) ? global.length : 0) + 2; isFinite('') === true isFinite('1') === true isFinite does a ToNumber on its argument. I think you should guard it with a typeof x === 'number'.
Sign in to reply to this message.
Fixes <http://code.google.com/p/v8/issues/detail?id=2758>. Array.prototype.{pop, push, shift, unshift, slice, splice, concat} are wrapped if they are not already wrapped by other repairs. Future work after this security patch is deployed: * In principle, the bug might get partially fixed, and we should thus have an independent test and repair for each method to avoid unnecessary wrappers and the subtest() gimmick; I have refrained from adding such novel things to repairES5's patterns for now. * es53-test-scan-guest should be extended to detect this category of problems.
Sign in to reply to this message.
https://codereview.appspot.com/10711045/diff/10001/src/com/google/caja/ses/re... File src/com/google/caja/ses/repairES5.js (right): https://codereview.appspot.com/10711045/diff/10001/src/com/google/caja/ses/re... src/com/google/caja/ses/repairES5.js:841: var minSaveLength = (isFinite(global.length) ? global.length : 0) + 2; On 2013/07/04 01:04:03, MarkM wrote: > isFinite('') === true > isFinite('1') === true > > isFinite does a ToNumber on its argument. I think you should guard it with a > typeof x === 'number'. Done.
Sign in to reply to this message.
|
