[Mesa-dev] [PATCH 3/3] anv/pipeline: Don't dereference NULL dynamic state pointers

[Nanley Chery]

On Fri, Jun 10, 2016 at 04:33:06PM -0700, Jason Ekstrand wrote:
> On Fri, Jun 10, 2016 at 4:12 PM, Nanley Chery <nanleychery at gmail.com> wrote:
> 
> > From: Nanley Chery <nanley.g.chery at intel.com>
> >
> > Add guards to prevent dereferencing NULL dynamic pipeline state.
> >
> > This fixes a segfault seen in the McNopper demo, VKTS_Example09.
> >
> > Signed-off-by: Nanley Chery <nanley.g.chery at intel.com>
> > Cc: "12.0" <mesa-stable at lists.freedesktop.org>
> > ---
> >  src/intel/vulkan/anv_pipeline.c | 51
> > ++++++++++++++++++++++++++++++-----------
> >  1 file changed, 37 insertions(+), 14 deletions(-)
> >
> > diff --git a/src/intel/vulkan/anv_pipeline.c
> > b/src/intel/vulkan/anv_pipeline.c
> > index d5a3a21..4ce13eb 100644
> > --- a/src/intel/vulkan/anv_pipeline.c
> > +++ b/src/intel/vulkan/anv_pipeline.c
> > @@ -979,11 +979,28 @@ copy_non_dynamic_state(struct anv_pipeline *pipeline,
> >
> >     struct anv_dynamic_state *dynamic = &pipeline->dynamic_state;
> >
> > -   dynamic->viewport.count = pCreateInfo->pViewportState->viewportCount;
> > -   if (states & (1 << VK_DYNAMIC_STATE_VIEWPORT)) {
> > -      typed_memcpy(dynamic->viewport.viewports,
> > -                   pCreateInfo->pViewportState->pViewports,
> > -                   pCreateInfo->pViewportState->viewportCount);
> > +   bool fs_used = pipeline->active_stages & VK_SHADER_STAGE_FRAGMENT_BIT;
> > +   bool color_att_used = false;
> > +   for (unsigned i = 0; i < subpass->color_count; ++i) {
> > +      if (subpass->color_attachments[i] != VK_ATTACHMENT_UNUSED) {
> > +         color_att_used = true;
> > +         break;
> > +      }
> > +   }
> > +
> > +   /* Section 9.2 of the Vulkan 1.0.15 spec says:
> > +    *
> > +    *    pViewportState is [...] NULL if the pipeline
> > +    *    has rasterization disabled.
> > +    */
> > +   if (fs_used) {
> >
> 
> fs_used is not equivalent to rasterization being disabled.  The only think
> I know of that can actually cause rasterization to be disabled is if you
> set rasterizerDiscardEnable.  If you don't have a fragment shader they
> still get rastierized, but it's depth-only.
> 

You're right, the check should be on rasterizerDiscardEnable. This
agrees with the Valid Usage note under VkGraphicsPipelineCreateInfo.
I forgot to mention that fs_used was based on an assumption of mine.
Thanks!

- Nanley


> --Jason
> 
> 
> > +      assert(pCreateInfo->pViewportState);
> > +      dynamic->viewport.count =
> > pCreateInfo->pViewportState->viewportCount;
> > +      if (states & (1 << VK_DYNAMIC_STATE_VIEWPORT)) {
> > +         typed_memcpy(dynamic->viewport.viewports,
> > +                     pCreateInfo->pViewportState->pViewports,
> > +                     pCreateInfo->pViewportState->viewportCount);
> > +      }
> >     }
> >
> >     dynamic->scissor.count = pCreateInfo->pViewportState->scissorCount;
> > @@ -1008,7 +1025,14 @@ copy_non_dynamic_state(struct anv_pipeline
> > *pipeline,
> >           pCreateInfo->pRasterizationState->depthBiasSlopeFactor;
> >     }
> >
> > -   if (states & (1 << VK_DYNAMIC_STATE_BLEND_CONSTANTS)) {
> > +   /* Section 9.2 of the Vulkan 1.0.15 spec says:
> > +    *
> > +    *    pColorBlendState is [...] NULL if the pipeline has rasterization
> > +    *    disabled or if the subpass of the render pass the pipeline is
> > +    *    created against does not use any color attachments.
> > +    */
> > +   if (fs_used && color_att_used &&
> > +       states & (1 << VK_DYNAMIC_STATE_BLEND_CONSTANTS)) {
> >        assert(pCreateInfo->pColorBlendState);
> >        typed_memcpy(dynamic->blend_constants,
> >                     pCreateInfo->pColorBlendState->blendConstants, 4);
> > @@ -1020,14 +1044,16 @@ copy_non_dynamic_state(struct anv_pipeline
> > *pipeline,
> >      * no need to override the depthstencil defaults in
> >      * anv_pipeline::dynamic_state when there is no depthstencil
> > attachment.
> >      *
> > -    * From the Vulkan spec (20 Oct 2015, git-aa308cb):
> > +    * Section 9.2 of the Vulkan 1.0.15 spec says:
> >      *
> > -    *    pDepthStencilState [...] may only be NULL if renderPass and
> > subpass
> > -    *    specify a subpass that has no depth/stencil attachment.
> > +    *    pDepthStencilState is [...] NULL if the pipeline has
> > rasterization
> > +    *    disabled or if the subpass of the render pass the pipeline is
> > created
> > +    *    against does not use a depth/stencil attachment.
> >      */
> > -   if (subpass->depth_stencil_attachment != VK_ATTACHMENT_UNUSED) {
> > +   if (fs_used && subpass->depth_stencil_attachment !=
> > VK_ATTACHMENT_UNUSED) {
> > +      assert(pCreateInfo->pDepthStencilState);
> > +
> >        if (states & (1 << VK_DYNAMIC_STATE_DEPTH_BOUNDS)) {
> > -         assert(pCreateInfo->pDepthStencilState);
> >           dynamic->depth_bounds.min =
> >              pCreateInfo->pDepthStencilState->minDepthBounds;
> >           dynamic->depth_bounds.max =
> > @@ -1035,7 +1061,6 @@ copy_non_dynamic_state(struct anv_pipeline *pipeline,
> >        }
> >
> >        if (states & (1 << VK_DYNAMIC_STATE_STENCIL_COMPARE_MASK)) {
> > -         assert(pCreateInfo->pDepthStencilState);
> >           dynamic->stencil_compare_mask.front =
> >              pCreateInfo->pDepthStencilState->front.compareMask;
> >           dynamic->stencil_compare_mask.back =
> > @@ -1043,7 +1068,6 @@ copy_non_dynamic_state(struct anv_pipeline *pipeline,
> >        }
> >
> >        if (states & (1 << VK_DYNAMIC_STATE_STENCIL_WRITE_MASK)) {
> > -         assert(pCreateInfo->pDepthStencilState);
> >           dynamic->stencil_write_mask.front =
> >              pCreateInfo->pDepthStencilState->front.writeMask;
> >           dynamic->stencil_write_mask.back =
> > @@ -1051,7 +1075,6 @@ copy_non_dynamic_state(struct anv_pipeline *pipeline,
> >        }
> >
> >        if (states & (1 << VK_DYNAMIC_STATE_STENCIL_REFERENCE)) {
> > -         assert(pCreateInfo->pDepthStencilState);
> >           dynamic->stencil_reference.front =
> >              pCreateInfo->pDepthStencilState->front.reference;
> >           dynamic->stencil_reference.back =
> > --
> > 2.8.3
> >
> >