[Mesa-dev] [PATCH] swr: Fixed an uncommon freed-memory access during state validation

[Kyriazis, George]

Looks good..

Reviewed-By: George Kyriazis <george.kyriazis at intel.com<mailto:george.kyriazis at intel.com>>

On Nov 8, 2017, at 6:39 PM, Bruce Cherniak <bruce.cherniak at intel.com<mailto:bruce.cherniak at intel.com>> wrote:

State validation is performed during clear and draw calls.  Validation
during clear was still accessing vertex buffer state.  When the currently
set vertex buffers are client arrays, this could lead to accessing freed
memory.  Such is the case with the VMD application.

Previously, vertex buffer validation depended on a dirty bit or the
draw info indicating an indexed draw.  This required special handling for
clears.  But, vertex buffer validation still occurred which was unnecessary
and wrong.

Now, only minimal validation is performed during clear, deferring the
remainder to the next draw.  And, by setting the dirty bit in swr_draw_vbo
for indexed draws, vertex buffer validation is only dependent upon a
single dirty bit.

This fixes a bug exposed by the VMD application when changing models.
---
src/gallium/drivers/swr/swr_draw.cpp  |  7 ++++++-
src/gallium/drivers/swr/swr_state.cpp | 35 +++++++++++++++++++----------------
2 files changed, 25 insertions(+), 17 deletions(-)

diff --git a/src/gallium/drivers/swr/swr_draw.cpp b/src/gallium/drivers/swr/swr_draw.cpp
index 57660c7464..a94cdd6da0 100644
--- a/src/gallium/drivers/swr/swr_draw.cpp
+++ b/src/gallium/drivers/swr/swr_draw.cpp
@@ -52,7 +52,12 @@ swr_draw_vbo(struct pipe_context *pipe, const struct pipe_draw_info *info)
      return;
   }

-   /* Update derived state, pass draw info to update function */
+   /* If indexed draw, force vertex validation since index buffer comes
+    * from draw info. */
+   if (info->index_size)
+      ctx->dirty |= SWR_NEW_VERTEX;
+
+   /* Update derived state, pass draw info to update function. */
   swr_update_derived(pipe, info);

   swr_update_draw_context(ctx);
diff --git a/src/gallium/drivers/swr/swr_state.cpp b/src/gallium/drivers/swr/swr_state.cpp
index c6da4fcb8e..4530d377ee 100644
--- a/src/gallium/drivers/swr/swr_state.cpp
+++ b/src/gallium/drivers/swr/swr_state.cpp
@@ -1204,11 +1204,6 @@ swr_update_derived(struct pipe_context *pipe,
      ctx->api.pfnSwrSetRastState(ctx->swrContext, rastState);
   }

-   /* Scissor */
-   if (ctx->dirty & SWR_NEW_SCISSOR) {
-      ctx->api.pfnSwrSetScissorRects(ctx->swrContext, 1, &ctx->swr_scissor);
-   }
-
   /* Viewport */
   if (ctx->dirty & (SWR_NEW_VIEWPORT | SWR_NEW_FRAMEBUFFER
                     | SWR_NEW_RASTERIZER)) {
@@ -1249,18 +1244,26 @@ swr_update_derived(struct pipe_context *pipe,
      ctx->api.pfnSwrSetViewports(ctx->swrContext, 1, vp, vpm);
   }

-   /* Set vertex & index buffers
-    * (using draw info if called by swr_draw_vbo)
-    * If indexed draw, revalidate since index buffer comes from
-    * pipe_draw_info.
-    */
-   if (ctx->dirty & SWR_NEW_VERTEX ||
-      (p_draw_info && p_draw_info->index_size)) {
+   /* When called from swr_clear (p_draw_info = null), render targets,
+    * rasterState and viewports (dependent on render targets) are the only
+    * necessary validation.  Defer remaining validation by setting
+    * post_update_dirty_flags and clear all dirty flags.  BackendState is
+    * still unconditionally validated below */
+   if (!p_draw_info) {
+      post_update_dirty_flags = ctx->dirty & ~(SWR_NEW_FRAMEBUFFER |
+                                               SWR_NEW_RASTERIZER |
+                                               SWR_NEW_VIEWPORT);
+      ctx->dirty = 0;
+   }
+
+   /* Scissor */
+   if (ctx->dirty & SWR_NEW_SCISSOR) {
+      ctx->api.pfnSwrSetScissorRects(ctx->swrContext, 1, &ctx->swr_scissor);
+   }

-      /* If being called by swr_draw_vbo, copy draw details */
-      struct pipe_draw_info info = {0};
-      if (p_draw_info)
-         info = *p_draw_info;
+   /* Set vertex & index buffers */
+   if (ctx->dirty & SWR_NEW_VERTEX) {
+      const struct pipe_draw_info &info = *p_draw_info;

      /* vertex buffers */
      SWR_VERTEX_BUFFER_STATE swrVertexBuffers[PIPE_MAX_ATTRIBS];
--
2.11.0

_______________________________________________
mesa-dev mailing list
mesa-dev at lists.freedesktop.org<mailto: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/20171109/48ddb966/attachment-0001.html>