Check for integer overflow when validating API function arguments.

Check for integer overflow when validating API function arguments.

BUG=65874
TEST=Added API test.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=86343

[asargent_no_longer_on_chrome, 2011-05-19 18:36:48]

+kasperl to double-check the javascript part, as I don't know off the top of my
head if future large integer support might break the way you've coded the check.

http://codereview.chromium.org/7042021/diff/1/chrome/browser/extensions/windo...
File chrome/browser/extensions/window_open_apitest.cc (right):

http://codereview.chromium.org/7042021/diff/1/chrome/browser/extensions/windo...
chrome/browser/extensions/window_open_apitest.cc:104:
ASSERT_TRUE(RunExtensionTest("window_open/argument_overflow")) << message_;
nit: is this test at all related to the other tests in the window_open
directory? If not, maybe move the argument_overflow directory to be at the same
level as window_open (eg in chrome/test/data/extensions/api_test/) and move this
block of code into something like
chrome/browser/extensions/validation_apitest.cc.

http://codereview.chromium.org/7042021/diff/1/chrome/renderer/resources/json_...
File chrome/renderer/resources/json_schema.js (right):

http://codereview.chromium.org/7042021/diff/1/chrome/renderer/resources/json_...
chrome/renderer/resources/json_schema.js:415: if (schema.type == "integer" &&
(instance | 0) != instance)
Kasper - can you confirm that this will continue to be the correct way to test
for large integers (outside signed 32bit range) for any future v8 modifications?

[Erik does not do reviews, 2011-05-19 18:42:28]

+arv

Given timezone differences, arv may be a quicker answer to your JS question.

Erik


On Thu, May 19, 2011 at 11:36 AM, <asargent@chromium.org> wrote:

> +kasperl to double-check the javascript part, as I don't know off the top
> of my
> head if future large integer support might break the way you've coded the
> check.
>
>
>
>
>
>
http://codereview.chromium.org/7042021/diff/1/chrome/browser/extensions/windo...
> File chrome/browser/extensions/window_open_apitest.cc (right):
>
>
>
http://codereview.chromium.org/7042021/diff/1/chrome/browser/extensions/windo...
> chrome/browser/extensions/window_open_apitest.cc:104:
>
> ASSERT_TRUE(RunExtensionTest("window_open/argument_overflow")) <<
> message_;
> nit: is this test at all related to the other tests in the window_open
> directory? If not, maybe move the argument_overflow directory to be at
> the same level as window_open (eg in
> chrome/test/data/extensions/api_test/) and move this block of code into
> something like chrome/browser/extensions/validation_apitest.cc.
>
>
>
http://codereview.chromium.org/7042021/diff/1/chrome/renderer/resources/json_...
> File chrome/renderer/resources/json_schema.js (right):
>
>
>
http://codereview.chromium.org/7042021/diff/1/chrome/renderer/resources/json_...
> chrome/renderer/resources/json_schema.js:415: if (schema.type ==
> "integer" && (instance | 0) != instance)
> Kasper - can you confirm that this will continue to be the correct way
> to test for large integers (outside signed 32bit range) for any future
> v8 modifications?
>
>
> http://codereview.chromium.org/7042021/
>

[Ben Olmstead, 2011-05-19 20:34:18]

http://codereview.chromium.org/7042021/diff/1/chrome/browser/extensions/windo...
File chrome/browser/extensions/window_open_apitest.cc (right):

http://codereview.chromium.org/7042021/diff/1/chrome/browser/extensions/windo...
chrome/browser/extensions/window_open_apitest.cc:104:
ASSERT_TRUE(RunExtensionTest("window_open/argument_overflow")) << message_;
On 2011/05/19 18:36:48, Antony Sargent wrote:
> nit: is this test at all related to the other tests in the window_open
> directory? If not, maybe move the argument_overflow directory to be at the
same
> level as window_open (eg in chrome/test/data/extensions/api_test/) and move
this
> block of code into something like
> chrome/browser/extensions/validation_apitest.cc.

It's debatable.  The bug explicitly calls out windows.create(), but almost
certainly affects any API that takes integer arguments.

I found out about chrome/test/data/extensions/json_schema_test.js, so I will add
a more general test there.

http://codereview.chromium.org/7042021/diff/1/chrome/renderer/resources/json_...
File chrome/renderer/resources/json_schema.js (right):

http://codereview.chromium.org/7042021/diff/1/chrome/renderer/resources/json_...
chrome/renderer/resources/json_schema.js:415: if (schema.type == "integer" &&
(instance | 0) != instance)
On 2011/05/19 18:36:48, Antony Sargent wrote:
> Kasper - can you confirm that this will continue to be the correct way to test
> for large integers (outside signed 32bit range) for any future v8
modifications?

Truncation to 32 bits is actually specified by ECMAScript:

http://www.ecma-international.org/publications/files/ECMA-ST/ECMA-262.pdf

Relevant sections (in at least 2nd through 5th Editions) are 9.5 (ToInt32) and
11.10 (Binary Bitwise Operators).  I can add this as a comment.

[arv (Not doing code reviews), 2011-05-19 21:10:04]

LGTM

http://codereview.chromium.org/7042021/diff/1/chrome/renderer/resources/json_...
File chrome/renderer/resources/json_schema.js (right):

http://codereview.chromium.org/7042021/diff/1/chrome/renderer/resources/json_...
chrome/renderer/resources/json_schema.js:415: if (schema.type == "integer" &&
(instance | 0) != instance)
On 2011/05/19 18:36:48, Antony Sargent wrote:
> Kasper - can you confirm that this will continue to be the correct way to test
> for large integers (outside signed 32bit range) for any future v8
modifications?

Bitwise or is a good way to do ToInt32. You should use !== here though

[Ben Olmstead, 2011-05-19 23:07:57]

Added json_schema_test and switched to !==.

[asargent_no_longer_on_chrome, 2011-05-19 23:21:28]

LGTM

[commit-bot: I haz the power, 2011-05-19 23:37:37]

Presubmit check for 7042021-3004 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Run the command: svn pset svn:eol-style LF \
 
chrome/test/data/extensions/api_test/window_open/argument_overflow/manifest.json

Was the presubmit check useful? Please send feedback & hate mail to
maruel@chromium.org!

[commit-bot: I haz the power, 2011-05-19 23:42:58]

Presubmit check for 7042021-4002 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Run the command: svn pset svn:eol-style LF \
 
chrome/test/data/extensions/api_test/window_open/argument_overflow/manifest.json