Bug 32391 – Don't allow default framebuffer to be mutated

• Component: WebGL
• Status: RESOLVED FIXED

[Kenneth Russell]

In both WebKit and Chrome's WebGL implementations, when bindFramebuffer is called with a null framebuffer, an internal FBO is actually bound. To avoid breaking these implementations, framebufferRenderbuffer and framebufferTexture2D need to synthesize GL errors when the default framebuffer is bound.

[Kenneth Russell]

Created attachment 44658 [details]
Proposed patch

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 44658 [details] without any errors.

[Oliver Hunt]

Comment on attachment 44658 [details]
Proposed patch

I think that active fbo should be tracked in the WebGLRenderingContext rather than the lower level -- in general the design of the graphics contexts is intended to minimise low level checking and instead have the DOM binding perform checks (eg. we assume webcore internally uses GraphicsContext3D in a safe way -- the DOM API assumes that web developers aren't)

This has the benefit of allowing all implementations to share the same guard logic implementation as well.

In all honesty I currently believe that too much of the validation is performed in the GraphicsContext3D implementations and intend to fix that at some point -- we basically want GraphicsContext3D to be a C++ object wrapping the appropriate system libraries with very little (if any) validation.

--Oliver

[Kenneth Russell]

The fact that GraphicsContext3D uses a framebuffer object internally is an implementation detail. It is a violation of abstraction barriers to assume this in the WebGLRenderingContext. I believe this patch is correctly formulated as is.

[Kenneth Russell]

Created attachment 44704 [details]
Revised patch

Moved synthesis of error messages from GraphicsContext3D to WebGLRenderingContext.

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 44704 [details] without any errors.

[Oliver Hunt]

Comment on attachment 44704 [details]
Revised patch


> +    // Don't allow the default framebuffer to be mutated; all current
> +    // implementations use an FBO internally in place of the default
> +    // FBO.
I'm not sure why this needs to be specially commented as being implementation dependent, my understanding of the spec is such that when we unless the user has bound a framebuffer object then the default framebuffer 0 will be bound which is expected to produce an INVALID_OPERATION error. From the man page:
GL_INVALID_OPERATION is generated if the default framebuffer object name 0 is bound.

Given the GraphicsContext3D is not necessarily using GL it would seem incorrect to require it to synthesize the GL error, when the WebGLRenderingContext must itself already know whether or not a framebuffer has been bound.

Your thoughts?

(looking at the changes to the actual GraphicsContext3D is depressing, i should sit down at some point and try and remove the layering violations -- DOM types shouldn't be used in the platform layer, my bad :-( )

[Kenneth Russell]

(In reply to comment #7)
> (From update of attachment 44704 [details])
> 
> > +    // Don't allow the default framebuffer to be mutated; all current
> > +    // implementations use an FBO internally in place of the default
> > +    // FBO.
> I'm not sure why this needs to be specially commented as being implementation
> dependent, my understanding of the spec is such that when we unless the user
> has bound a framebuffer object then the default framebuffer 0 will be bound
> which is expected to produce an INVALID_OPERATION error. From the man page:
> GL_INVALID_OPERATION is generated if the default framebuffer object name 0 is
> bound.
> 
> Given the GraphicsContext3D is not necessarily using GL it would seem incorrect
> to require it to synthesize the GL error, when the WebGLRenderingContext must
> itself already know whether or not a framebuffer has been bound.
> 
> Your thoughts?

The WebGLRenderingContext relies heavily on the GraphicsContext3D to implement OpenGL ES 2.0 semantics for the majority of OpenGL errors that are raised -- INVALID_ENUM, INVALID_VALUE, etc. The number of errors which are synthesized is very small compared to the number of errors that need to be caught by the real OpenGL implementation. It's impractical to replicate in the WebGLRenderingContext all of the checks that OpenGL would do.

It's really an implementation detail that the current GraphicsContext3Ds use a FBO internally which they bind when the user-level code binds the default framebuffer. Conceptually, the GraphicsContext3D should make it look like the default framebuffer is bound even though it isn't, and it should be the entity responsible for synthesizing the error. However, given that all of the current GraphicsContext3Ds do use FBOs internally, it seems okay to intercept these calls at the WebGLRenderingContext level, and this does increase code sharing.

In Chrome we are soon going to need to move the OpenGL calls out of the WebKit / "renderer" process. At that point the renderer process will send commands to a separate GPU process responsible for making the actual OpenGL calls. Since the renderer is untrusted code in this security model, many or most of the checks being done by the WebGLRenderingContext will need to be replicated in the GPU process. When we have this working we will want to have a discussion about how to conditionalize some of the validation in the WebGLRenderingContext. If Apple might want to move in the same architectural direction, we could share a large amount of code.

> (looking at the changes to the actual GraphicsContext3D is depressing, i should
> sit down at some point and try and remove the layering violations -- DOM types
> shouldn't be used in the platform layer, my bad :-( )

I'd be happy to help clean this up as well.

[Oliver Hunt]

> The WebGLRenderingContext relies heavily on the GraphicsContext3D to implement
> OpenGL ES 2.0 semantics for the majority of OpenGL errors that are raised --
> INVALID_ENUM, INVALID_VALUE, etc. The number of errors which are synthesized is
> very small compared to the number of errors that need to be caught by the real
> OpenGL implementation. It's impractical to replicate in the
> WebGLRenderingContext all of the checks that OpenGL would do.
Input validation logically has to occur in WebGLRenderingContext that's the boundary between trusted and untrusted code.  GraphicsContext3D should not be doing any validation.
 
> In Chrome we are soon going to need to move the OpenGL calls out of the WebKit
> / "renderer" process. At that point the renderer process will send commands to
> a separate GPU process responsible for making the actual OpenGL calls. Since
> the renderer is untrusted code in this security model, many or most of the
> checks being done by the WebGLRenderingContext will need to be replicated in
> the GPU process. When we have this working we will want to have a discussion
> about how to conditionalize some of the validation in the
> WebGLRenderingContext. If Apple might want to move in the same architectural
> direction, we could share a large amount of code.

I was going to refactor GraphicsContext3DMac into GraphicsContext3DGL and GraphicsContext3DMac as building on GL to get code sharing going there.

[Kenneth Russell]

(In reply to comment #9)
> > The WebGLRenderingContext relies heavily on the GraphicsContext3D to implement
> > OpenGL ES 2.0 semantics for the majority of OpenGL errors that are raised --
> > INVALID_ENUM, INVALID_VALUE, etc. The number of errors which are synthesized is
> > very small compared to the number of errors that need to be caught by the real
> > OpenGL implementation. It's impractical to replicate in the
> > WebGLRenderingContext all of the checks that OpenGL would do.
> Input validation logically has to occur in WebGLRenderingContext that's the
> boundary between trusted and untrusted code.  GraphicsContext3D should not be
> doing any validation.

In an architecture where the process containing WebGLRenderingContext's compiled code is untrusted, it isn't possible to rely upon its validation results. An attacker might sidestep them and issue GraphicsContext3D calls directly. The WebGLRenderingContext can still catch certain classes of erroneous inputs, but on the other side the serialized commands from the GraphicsContext3D must be validated.

For the near term we can certainly centralize the validation in the WebGLRenderingContext. It's better to have that code in one place rather than splitting it across the WebGLRenderingContext and GraphicsContext3D.

I'd be glad to discuss this more via email or face-to-face. Hopefully this discussion will not impact the review of this particular patch.

> > In Chrome we are soon going to need to move the OpenGL calls out of the WebKit
> > / "renderer" process. At that point the renderer process will send commands to
> > a separate GPU process responsible for making the actual OpenGL calls. Since
> > the renderer is untrusted code in this security model, many or most of the
> > checks being done by the WebGLRenderingContext will need to be replicated in
> > the GPU process. When we have this working we will want to have a discussion
> > about how to conditionalize some of the validation in the
> > WebGLRenderingContext. If Apple might want to move in the same architectural
> > direction, we could share a large amount of code.
> 
> I was going to refactor GraphicsContext3DMac into GraphicsContext3DGL and
> GraphicsContext3DMac as building on GL to get code sharing going there.

In the Chrome implementation we'll soon need a way to swap out the calls to the GL entry points. I'm not familiar with the WebKit API that was just checked in but I think that we need a way to register a factory for GraphicsContext3D instances. If GraphicsContext3DGL contained only shared code but left the actual GL calls up to a subclass, that would work well. I'd like to discuss this more.

[Kenneth Russell]

Could I please get a review of this patch?

[WebKit Commit Bot]

Comment on attachment 44704 [details]
Revised patch

Clearing flags on attachment: 44704

Committed r52164: <http://trac.webkit.org/changeset/52164>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.