Possible use of uninitialized variables in WebGL2Context::BlitFramebuffer

RESOLVED FIXED in Firefox 42

Status

()

defect
RESOLVED FIXED
4 years ago
Last year

People

(Reporter: erahm, Assigned: lsalzman)

Tracking

(Blocks 1 bug, {coverity})

unspecified
mozilla42
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox42 fixed)

Details

(Whiteboard: [CID 1286198][CID 1286698][CID 1286568][CID 1286258])

Attachments

(1 attachment, 1 obsolete attachment)

+++ 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
Assignee: nobody → lsalzman
Status: NEW → ASSIGNED
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.
Attachment #8640633 - Flags: review?(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.
Attachment #8640633 - Flags: review?(jgilbert) → review-
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
Attachment #8640633 - Attachment is obsolete: true
Attachment #8640677 - Flags: review?(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?
Attachment #8640677 - Flags: review?(jgilbert) → review+
(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...
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/443dd4746c52
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
You need to log in before you can comment on or make changes to this bug.