Bug 41473 – Fix issues in boundary situations for WebGLRenderingContext::drawArrays/drawElements

• Component: WebGL
• Status: RESOLVED FIXED

[Zhenyao Mo]

in "first + count"

[Zhenyao Mo]

1) in drawArrays, possible overflow for (first + count)
2) drawArrays(first, count=0) where first is out-of-range should generate INVALID_OPERATION.
3) drawElements(count=0, offset=0) with buffer of size 0 should not generate an error.

[Zhenyao Mo]

Created attachment 60295 [details]
patch

CheckedInt.h is imported from mozilla as recommended by kbr to deal with integer overflow situation.  Modification is made to the file to remove further dependencies.

[WebKit Review Bot]

Attachment 60295 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
Last 3072 characters of output:
 use underscores in your identifier names.  [readability/naming] [4]
WebCore/html/canvas/CheckedInt.h:214:  twice_bigger_type_is_supported is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/html/canvas/CheckedInt.h:219:  This { should be at the end of the previous line  [whitespace/braces] [4]
WebCore/html/canvas/CheckedInt.h:230:  This { should be at the end of the previous line  [whitespace/braces] [4]
WebCore/html/canvas/CheckedInt.h:241:  This { should be at the end of the previous line  [whitespace/braces] [4]
WebCore/html/canvas/CheckedInt.h:247:  More than one command on the same line in if  [whitespace/parens] [4]
WebCore/html/canvas/CheckedInt.h:247:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
WebCore/html/canvas/CheckedInt.h:250:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
WebCore/html/canvas/CheckedInt.h:255:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
WebCore/html/canvas/CheckedInt.h:265:  This { should be at the end of the previous line  [whitespace/braces] [4]
WebCore/html/canvas/CheckedInt.h:269:  More than one command on the same line in if  [whitespace/parens] [4]
WebCore/html/canvas/CheckedInt.h:269:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
WebCore/html/canvas/CheckedInt.h:283:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
WebCore/html/canvas/CheckedInt.h:285:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
WebCore/html/canvas/CheckedInt.h:340:  This { should be at the end of the previous line  [whitespace/braces] [4]
WebCore/html/canvas/CheckedInt.h:409:  CheckedInt_internal::is_sub_valid is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/html/canvas/CheckedInt.h:468:  This { should be at the end of the previous line  [whitespace/braces] [4]
WebCore/html/canvas/CheckedInt.h:469:  return_type is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/html/canvas/CheckedInt.h:475:  This { should be at the end of the previous line  [whitespace/braces] [4]
WebCore/html/canvas/CheckedInt.h:476:  return_type is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/html/canvas/CheckedInt.h:490:  COMPOUND_OP is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/html/canvas/CheckedInt.h:509:  Missing spaces around /  [whitespace/operators] [3]
Total errors found: 39 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Kenneth Russell]

Comment on attachment 60295 [details]
patch

Looks good. I think we need Darin's feedback on whether it's okay to incorporate CheckedInt verbatim. In order to be able to take future updates from Mozilla I really think we should not force the code into WebKit style. One comment.


WebCore/html/canvas/WebGLRenderingContext.cpp:859
 +      if (!(checkedFirst + checkedCount).valid() || !validateRenderingState(first + count)) {
How about

mozilla::CheckedInt checkedSum = checkedFirst + checkedCount;
if (!checkedSum.valid() || !validateRenderingState(checkedSum.value()) {

[Darin Fisher (:fishd, Google)]

WebKit has several other source files imported from Mozilla.  I don't know what the process is for adding more.

Preserving the original formatting and style of the code makes sense to me, but we might want to make the interface more WebKit friendly.  For example, instead of the mozilla namespace, perhaps we'd want to just have this code live in the WebCore namespace.

That way instead of writing:

  mozilla::CheckedInt<int32_t> checkedCount(count);

We could just write:

  CheckedInt<int32_t> checkedCount(count);

I doubt the cost of renaming the namespace would cause that much of a merge headache, especially considering the other changes that were needed to support the removal of prtypes.h.

[Kenneth Russell]

(In reply to comment #5)
> Preserving the original formatting and style of the code makes sense to me, but we might want to make the interface more WebKit friendly.  For example, instead of the mozilla namespace, perhaps we'd want to just have this code live in the WebCore namespace.

Agree with renaming the namespace -- usage is tedious otherwise.

BTW, the only reason we placed this class here was to keep it close to its only current uses. We plan to use it more in the WebGL implementation. Open to suggestions of where to incorporate it in the tree. (Hoping there aren't any issues doing so -- Mozilla encouraged us to integrate it.)

[Zhenyao Mo]

Created attachment 60316 [details]
revised patch

[WebKit Review Bot]

Attachment 60316 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
Last 3072 characters of output:
 use underscores in your identifier names.  [readability/naming] [4]
WebCore/html/canvas/CheckedInt.h:215:  twice_bigger_type_is_supported is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/html/canvas/CheckedInt.h:220:  This { should be at the end of the previous line  [whitespace/braces] [4]
WebCore/html/canvas/CheckedInt.h:231:  This { should be at the end of the previous line  [whitespace/braces] [4]
WebCore/html/canvas/CheckedInt.h:242:  This { should be at the end of the previous line  [whitespace/braces] [4]
WebCore/html/canvas/CheckedInt.h:248:  More than one command on the same line in if  [whitespace/parens] [4]
WebCore/html/canvas/CheckedInt.h:248:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
WebCore/html/canvas/CheckedInt.h:251:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
WebCore/html/canvas/CheckedInt.h:256:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
WebCore/html/canvas/CheckedInt.h:266:  This { should be at the end of the previous line  [whitespace/braces] [4]
WebCore/html/canvas/CheckedInt.h:270:  More than one command on the same line in if  [whitespace/parens] [4]
WebCore/html/canvas/CheckedInt.h:270:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
WebCore/html/canvas/CheckedInt.h:284:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
WebCore/html/canvas/CheckedInt.h:286:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
WebCore/html/canvas/CheckedInt.h:341:  This { should be at the end of the previous line  [whitespace/braces] [4]
WebCore/html/canvas/CheckedInt.h:410:  CheckedInt_internal::is_sub_valid is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/html/canvas/CheckedInt.h:469:  This { should be at the end of the previous line  [whitespace/braces] [4]
WebCore/html/canvas/CheckedInt.h:470:  return_type is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/html/canvas/CheckedInt.h:476:  This { should be at the end of the previous line  [whitespace/braces] [4]
WebCore/html/canvas/CheckedInt.h:477:  return_type is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/html/canvas/CheckedInt.h:491:  COMPOUND_OP is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/html/canvas/CheckedInt.h:510:  Missing spaces around /  [whitespace/operators] [3]
Total errors found: 39 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Kenneth Russell]

Comment on attachment 60316 [details]
revised patch

Looks good to me.

[Darin Fisher (:fishd, Google)]

Comment on attachment 60316 [details]
revised patch

WebCore/html/canvas/CheckedInt.h:44
 +  #ifndef mozilla_CheckedInt_h
one more nit: let's rename this include guard to be CheckedInt_h
to match WebKit style.  it seems valuable to follow this rule
since it is related to the naming conventions for files.

otherwise, r=me

[Zhenyao Mo]

Created attachment 60373 [details]
revised patch: changed define to CheckedInt_h

[Darin Fisher (:fishd, Google)]

Comment on attachment 60373 [details]
revised patch: changed define to CheckedInt_h

thanks, r=me

[WebKit Commit Bot]

Comment on attachment 60373 [details]
revised patch: changed define to CheckedInt_h

Rejecting patch 60373 from commit-queue.

Failed to run "[u'/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/svn-apply', u'--reviewer', u'Darin Fisher', u'--force']" exit_code: 1
Last 500 characters of output:
 (offset 18 lines).
Hunk #3 succeeded at 871 (offset 18 lines).
Hunk #4 succeeded at 915 (offset 26 lines).
patching file LayoutTests/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file LayoutTests/fast/canvas/webgl/draw-arrays-out-of-bounds-expected.txt
patching file LayoutTests/fast/canvas/webgl/draw-arrays-out-of-bounds.html
patching file LayoutTests/fast/canvas/webgl/draw-elements-out-of-bounds-expected.txt
patching file LayoutTests/fast/canvas/webgl/draw-elements-out-of-bounds.html

Full output: http://webkit-commit-queue.appspot.com/results/3410090

[Zhenyao Mo]

Created attachment 60382 [details]
revised patch: resolve a svn merge conflict

[WebKit Review Bot]

Attachment 60382 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
Last 3072 characters of output:
 use underscores in your identifier names.  [readability/naming] [4]
WebCore/html/canvas/CheckedInt.h:216:  twice_bigger_type_is_supported is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/html/canvas/CheckedInt.h:221:  This { should be at the end of the previous line  [whitespace/braces] [4]
WebCore/html/canvas/CheckedInt.h:232:  This { should be at the end of the previous line  [whitespace/braces] [4]
WebCore/html/canvas/CheckedInt.h:243:  This { should be at the end of the previous line  [whitespace/braces] [4]
WebCore/html/canvas/CheckedInt.h:249:  More than one command on the same line in if  [whitespace/parens] [4]
WebCore/html/canvas/CheckedInt.h:249:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
WebCore/html/canvas/CheckedInt.h:252:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
WebCore/html/canvas/CheckedInt.h:257:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
WebCore/html/canvas/CheckedInt.h:267:  This { should be at the end of the previous line  [whitespace/braces] [4]
WebCore/html/canvas/CheckedInt.h:271:  More than one command on the same line in if  [whitespace/parens] [4]
WebCore/html/canvas/CheckedInt.h:271:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
WebCore/html/canvas/CheckedInt.h:285:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
WebCore/html/canvas/CheckedInt.h:287:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
WebCore/html/canvas/CheckedInt.h:342:  This { should be at the end of the previous line  [whitespace/braces] [4]
WebCore/html/canvas/CheckedInt.h:411:  CheckedInt_internal::is_sub_valid is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/html/canvas/CheckedInt.h:470:  This { should be at the end of the previous line  [whitespace/braces] [4]
WebCore/html/canvas/CheckedInt.h:471:  return_type is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/html/canvas/CheckedInt.h:477:  This { should be at the end of the previous line  [whitespace/braces] [4]
WebCore/html/canvas/CheckedInt.h:478:  return_type is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/html/canvas/CheckedInt.h:492:  COMPOUND_OP is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/html/canvas/CheckedInt.h:511:  Missing spaces around /  [whitespace/operators] [3]
Total errors found: 38 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Dimitri Glazkov (Google)]

Comment on attachment 60382 [details]
revised patch: resolve a svn merge conflict

once more.

[WebKit Commit Bot]

Comment on attachment 60382 [details]
revised patch: resolve a svn merge conflict

Clearing flags on attachment: 60382

Committed r62397: <http://trac.webkit.org/changeset/62397>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Benoit Jacob]

Hi, I wanted to let you know that we also have an extensive unit test for CheckedInt, in case you might want to import that too:

http://hg.mozilla.org/mozilla-central/file/tip/xpcom/tests/TestCheckedInt.cpp