1188195 - Possible use of uninitialized variables in WebGL2Context::BlitFramebuffer

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

[Eric Rahm [:erahm]]

+++ This bug was initially created as a clone of Bug #1093967 +++

Coverity indicates that |dstDepthFormat|, |srcDepthFormat| [1], |dstStencilFormat|, |srcStencilFormat| [2] can all be accessed without having been initialized in |WebGL2Context::BlitFramebuffer|.

[1] https://hg.mozilla.org/mozilla-central/annotate/2ddec2dedced/dom/canvas/WebGL2ContextFramebuffers.cpp#l284
[2] https://hg.mozilla.org/mozilla-central/annotate/2ddec2dedced/dom/canvas/WebGL2ContextFramebuffers.cpp#l292

Comment 1

[Lee Salzman [:lsalzman]]

Attachment: always initialize BlitFramebuffer src/dst depth/stencil formats to some value

This patch just makes sure {src,dst}{Depth,Stencil}Format get initialized to 0 in case mOptions.{depth,stencil} aren't set. As otherwise if these are not properly initialized, the following error checking code reads these vars, which can have any random value left in memory and so cause undefined behavior.

This change should match what the nearby code in GetFBInfoForBlit does when an FBO is bound without a depth or stencil attachment, so at least the behavior is consistent with respect to the error checking code.

Comment 2

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8640633 [details] [diff] [review]
always initialize BlitFramebuffer src/dst depth/stencil formats to some value

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

::: dom/canvas/WebGL2ContextFramebuffers.cpp
@@ +210,5 @@
>              srcDepthFormat = LOCAL_GL_DEPTH24_STENCIL8;
>              srcStencilFormat = srcDepthFormat;
>          } else {
> +            srcDepthFormat = mOptions.depth ? LOCAL_GL_DEPTH_COMPONENT16 : 0;
> +            srcStencilFormat = mOptions.stencil ? LOCAL_GL_STENCIL_INDEX8 : 0;

Prefer setting the default (here `0`) at the decl, instead of requiring it at all usages.

Comment 3

[Lee Salzman [:lsalzman]]

Attachment: always initialize WebGL2 BlitFramebuffer src/dst formats to some value

This version of the patch just initializes the formats to 0 at the declaration, as requested.

Try results: https://treeherder.mozilla.org/#/jobs?repo=try&revision=250c204df3cb

Comment 4

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8640677 [details] [diff] [review]
always initialize WebGL2 BlitFramebuffer src/dst formats to some value

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

Thanks.

::: dom/canvas/WebGL2ContextFramebuffers.cpp
@@ -187,5 @@
>                                " differ.");
>          return;
>      }
>  
>      GLsizei srcSamples;

Do we need to give this a default?

Comment 5

[Lee Salzman [:lsalzman]]

(In reply to Jeff Gilbert [:jgilbert] from comment #4)
> Comment on attachment 8640677 [details] [diff] [review]
> always initialize WebGL2 BlitFramebuffer src/dst formats to some value
> 
> Review of attachment 8640677 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks.
> 
> ::: dom/canvas/WebGL2ContextFramebuffers.cpp
> @@ -187,5 @@
> >                                " differ.");
> >          return;
> >      }
> >  
> >      GLsizei srcSamples;
> 
> Do we need to give this a default?

I looked at the code and thought about setting it to 1, but many of the places were sadly marked with "TODO", as if to indicate that 1 is not necessarily always the right value as a default. So I erred on the side of caution and left it alone, since it is always set in all possible execution paths, though ironically its value is always 1 at the moment...

Comment 6

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/443dd4746c52

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/443dd4746c52