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

[Jason Ekstrand]

On Fri, Jun 10, 2016 at 6:03 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. Asserts
> of pCreateInfo members are moved to the earliest points at which they
> should not be NULL.
>
> This fixes a segfault seen in the McNopper demo, VKTS_Example09.
>
> v2: Fix disabled rasterization check (Jason Ekstrand)
>
> Signed-off-by: Nanley Chery <nanley.g.chery at intel.com>
> Cc: "12.0" <mesa-stable at lists.freedesktop.org>
> ---
>
> v2:  Also guard scissor state
>      Move pColorBlend assert to earliest point known to be non-NULL
>      Update commit message to mention changes with asserts
>
>  src/intel/vulkan/anv_pipeline.c | 65
> +++++++++++++++++++++++++++--------------
>  1 file changed, 43 insertions(+), 22 deletions(-)
>
> diff --git a/src/intel/vulkan/anv_pipeline.c
> b/src/intel/vulkan/anv_pipeline.c
> index ae03787..b15296a 100644
> --- a/src/intel/vulkan/anv_pipeline.c
> +++ b/src/intel/vulkan/anv_pipeline.c
> @@ -979,18 +979,27 @@ 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);
> -   }
> +   /* Section 9.2 of the Vulkan 1.0.15 spec says:
> +    *
> +    *    pViewportState is [...] NULL if the pipeline
> +    *    has rasterization disabled.
> +    */
> +   if (pCreateInfo->pRasterizationState->rasterizerDiscardEnable) {
>

I think you have the condition backwards.  Did you mean
!rasterizerDiscardEnable?


> +      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;
> -   if (states & (1 << VK_DYNAMIC_STATE_SCISSOR)) {
> -      typed_memcpy(dynamic->scissor.scissors,
> -                   pCreateInfo->pViewportState->pScissors,
> -                   pCreateInfo->pViewportState->scissorCount);
> +      dynamic->scissor.count = pCreateInfo->pViewportState->scissorCount;
> +      if (states & (1 << VK_DYNAMIC_STATE_SCISSOR)) {
> +         typed_memcpy(dynamic->scissor.scissors,
> +                     pCreateInfo->pViewportState->pScissors,
> +                     pCreateInfo->pViewportState->scissorCount);
> +      }
>     }
>

We may want a sensible else case.  Probably best to just set scissor and
viewport count to zero.


>
>     if (states & (1 << VK_DYNAMIC_STATE_LINE_WIDTH)) {
> @@ -1008,10 +1017,22 @@ 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.
> +    */
> +   unsigned num_color_att_unused = 0;
> +   while (num_color_att_unused < subpass->color_count &&
> +          subpass->color_attachments[num_color_att_unused++] ==
> VK_ATTACHMENT_UNUSED);
>

While the while loop here certainly is clever, it took me a while to figure
out what it's doing.  How about something like

bool writes_color = false;
for (unsigned i = 0; i < subpass->color_count; i++) {
   if (subpass->color_attachments[i] != VK_ATTACHMENT_UNUSED) {
      writes_color = true;
      break;
   }
}

That's a good deal more obvious.


> +   if (num_color_att_unused < subpass->color_count &&
> +       pCreateInfo->pRasterizationState->rasterizerDiscardEnable) {
>        assert(pCreateInfo->pColorBlendState);
> -      typed_memcpy(dynamic->blend_constants,
> -                   pCreateInfo->pColorBlendState->blendConstants, 4);
> +
> +      if (states & (1 << VK_DYNAMIC_STATE_BLEND_CONSTANTS))
> +         typed_memcpy(dynamic->blend_constants,
> +                     pCreateInfo->pColorBlendState->blendConstants, 4);
>     }
>
>     /* If there is no depthstencil attachment, then don't read
> @@ -1020,14 +1041,17 @@ 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 (pCreateInfo->pRasterizationState->rasterizerDiscardEnable &&
>

Again, !rasterizerDiscardEnable

Other than the above comments, it seems reasonable.  Did you run it through
Jenkins?  I'm kind-of surprised that the CTS didn't catch the flipped
rasterizerDiscardEnable flag.  It's possible that it only ever uses dynamic
states but that would be silly.

--Jason


> +       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 +1059,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 +1066,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 +1073,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
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20160610/ff0ffd67/attachment-0001.html>