Closed Bug 896601 Opened 7 years ago Closed 7 years ago

WebGL draw commands overflow

Categories

(Core :: Canvas: WebGL, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla25

People

(Reporter: guillaume.abadie, Assigned: guillaume.abadie)

References

Details

Attachments

(1 file, 5 obsolete files)

GLsizei should be checked with CheckedInt32 and not CheckedUint32
Assignee: nobody → gabadie
Blocks: 892546
Attached patch patch revision 1 (obsolete) — Splinter Review
Any regressions detected on WebGL conformance test 1.0.2.
Attachment #779379 - Flags: review?(bjacob)
Comment on attachment 779379 [details] [diff] [review]
patch revision 1

Review of attachment 779379 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/canvas/src/WebGLContextValidate.cpp
@@ +94,5 @@
>   * that will be legal to be read from bound VBOs.
>   */
>  
>  bool
> +WebGLContext::ValidateBuffers(int32_t *maxAllowedCount, const char *info)

Since the reason why we are doing int32_t is that GLsizei is the same as int32_t, why not use GLsizei here?

You can perfectly use CheckedInt<GLsizei>

You can get the max value with

  std::numeric<GLsizei>::max()
Attachment #779379 - Flags: review?(bjacob) → review-
Attached patch patch revision 2 (obsolete) — Splinter Review
Attachment #779379 - Attachment is obsolete: true
Attachment #779978 - Flags: review?(bjacob)
Comment on attachment 779978 [details] [diff] [review]
patch revision 2

Review of attachment 779978 [details] [diff] [review]:
-----------------------------------------------------------------

Please use GLsizei, not int32_t, not WebGLsizei.

::: content/canvas/src/WebGLContext.h
@@ +907,5 @@
>  
>      nsTArray<WebGLenum> mCompressedTextureFormats;
>  
>      bool InitAndValidateGL();
> +    bool ValidateBuffers(int32_t *maxAllowedCount, const char *info);

GLsizei (see other comment).

::: content/canvas/src/WebGLContextGL.cpp
@@ +1369,5 @@
>      // Any checks below this depend on a program being available.
>      if (!mCurrentProgram)
>          return;
>  
> +    int32_t maxAllowedCount = 0;

GLsizei

@@ +1374,4 @@
>      if (!ValidateBuffers(&maxAllowedCount, "drawArrays"))
>          return;
>  
> +    CheckedInt<WebGLsizei> checked_firstPlusCount = CheckedInt<WebGLsizei>(first) + count;

GLsizei, not WebGLsizei (see other comment i made on WebGLContextValidate.cpp)

::: content/canvas/src/WebGLContextValidate.cpp
@@ +94,5 @@
>   * that will be legal to be read from bound VBOs.
>   */
>  
>  bool
> +WebGLContext::ValidateBuffers(int32_t *maxAllowedCount, const char *info)

maxAllowedCount should be a GLsizei*

@@ +111,5 @@
>          *maxAllowedCount = mMinInUseAttribArrayLength;
>          return true;
>      }
>  
> +    int32_t maxAllowed = INT32_MAX;

Should be GLsizei too, using std::numeric_limits as explained above.

@@ +148,5 @@
>          if (checked_byteLength.value() < checked_sizeOfLastElement.value()) {
>              maxAllowed = 0;
>              break;
>          } else {
> +            CheckedInt<WebGLsizei> checked_maxAllowedCount

Use GLsizei, not WebGLsizei please.

I'm not clear on where exactly it makes sense to use the WebGL types, but that would be only in parts of the code very close to the DOM API --- not so far deep down into the C++ logic.
Attachment #779978 - Flags: review?(bjacob) → review-
Attached patch patch revision 3 (obsolete) — Splinter Review
Sorry about that. This patch fix those and have any regression on WebGL conformance test 1.0.2
Attachment #779978 - Attachment is obsolete: true
Attachment #780047 - Flags: review?(bjacob)
Attached patch patch revision 4 (obsolete) — Splinter Review
New regression after regression on tpbl
https://tbpl.mozilla.org/?tree=Try&rev=7b7ddd2b7659
Attachment #780047 - Attachment is obsolete: true
Attachment #780047 - Flags: review?(bjacob)
Attachment #780542 - Flags: review?(bjacob)
Comment on attachment 780542 [details] [diff] [review]
patch revision 4

Review of attachment 780542 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/canvas/src/WebGLContextValidate.cpp
@@ +149,5 @@
>              maxAllowed = 0;
>              break;
>          } else {
> +            CheckedInt<WebGLsizei> checked_maxAllowedCount
> +                = (CheckedInt<WebGLsizei>(checked_byteLength.value() - checked_sizeOfLastElement.value()) / vd.actualStride()) + 1;

Catastrophe! The subtraction is performed on plain integers before the CheckedInt is constructed!
Attachment #780542 - Flags: review?(bjacob) → review-
Attached patch patch revision 5 (obsolete) — Splinter Review
After offline discussion, we went on the wrong way for this patch. Here is the new one.
https://tbpl.mozilla.org/?tree=Try&rev=286a518a507d
Attachment #780542 - Attachment is obsolete: true
Attachment #780595 - Flags: review?(bjacob)
Regression fix.
Attachment #780595 - Attachment is obsolete: true
Attachment #780595 - Flags: review?(bjacob)
Attachment #780792 - Flags: review?(bjacob)
Attachment #780792 - Flags: review?(bjacob) → review+
Thanks Benoit!
https://hg.mozilla.org/integration/mozilla-inbound/rev/5882b593646a
Target Milestone: --- → mozilla25
https://hg.mozilla.org/mozilla-central/rev/5882b593646a
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.