958723 - Crash in WebGLContext::GetFramebufferAttachmentParameter when WEBGL_draw_buffers is enabled

Status: VERIFIED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

[Guillaume Abadie]

I predict crash right here if we have not bind any frame buffer before, because we are checking for if (!mBoundFramebuffer) after. :S

http://dxr.mozilla.org/mozilla-central/source/content/canvas/src/WebGLContextGL.cpp#1399

Here is a test case that would raise the crash:

gl = create a WebGL 1 context
gl.getExtension("WEBGL_draw_buffers");
gl.getFramebufferAttachmentParameter(gl.FRAMEBUFFER, gl.COLOR_ATTACHEMENT0, 0x8210);

Comment 1

[Kelsey Gilbert [:jgilbert]]

Attachment: testcase (crashes on failure)

Confirmed on nightly29.

Comment 2

[Guillaume Abadie]

Patch in coming! =)

Comment 3

[Guillaume Abadie]

Attachment: patch revision 1

Comment 4

[Kelsey Gilbert [:jgilbert]]

Attachment: testcase (crashes on failure)

Fixed testcase.

Comment 5

[Kelsey Gilbert [:jgilbert]]

Attachment: fb-query-crash

I think this is a cleaner way to do this.

Comment 6

[Kelsey Gilbert [:jgilbert]]

This should just be sec-dos, or whatever the modern equivalent is, but might be higher. Null pointer deref.

Comment 7

[Guillaume Abadie]

Comment on attachment 8358653 [details] [diff] [review]
fb-query-crash

Review of attachment 8358653 [details] [diff] [review]:
-----------------------------------------------------------------

I don't think so

::: content/canvas/src/WebGLContextGL.cpp
@@ +1408,5 @@
>          return JS::NullValue();
>      }
> +    
> +    // Particularly important for WEBGL_draw_buffers, but we might as well do it unconditionally.
> +    mBoundFramebuffer->EnsureColorAttachments(attachment - LOCAL_GL_COLOR_ATTACHMENT0);

No it isn't because if attachment == DEPTH_STENCIL_ATTACHMENT then your are having a negative number given to EnsureColorAttachments()

Comment 8

[Guillaume Abadie]

Attachment: patch revision 2

What about this? Knowing that mGLMaxColorAttachments is set to 1 by default, and then changed when enabling the WEBGL_draw_buffers extension.

Comment 9

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8358662 [details] [diff] [review]
patch revision 2

Review of attachment 8358662 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/canvas/src/WebGLContextGL.cpp
@@ +1403,2 @@
>      }
> +    else if (!mBoundFramebuffer) {

I don't like that this mixes trains of logic. Let's just check for a valid FB first.

Comment 10

[Kelsey Gilbert [:jgilbert]]

Attachment: fb-query-crash2

Comment 11

[Guillaume Abadie]

Comment on attachment 8358667 [details] [diff] [review]
fb-query-crash2

Review of attachment 8358667 [details] [diff] [review]:
-----------------------------------------------------------------

I have to say that I have thought about this idea too. But I also think that changing orders between the INVALID_OPERATION and the INVALID_ENUM could generates regression on the WebGL conformance test. Also generating an INVALID_OPERATION when the operation might even not be VALID regarding enumerations doesn't make any senses. That is why I believe the order between the INVALID_OPERATION and the INVALID_ENUM must not be changed.

Comment 12

[Guillaume Abadie]

Attachment: patch revision 3

What about that? =)

Comment 13

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8358667 [details] [diff] [review]
fb-query-crash2

Review of attachment 8358667 [details] [diff] [review]:
-----------------------------------------------------------------

We already have some INVALID_ENUM tests after the INVALID_OPERATION. I'm pretty sure we're allowed to return these in any order, so if the tests have problems with this, we should fix the tests.

That said, I ran the tests, and they all pass on both.

Comment 14

[Guillaume Abadie]

Comment on attachment 8358667 [details] [diff] [review]
fb-query-crash2

Review of attachment 8358667 [details] [diff] [review]:
-----------------------------------------------------------------

All right.

Comment 16

[Kelsey Gilbert [:jgilbert]]

This is just a null-deref. It shouldn't even qualify as sec-moderate.

Comment 17

[Kelsey Gilbert [:jgilbert]]

Attachment: patch 2: test

Comment 18

[u480271]

Comment on attachment 8385048 [details] [diff] [review]
patch 2: test

Review of attachment 8385048 [details] [diff] [review]:
-----------------------------------------------------------------

Nits

::: content/canvas/test/webgl/non-conf-tests/test_fb_param_crash.html
@@ +29,5 @@
> +    var text = 'gl.getError should be 0x' + reference.toString(16) +
> +               ', was 0x' + error.toString(16) + '.';
> +    func(error == reference, prefix + text);
> +  }
> +  

To the god of whitespace, we say "not today!"

@@ +40,5 @@
> +
> +  var result = gl.getFramebufferAttachmentParameter(gl.FRAMEBUFFER,
> +                                                    gl.COLOR_ATTACHMENT0,
> +                                                    gl.FRAMEBUFFER_ATTACHMENT_OBJECT_NAME);
> +  

To the god of whitespace, we say "not today!"

Comment 19

[Kelsey Gilbert [:jgilbert]]

Haha, alright.

Comment 20

[Kelsey Gilbert [:jgilbert]]

Attachment: patch 2: test

Comment 21

[u480271]

Comment on attachment 8385702 [details] [diff] [review]
patch 2: test

Review of attachment 8385702 [details] [diff] [review]:
-----------------------------------------------------------------

gtg

Comment 22

[Kelsey Gilbert [:jgilbert]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e43df9cf4e7d
https://hg.mozilla.org/integration/mozilla-inbound/rev/c97ab81ac53f

Comment 23

[Kelsey Gilbert [:jgilbert]]

I'll just take responsibility for this.

Comment 24

[Carsten Book [:Tomcat]]

landed on mozilla-central https://hg.mozilla.org/mozilla-central/rev/e43df9cf4e7d and 	https://hg.mozilla.org/mozilla-central/rev/c97ab81ac53f

Comment 25

[Matt Wobensmith [:mwobensmith][:matt:]]

Confirmed crash, 2014-02-11, Fx30.
Verified fixed, 2014-05-07, Fx30.