If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

WebGL draw commands overflow

RESOLVED FIXED in mozilla25

Status

()

Core
Canvas: WebGL
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: Guillaume Abadie, Assigned: Guillaume Abadie)

Tracking

unspecified
mozilla25
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 5 obsolete attachments)

(Assignee)

Description

4 years ago
GLsizei should be checked with CheckedInt32 and not CheckedUint32
(Assignee)

Updated

4 years ago
Assignee: nobody → gabadie
Blocks: 892546
(Assignee)

Comment 1

4 years ago
Created attachment 779379 [details] [diff] [review]
patch revision 1

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-
(Assignee)

Comment 3

4 years ago
Created attachment 779978 [details] [diff] [review]
patch revision 2
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-
(Assignee)

Comment 5

4 years ago
Created attachment 780047 [details] [diff] [review]
patch revision 3

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)
(Assignee)

Comment 6

4 years ago
https://tbpl.mozilla.org/?tree=Try&rev=2511443fb968
(Assignee)

Comment 7

4 years ago
Created attachment 780542 [details] [diff] [review]
patch revision 4

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-
(Assignee)

Comment 9

4 years ago
Created attachment 780595 [details] [diff] [review]
patch revision 5

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)
(Assignee)

Comment 10

4 years ago
Created attachment 780792 [details] [diff] [review]
patch for landing

Regression fix.
Attachment #780595 - Attachment is obsolete: true
Attachment #780595 - Flags: review?(bjacob)
Attachment #780792 - Flags: review?(bjacob)
Attachment #780792 - Flags: review?(bjacob) → review+
(Assignee)

Comment 11

4 years ago
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
Last Resolved: 4 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.