Bug 31892 – Index validation code validates too many vertex attributes

• Component: WebGL
• Status: RESOLVED FIXED

[Kenneth Russell]

The index validation code added in https://bugs.webkit.org/show_bug.cgi?id=31239 validates too many vertex attributes. It currently validates all enabled vertex attribute slots, but should only validate those consumed by the currently bound program.

[Zhenyao Mo]

Created attachment 52797 [details]
patch

[WebKit Review Bot]

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

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/html/canvas/WebGLProgram.cpp:53:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 1 in 7 files


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

[Zhenyao Mo]

Created attachment 52798 [details]
revised patch: fix a typo

[WebKit Review Bot]

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

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/html/canvas/WebGLProgram.cpp:53:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 1 in 7 files


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

[Zhenyao Mo]

Created attachment 52800 [details]
revised patch: fix a style issue

[Kenneth Russell]

Comment on attachment 52800 [details]
revised patch: fix a style issue

> Index: WebCore/html/canvas/WebGLProgram.cpp
> ===================================================================
> --- WebCore/html/canvas/WebGLProgram.cpp	(revision 57235)
> +++ WebCore/html/canvas/WebGLProgram.cpp	(working copy)
> @@ -48,6 +48,34 @@ void WebGLProgram::_deleteObject(Platfor
>      context()->graphicsContext3D()->deleteProgram(object);
>  }
>  
> +bool WebGLProgram::cacheActiveAttribLocations()
> +{
> +    if (!object())
> +        return false;
> +    GraphicsContext3D* context3d = context()->graphicsContext3D();
> +    int linkStatus;
> +    context3d->getProgramiv(this, GraphicsContext3D::LINK_STATUS, &linkStatus);
> +    if (!linkStatus)

Should call m_activeAttribLocations.clear() in this case.

> Index: WebCore/html/canvas/WebGLProgram.h
> ===================================================================
> --- WebCore/html/canvas/WebGLProgram.h	(revision 57235)
> +++ WebCore/html/canvas/WebGLProgram.h	(working copy)
> @@ -30,6 +30,7 @@
>  
>  #include <wtf/PassRefPtr.h>
>  #include <wtf/RefCounted.h>
> +#include <wtf/Vector.h>
>  
>  namespace WebCore {
>      
> @@ -38,11 +39,22 @@ namespace WebCore {
>          virtual ~WebGLProgram() { deleteObject(); }
>          
>          static PassRefPtr<WebGLProgram> create(WebGLRenderingContext*);
> +
> +        // cacheActiveAttribLocation() is only called once after linkProgram()
> +        // succeeds.
> +        bool cacheActiveAttribLocations();
> +        // doesAttribLocationBelongToProgram only returns true when
> +        // cacheActiveAttribLocations() already succeeded and attrib location
> +        // actually belongs to the program.
> +        bool doesAttribLocationBelongToProgram(unsigned location);

Instead of doesAttribLocationBelongToProgram, please add two APIs, int numActiveAttribLocations() and int getActiveAttribLocation(int).

> Index: WebCore/html/canvas/WebGLRenderingContext.cpp
> ===================================================================
> --- WebCore/html/canvas/WebGLRenderingContext.cpp	(revision 57235)
> +++ WebCore/html/canvas/WebGLRenderingContext.cpp	(working copy)
> @@ -688,11 +688,14 @@ bool WebGLRenderingContext::validateInde
>  
>  bool WebGLRenderingContext::validateRenderingState(long numElementsRequired)
>  {
> +    if (!m_currentProgram)
> +        return false;
> +
>      // Look in each enabled vertex attrib and find the smallest buffer size
>      long smallestNumElements = LONG_MAX;
>      for (unsigned i = 0; i < m_vertexAttribState.size(); ++i) {
>          const VertexAttribState& state = m_vertexAttribState[i];
> -        if (state.enabled && state.numElements < smallestNumElements)
> +        if (state.enabled && m_currentProgram->doesAttribLocationBelongToProgram(i) && state.numElements < smallestNumElements)
>              smallestNumElements = state.numElements;
>      }

Please rewrite this loop to iterate from 0..m_currentProgram->numActiveAttribLocations(). Even though the number of active attributes is likely to be small, this loop is O(n*m) where it should be O(m).

> Index: LayoutTests/ChangeLog
> ===================================================================
> --- LayoutTests/ChangeLog	(revision 57240)
> +++ LayoutTests/ChangeLog	(working copy)
> @@ -1,3 +1,13 @@
> [phone]  Zhenyao Mo  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Index validation code validates too many vertex attributes
> +        https://bugs.webkit.org/show_bug.cgi?id=31892
> +
> +        * fast/canvas/webgl/index-validation-expected.txt: Add new test cases for index validations.
> +        * fast/canvas/webgl/script-tests/index-validation.js: Ditto.
> +
>  2010-04-07  Erik Arvidsson  <[e-mail]>
>  
>          Reviewed by Adam Barth.
> Index: LayoutTests/fast/canvas/webgl/index-validation-expected.txt
> ===================================================================
> --- LayoutTests/fast/canvas/webgl/index-validation-expected.txt	(revision 57235)
> +++ LayoutTests/fast/canvas/webgl/index-validation-expected.txt	(working copy)
> @@ -1,11 +1,30 @@
> -Test of get calls against GL objects like getBufferParameter, etc.
> +Test of validating indices for drawElements().
>  
>  On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
>  
> +Testing with valid indices
>  PASS gl.checkFramebufferStatus(gl.FRAMEBUFFER) is gl.FRAMEBUFFER_COMPLETE
>  PASS gl.getError() is 0
>  PASS gl.drawElements(gl.TRIANGLES, 3, gl.UNSIGNED_SHORT, 0) is undefined.
>  PASS gl.getError() is 0
> +Testing with out-of-range indices
> +Enable vertices, valid
> +PASS gl.getError() is 0
> +PASS gl.drawElements(gl.TRIANGLES, 3, gl.UNSIGNED_SHORT, 0) is undefined.
> +PASS gl.getError() is 0
> +Enable normals, out-of-range
> +PASS gl.getError() is 0
> +PASS gl.drawElements(gl.TRIANGLES, 3, gl.UNSIGNED_SHORT, 0) is undefined.
> +PASS gl.getError() is gl.INVALID_OPERATION
> +Test with enabled attribute that does not belong to current program
> +Enable an extra attribute with null
> +PASS gl.getError() is 0
> +PASS gl.drawElements(gl.TRIANGLES, 3, gl.UNSIGNED_SHORT, 0) is undefined.
> +PASS gl.getError() is 0
> +Enable an extra attribute with inefficient data buffer
> +PASS gl.getError() is 0
> +PASS gl.drawElements(gl.TRIANGLES, 3, gl.UNSIGNED_SHORT, 0) is undefined.
> +PASS gl.getError() is 0
>  PASS successfullyParsed is true
>  
>  TEST COMPLETE
> Index: LayoutTests/fast/canvas/webgl/script-tests/index-validation.js
> ===================================================================
> --- LayoutTests/fast/canvas/webgl/script-tests/index-validation.js	(revision 57235)
> +++ LayoutTests/fast/canvas/webgl/script-tests/index-validation.js	(working copy)
> @@ -1,20 +1,27 @@
> -description("Test of get calls against GL objects like getBufferParameter, etc.");
> +description("Test of validating indices for drawElements().");
>  
>  var gl = create3DContext();
>  var program = loadStandardProgram(gl);
>  
>  // 3 vertices => 1 triangle, interleaved data
> -var data = new WebGLFloatArray([0, 0, 0, 1,
> -                                0, 0, 1,
> -                                1, 0, 0, 1,
> -                                0, 0, 1,
> -                                1, 1, 1, 1,
> -                                0, 0, 1]);
> +var dataComplete = new WebGLFloatArray([0, 0, 0, 1,
> +                                        0, 0, 1,
> +                                        1, 0, 0, 1,
> +                                        0, 0, 1,
> +                                        1, 1, 1, 1,
> +                                        0, 0, 1]);
> +var dataIncomplete = new WebGLFloatArray([0, 0, 0, 1,
> +                                          0, 0, 1,
> +                                          1, 0, 0, 1,
> +                                          0, 0, 1,
> +                                          1, 1, 1, 1]);
>  var indices = new WebGLUnsignedShortArray([0, 1, 2]);
>  
> -var buffer = gl.createBuffer();
> -gl.bindBuffer(gl.ARRAY_BUFFER, buffer);
> -gl.bufferData(gl.ARRAY_BUFFER, data, gl.STATIC_DRAW);
> +debug("Testing with valid indices");
> +
> +var bufferComplete = gl.createBuffer();
> +gl.bindBuffer(gl.ARRAY_BUFFER, bufferComplete);
> +gl.bufferData(gl.ARRAY_BUFFER, dataComplete, gl.STATIC_DRAW);
>  var elements = gl.createBuffer();
>  gl.bindBuffer(gl.ELEMENT_ARRAY_BUFFER, elements);
>  gl.bufferData(gl.ELEMENT_ARRAY_BUFFER, indices, gl.STATIC_DRAW);
> @@ -23,11 +30,45 @@ var vertexLoc = gl.getAttribLocation(pro
>  var normalLoc = gl.getAttribLocation(program, "a_normal");
>  gl.vertexAttribPointer(vertexLoc, 4, gl.FLOAT, false, 7 * gl.sizeInBytes(gl.FLOAT), 0);
>  gl.enableVertexAttribArray(vertexLoc);
> -gl.vertexAttribPointer(normalLoc, 3, gl.FLOAT, false, 7 * gl.sizeInBytes(gl.FLOAT), 3 * gl.sizeInBytes(gl.FLOAT));
> +gl.vertexAttribPointer(normalLoc, 3, gl.FLOAT, false, 7 * gl.sizeInBytes(gl.FLOAT), 4 * gl.sizeInBytes(gl.FLOAT));
>  gl.enableVertexAttribArray(normalLoc);
>  shouldBe('gl.checkFramebufferStatus(gl.FRAMEBUFFER)', 'gl.FRAMEBUFFER_COMPLETE');
>  shouldBe('gl.getError()', '0');
>  shouldBeUndefined('gl.drawElements(gl.TRIANGLES, 3, gl.UNSIGNED_SHORT, 0)');
>  shouldBe('gl.getError()', '0');
>  
> +debug("Testing with out-of-range indices");
> +
> +var bufferIncomplete = gl.createBuffer();
> +gl.bindBuffer(gl.ARRAY_BUFFER, bufferIncomplete);
> +gl.bufferData(gl.ARRAY_BUFFER, dataIncomplete, gl.STATIC_DRAW);
> +gl.vertexAttribPointer(vertexLoc, 4, gl.FLOAT, false, 7 * gl.sizeInBytes(gl.FLOAT), 0);
> +gl.enableVertexAttribArray(vertexLoc);
> +gl.disableVertexAttribArray(normalLoc);
> +debug("Enable vertices, valid");
> +shouldBe('gl.getError()', '0');
> +shouldBeUndefined('gl.drawElements(gl.TRIANGLES, 3, gl.UNSIGNED_SHORT, 0)');
> +shouldBe('gl.getError()', '0');
> +debug("Enable normals, out-of-range");
> +gl.vertexAttribPointer(normalLoc, 3, gl.FLOAT, false, 7 * gl.sizeInBytes(gl.FLOAT), 4 * gl.sizeInBytes(gl.FLOAT));
> +gl.enableVertexAttribArray(normalLoc);
> +shouldBe('gl.getError()', '0');
> +shouldBeUndefined('gl.drawElements(gl.TRIANGLES, 3, gl.UNSIGNED_SHORT, 0)');
> +shouldBe('gl.getError()', 'gl.INVALID_OPERATION');
> +
> +debug("Test with enabled attribute that does not belong to current program");
> +
> +gl.disableVertexAttribArray(normalLoc);
> +var extraLoc = Math.max(vertexLoc, normalLoc) + 1;
> +gl.enableVertexAttribArray(extraLoc);
> +debug("Enable an extra attribute with null");
> +shouldBe('gl.getError()', '0');
> +shouldBeUndefined('gl.drawElements(gl.TRIANGLES, 3, gl.UNSIGNED_SHORT, 0)');
> +shouldBe('gl.getError()', '0');
> +debug("Enable an extra attribute with inefficient data buffer");

inefficient -> insufficient.

New tests look good otherwise. Thanks for fixing the bug in the preexisting test.

[Zhenyao Mo]

Created attachment 52830 [details]
revised patch : responding to Ken Russell's review

[Zhenyao Mo]

Created attachment 52831 [details]
revised patch: fix the change log

[Kenneth Russell]

Comment on attachment 52831 [details]
revised patch: fix the change log

Looks good. One comment.

> Index: WebCore/html/canvas/WebGLProgram.cpp
> ===================================================================
> --- WebCore/html/canvas/WebGLProgram.cpp	(revision 57240)
> +++ WebCore/html/canvas/WebGLProgram.cpp	(working copy)
> @@ -48,6 +48,41 @@ void WebGLProgram::_deleteObject(Platfor
>      context()->graphicsContext3D()->deleteProgram(object);
>  }
>  
> +bool WebGLProgram::cacheActiveAttribLocations()
> +{
> +    m_activeAttribLocations.clear();
> +    if (!object())
> +        return false;
> +    GraphicsContext3D* context3d = context()->graphicsContext3D();
> +    int linkStatus;
> +    context3d->getProgramiv(this, GraphicsContext3D::LINK_STATUS, &linkStatus);
> +    if (!linkStatus)
> +        return false;
> +
> +    int numAttribs = 0;
> +    context3d->getProgramiv(this, GraphicsContext3D::ACTIVE_ATTRIBUTES, &numAttribs);
> +    m_activeAttribLocations.resize(static_cast<size_t>(numAttribs));
> +    for (int i = 0; i < numAttribs; ++i) {
> +        ActiveInfo info;
> +        context3d->getActiveAttrib(this, i, info);
> +        m_activeAttribLocations[i] = context3d->getAttribLocation(this, info.name.charactersWithNullTermination());
> +    }
> +
> +    return true;
> +}
> +
> +int WebGLProgram::numActiveAttribLocations()
> +{
> +    return static_cast<int>(m_activeAttribLocations.size());
> +}
> +
> +int WebGLProgram::getActiveAttribLocation(int index)
> +{
> +    if (index < 0 || index >= static_cast<int>(m_activeAttribLocations.size()))

This would be better written as
  if (index < 0 || index >= numActiveAttribLocations())

[Zhenyao Mo]

Created attachment 52895 [details]
revised patch: minor modification as Ken Russell suggested

[Kenneth Russell]

Comment on attachment 52895 [details]
revised patch: minor modification as Ken Russell suggested

Looks good to me.

[Dimitri Glazkov (Google)]

Comment on attachment 52895 [details]
revised patch: minor modification as Ken Russell suggested

rs=me given kbr approves.

[WebKit Commit Bot]

Comment on attachment 52895 [details]
revised patch: minor modification as Ken Russell suggested

Clearing flags on attachment: 52895

Committed r57709: <http://trac.webkit.org/changeset/57709>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.