Bug 39704 – readPixels with negative width/height should generate INVALID_VALUE and return

• Status: RESOLVED
• Resolution: FIXED
• Component: WebGL

[Zhenyao Mo]

Currently readPixels will crash if width/height are negative.  Instead, INVALID_VALUE should be generated.

[Zhenyao Mo]

Also, with very large width/height and WebGL*Array creation failure, a INVALID_VALUE error will generate.

[Zhenyao Mo]

Created attachment 57063 [details]
patch

[David Levin]

What bug is this new line fixing?

 1707     memset(data, 0, totalBytes);

And does the layout test cover that? (If not, can it be added?)

[Zhenyao Mo]

(In reply to comment #3)
> What bug is this new line fixing?
> 
>  1707     memset(data, 0, totalBytes);
> 
> And does the layout test cover that? (If not, can it be added?)

This is not a bug fix.  For safety reason, we clear the buffer so users will not get access to content they should not.  According to GLES spec, for the un-covered portion of the memory, their content is un-defined.  I don't think we should write a definitive test against something undefined, because whatever the content is, it should not be "incorrect".

[David Levin]

(In reply to comment #4)
> (In reply to comment #3)
> > What bug is this new line fixing?
> > 
> >  1707     memset(data, 0, totalBytes);
> > 
> > And does the layout test cover that? (If not, can it be added?)
> 
> This is not a bug fix.  For safety reason, we clear the buffer so users will not get access to content they should not.  According to GLES spec, for the un-covered portion of the memory, their content is un-defined.  I don't think we should write a definitive test against something undefined, because whatever the content is, it should not be "incorrect".

Should it be set to something (like -10 for example) that is less likely to make users rely on undefined behavior?

0 tends to make things just work (that maybe shouldn't) and thus hide problems.

[Oliver Hunt]

(In reply to comment #4)
> (In reply to comment #3)
> > What bug is this new line fixing?
> > 
> >  1707     memset(data, 0, totalBytes);
> > 
> > And does the layout test cover that? (If not, can it be added?)
> 
> This is not a bug fix.  For safety reason, we clear the buffer so users will not get access to content they should not.  According to GLES spec, for the un-covered portion of the memory, their content is un-defined.  I don't think we should write a definitive test against something undefined, because whatever the content is, it should not be "incorrect".


There should be a test, otherwise there's nothing to verify that the data is safe -- if some other implementation decides to produces something other than a zero-filled buffer they can add platform specific expected results.

[Zhenyao Mo]

(In reply to comment #5)
> (In reply to comment #4)
> > (In reply to comment #3)
> > > What bug is this new line fixing?
> > > 
> > >  1707     memset(data, 0, totalBytes);
> > > 
> > > And does the layout test cover that? (If not, can it be added?)
> > 
> > This is not a bug fix.  For safety reason, we clear the buffer so users will not get access to content they should not.  According to GLES spec, for the un-covered portion of the memory, their content is un-defined.  I don't think we should write a definitive test against something undefined, because whatever the content is, it should not be "incorrect".
> 
> Should it be set to something (like -10 for example) that is less likely to make users rely on undefined behavior?
> 
> 0 tends to make things just work (that maybe shouldn't) and thus hide problems.

Good point.  I'll double check WebGL spec, if it doesn't have extra constraints about buffers initialized to 0, I'll use an odd number instead.

[Zhenyao Mo]

(In reply to comment #6)
> (In reply to comment #4)
> > (In reply to comment #3)
> > > What bug is this new line fixing?
> > > 
> > >  1707     memset(data, 0, totalBytes);
> > > 
> > > And does the layout test cover that? (If not, can it be added?)
> > 
> > This is not a bug fix.  For safety reason, we clear the buffer so users will not get access to content they should not.  According to GLES spec, for the un-covered portion of the memory, their content is un-defined.  I don't think we should write a definitive test against something undefined, because whatever the content is, it should not be "incorrect".
> 
> 
> There should be a test, otherwise there's nothing to verify that the data is safe -- if some other implementation decides to produces something other than a zero-filled buffer they can add platform specific expected results.

Ok, will add a case then.

[Zhenyao Mo]

ReadPixels signature will change.  This patch becomes invalid.

[Zhenyao Mo]

Due to https://bugs.webkit.org/show_bug.cgi?id=40322, negative width/height will no longer cause crash, but still, we need to deal with it correctly, i.e., generate INVALID_VALUE and return.

[Zhenyao Mo]

Created attachment 58418 [details]
patch

[Kenneth Russell]

Comment on attachment 58418 [details]
patch

WebCore/html/canvas/WebGLRenderingContext.cpp:1673
 +      }
Let's put this check right after the NULL check for the "pixels" argument tomorrow to try to follow the request for prioritization of error returns from Mozilla:
https://www.khronos.org/webgl/public-mailing-list/archives/1006/msg00040.html


LayoutTests/fast/canvas/webgl/read-pixels.html:184
 +          return;
Should presumably say:

} else if (!array.length) {
    shouldBe("gl.getError()", "gl.NO_ERROR");
    return;
}

[Zhenyao Mo]

Created attachment 58426 [details]
revised patch: responding to kbr's review

[Kenneth Russell]

Comment on attachment 58426 [details]
revised patch: responding to kbr's review

Looks good.

[Dimitri Glazkov (Google)]

Comment on attachment 58426 [details]
revised patch: responding to kbr's review

Make it so.

[WebKit Commit Bot]

Comment on attachment 58426 [details]
revised patch: responding to kbr's review

Clearing flags on attachment: 58426

Committed r61019: <http://trac.webkit.org/changeset/61019>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.