Bug 249633 - WebGLRenderingContextBase::copyTexImage2D does not handle "GCGLint level" parameter
Summary: WebGLRenderingContextBase::copyTexImage2D does not handle "GCGLint level" par...
Status: RESOLVED CONFIGURATION CHANGED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebGL (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Critical
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2022-12-19 22:07 PST by byao
Modified: 2022-12-22 03:57 PST (History)
4 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description byao 2022-12-19 22:07:37 PST
'level' parameter can not be minus value which has not been handled in WebGLRenderingContextBase::validateTexFuncParameters function.
This easily causes WPEWebProcess process crash!
Comment 1 Kimmo Kinnunen 2022-12-20 09:08:45 PST
Thank you for the report.
Would you have a html+JS reproduction of the problem?
In the trunk code, he intention is that this is handled at ANGLE level, at:
  bool ValidMipLevel(const Context *context, TextureType type, GLint level)
Comment 2 byao 2022-12-20 17:33:18 PST
Yes, I run below codes:
PASS: Bad target (in assertFail)(function(){ 
  gl.copyTexImage2D(gl.FLOAT, 0, gl.RGBA, 0,0, 16,16,0); 
})
PASS: Bad internal format (in assertFail)(function(){ 
  gl.copyTexImage2D(gl.TEXTURE_2D, 0, gl.FLOAT, 0,0, 16,16,0); 
})
Fail: Negative level (in assertFail)(function(){ 
  gl.copyTexImage2D(gl.TEXTURE_2D, -1, gl.RGBA, 0,0, 16,16,0); 
})
The last one causes the crash.
I don't know ValidMipLevel until you told me actually.
Comment 3 Kimmo Kinnunen 2022-12-21 04:47:32 PST
If you have a standalone runnable .html file to try, I can try it.
Additionally, if possible a  backtrace of the crash is always helpful.
Also useful information would be to know if the version of WebKit you are using is close to current trunk or older?
Comment 5 Kimmo Kinnunen 2022-12-22 00:29:35 PST
That page passes for current trunk at least on Mac.
We've recently, within last 6 months, removed legacy WebGL backend implementation which might not have the validation. 

In case you are testing an older WPE build, it might be triggering the old code path. A backtrace of the crash would help clarify that.
Comment 6 byao 2022-12-22 00:49:29 PST
We are really using the older version webkit, but I also checked the latest version webkit it has the same codes which I pasted below.

void WebGLRenderingContextBase::copyTexImage2D(GCGLenum target, GCGLint level, GCGLenum internalFormat, GCGLint x, GCGLint y, GCGLsizei width, GCGLsizei height, GCGLint border)
{
...
// FIXME: if the framebuffer is not complete, none of the below should be executed.
    tex->setLevelInfo(target, level, internalFormat, width, height, GraphicsContextGL::UNSIGNED_BYTE);
...
}
void WebGLTexture::setLevelInfo(GCGLenum target, GCGLint level, GCGLenum internalFormat, GCGLsizei width, GCGLsizei height, GCGLenum type)
{
...
    m_info[index][level].setInfo(internalFormat, width, height, type);
...
}
It possibly uses minus level as the array index then cause the crash.
Hopefully it is helpful to you!
Comment 7 byao 2022-12-22 01:30:49 PST
I just patched below codes in bool WebGLRenderingContextBase::validateTexFuncParameters

  6          synthesizeGLError(GraphicsContextGL::INVALID_VALUE, functionName, "width or height <     0");
  7          return false;
  8      }
  9 +
 10 +    if (level < 0) {
 11 +        synthesizeGLError(GraphicsContextGL::INVALID_VALUE, functionName, "level < 0");
 12 +        return false;
 13 +    }
Comment 8 Kimmo Kinnunen 2022-12-22 03:57:21 PST
> I also checked the latest version webkit it has the same codes which I pasted below.
> void WebGLTexture::setLevelInfo(GCGLenum target, GCGLint level, GCGLenum internalFormat, GCGLsizei width, GCGLsizei height, GCGLenum type)

Thanks!
The code has been removed from WebKit around 3 months ago.

https://github.com/WebKit/WebKit/blob/main/Source/WebCore/html/canvas/WebGLTexture.cpp