Bug 122610 - Validate uniform input when setting texture sampler uniforms
Summary: Validate uniform input when setting texture sampler uniforms
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebGL (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Roger Fong
URL:
Keywords:
Depends on: 122607
Blocks: 126775
  Show dependency treegraph
 
Reported: 2013-10-10 11:46 PDT by Roger Fong
Modified: 2014-01-10 14:40 PST (History)
6 users (show)

See Also:


Attachments
Patch (2.87 KB, patch)
2013-10-10 13:33 PDT, Roger Fong
dino: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Roger Fong 2013-10-10 11:46:00 PDT
We have to make sure when we set a texture sampler that the value is less than the max combined texture units, otherwise we throw an INVALID_VALUE error.

The conformance test, conformance/uniforms/uniform-samplers-test.html, covers this.
Comment 1 Roger Fong 2013-10-10 11:46:47 PDT
Fix relies on https://bugs.webkit.org/show_bug.cgi?id=122607 to be committed first.
Comment 2 Roger Fong 2013-10-10 13:33:38 PDT
Created attachment 213923 [details]
Patch
Comment 3 Dean Jackson 2013-10-10 15:10:40 PDT
Comment on attachment 213923 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=213923&action=review

> Source/WebCore/html/canvas/WebGLRenderingContext.cpp:4170
> +        for (unsigned ii = 0; ii < v->length(); ++ii) {

Is there a reason for ii vs i here?

> Source/WebCore/html/canvas/WebGLRenderingContext.cpp:4171
> +            if (((GC3Dint*)v)[ii] >= (int)m_textureUnits.size()) {

Might as well use static_cast

> Source/WebCore/html/canvas/WebGLRenderingContext.cpp:4189
> +    if (location->type() == GraphicsContext3D::SAMPLER_2D || location->type() == GraphicsContext3D::SAMPLER_CUBE)
> +        for (unsigned ii = 0; ii < (unsigned)size; ++ii) {
> +            if (((GC3Dint*)v)[ii] >= (int)m_textureUnits.size()) {

Same.
Comment 4 Roger Fong 2013-10-10 15:19:23 PDT
(In reply to comment #3)
> (From update of attachment 213923 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=213923&action=review
> 
> > Source/WebCore/html/canvas/WebGLRenderingContext.cpp:4170
> > +        for (unsigned ii = 0; ii < v->length(); ++ii) {
> 
> Is there a reason for ii vs i here?
Eh, just saw it in a number of places in the file. There are also plenty of just i's though. I can change.

> 
> > Source/WebCore/html/canvas/WebGLRenderingContext.cpp:4171
> > +            if (((GC3Dint*)v)[ii] >= (int)m_textureUnits.size()) {
> 
> Might as well use static_cast
> 
> > Source/WebCore/html/canvas/WebGLRenderingContext.cpp:4189
> > +    if (location->type() == GraphicsContext3D::SAMPLER_2D || location->type() == GraphicsContext3D::SAMPLER_CUBE)
> > +        for (unsigned ii = 0; ii < (unsigned)size; ++ii) {
> > +            if (((GC3Dint*)v)[ii] >= (int)m_textureUnits.size()) {
> 
> Same.

KK
Comment 5 Roger Fong 2013-10-10 16:33:54 PDT
Committed with fixes: http://trac.webkit.org/changeset/157271