Bug 238134 – Validate readpixels format and type inside of WebCore

• Component: WebGL
• Status: ASSIGNED

[John Cunningham]

Validate readpixels against UNSIGNED_INT_24_8 type and DEPTH_COMPONENT format

[John Cunningham]

Created attachment 455221 [details]
Patch

[John Cunningham]

webgl/2.0.y/conformance/reading/read-pixels-test.html now passes in entirety.

[EWS Watchlist]

Note that there are important steps to take when updating ANGLE. See https://trac.webkit.org/wiki/UpdatingANGLE

[John Cunningham]

Created attachment 455308 [details]
Patch

[John Cunningham]

Created attachment 455310 [details]
Patch

[Myles C. Maxfield]

Comment on attachment 455310 [details]
Patch

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

> Source/WebCore/ChangeLog:3
> +        Validate readpixels format and type inside of WebCore

Silly question: Why doesn't ANGLE validate this for us?

[John Cunningham]

Created attachment 455380 [details]
Patch

[Kenneth Russell]

Comment on attachment 455380 [details]
Patch

Just FYI: ANGLE does do this validation. The only validation that should be needed at the WebCore level is to ensure that the incoming typed array type is compatible with the format being requested in readPixels. See Chromium's associated validation:

https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/modules/webgl/webgl_rendering_context_base.cc;l=4568
https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/modules/webgl/webgl2_rendering_context_base.cc;l=5608

[John Cunningham]

(In reply to Kenneth Russell from comment #8)
> Comment on attachment 455380 [details]
> Patch
> 
> Just FYI: ANGLE does do this validation. The only validation that should be
> needed at the WebCore level is to ensure that the incoming typed array type
> is compatible with the format being requested in readPixels. See Chromium's
> associated validation:
> 
> https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/
> renderer/modules/webgl/webgl_rendering_context_base.cc;l=4568
> https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/
> renderer/modules/webgl/webgl2_rendering_context_base.cc;l=5608

Thanks, Ken. My original patch here was on ANGLE. It seems there are two things going on:

1. ANGLE's readpixel validation allows UNSIGNED_INT_24_8, which does not seem to be accurate to my spec reading.

2. ANGLE's readpixel validation early outs for read-pixels-test.html because of a missing attachment which is undefined behavior, see these lines in validationES.cpp

if (readBuffer == nullptr)
{
    context->validationError(entryPoint, GL_INVALID_OPERATION, kMissingReadAttachment);
    return false;
}

I'm happy to fix the first in upstream ANGLE, and fix the undefined behavior in the test which is causing it to fail. I wonder if it still makes sense to swap the order of the undefined behavior check of missing attachment to after the pixel/type validation in ANGLE.

[Kenneth Russell]

(In reply to John Cunningham from comment #9)
> (In reply to Kenneth Russell from comment #8)
> > Comment on attachment 455380 [details]
> > Patch
> > 
> > Just FYI: ANGLE does do this validation. The only validation that should be
> > needed at the WebCore level is to ensure that the incoming typed array type
> > is compatible with the format being requested in readPixels. See Chromium's
> > associated validation:
> > 
> > https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/
> > renderer/modules/webgl/webgl_rendering_context_base.cc;l=4568
> > https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/
> > renderer/modules/webgl/webgl2_rendering_context_base.cc;l=5608
> 
> Thanks, Ken. My original patch here was on ANGLE. It seems there are two
> things going on:
> 
> 1. ANGLE's readpixel validation allows UNSIGNED_INT_24_8, which does not
> seem to be accurate to my spec reading.
> 
> 2. ANGLE's readpixel validation early outs for read-pixels-test.html because
> of a missing attachment which is undefined behavior, see these lines in
> validationES.cpp
> 
> if (readBuffer == nullptr)
> {
>     context->validationError(entryPoint, GL_INVALID_OPERATION,
> kMissingReadAttachment);
>     return false;
> }
> 
> I'm happy to fix the first in upstream ANGLE, and fix the undefined behavior
> in the test which is causing it to fail. I wonder if it still makes sense to
> swap the order of the undefined behavior check of missing attachment to
> after the pixel/type validation in ANGLE.

Thanks for the confirmation; I see.

For (1), it sounds fine to look into fixing this in ANGLE; but the bug is in Source/WebCore/html/canvas/WebGLRenderingContextBase.cpp, WebGLRenderingContextBase::validateArrayBufferType. The case for GraphicsContextGL::UNSIGNED_INT_24_8 should just be removed; then that part of the test will pass.

Not sure about the second bug though. Happy to discuss further.

[Kenneth Russell]

After further discussion on Slack, I see that the existing validateArrayBufferType is used for both Tex{Sub}Image and ReadPixels operations.

Still - would you consider passing an additional parameter to validateArrayBufferType, whether it's for ReadPixels or not? Then it could reject UNSIGNED_INT_24_8 for ReadPixels and avoid duplicating this code.

[Radar WebKit Bug Importer]

<rdar://problem/90910118>

[Kimmo Kinnunen]

Landed in ANGLE upstream:
https://bugs.chromium.org/p/angleproject/issues/detail?id=7119

[Kenneth Russell]

Should roll into WebKit in Bug 239114.

[Ahmad Saleem]

I think this bug can be closed (if scope is just update of ANGLE library) since it was within ANGLE library and it was updated as mentioned in Comment 13. But, from Comment 10 to remove a case bit within "Source/WebCore/html/canvas/WebGLRenderingContextBase.cpp", I noticed from Webkit Github mirror that it is still present on line 5504.

Webkit Mirror file link - https://github.com/WebKit/WebKit/blob/2e84b9750f8c04ed405b84e8fbfb2dbc0bf24497/Source/WebCore/html/canvas/WebGLRenderingContextBase.cpp

Thanks!