[Mesa-dev] [PATCH 2/3] llvmpipe: clamp scissors to be between 0 and max

[Brian Paul]

On 05/25/2013 12:12 AM, Zack Rusin wrote:
> We need to clamp to make sure invalid shader doesn't crash our
> driver. The spec says to return 0-th index for everything that's
> out of bounds and to reset everything above the last scissor
> to zero.
>
> Signed-off-by: Zack Rusin <zackr at vmware.com>
> ---
>   src/gallium/drivers/llvmpipe/lp_setup.h       |    3 +++
>   src/gallium/drivers/llvmpipe/lp_setup_line.c  |    2 +-
>   src/gallium/drivers/llvmpipe/lp_setup_point.c |    2 +-
>   src/gallium/drivers/llvmpipe/lp_setup_tri.c   |    2 +-
>   src/gallium/drivers/llvmpipe/lp_state_clip.c  |   16 ++++++++++++++++
>   5 files changed, 22 insertions(+), 3 deletions(-)
>
> diff --git a/src/gallium/drivers/llvmpipe/lp_setup.h b/src/gallium/drivers/llvmpipe/lp_setup.h
> index 1f55aa4..9a28cdc 100644
> --- a/src/gallium/drivers/llvmpipe/lp_setup.h
> +++ b/src/gallium/drivers/llvmpipe/lp_setup.h
> @@ -154,4 +154,7 @@ void
>   lp_setup_end_query(struct lp_setup_context *setup,
>                      struct llvmpipe_query *pq);
>
> +#define LP_CLAMP_SCISSOR(i)                \
> +   (PIPE_MAX_VIEWPORTS > i && i >= 0) ? i : 0

I'd opt to use an inline function for this, but not a big deal.


> +
>   #endif
> diff --git a/src/gallium/drivers/llvmpipe/lp_setup_line.c b/src/gallium/drivers/llvmpipe/lp_setup_line.c
> index c2a069f..289617b 100644
> --- a/src/gallium/drivers/llvmpipe/lp_setup_line.c
> +++ b/src/gallium/drivers/llvmpipe/lp_setup_line.c
> @@ -318,7 +318,7 @@ try_setup_line( struct lp_setup_context *setup,
>         nr_planes = 8;
>         if (setup->viewport_index_slot > 0) {
>            unsigned *udata = (unsigned*)v1[setup->viewport_index_slot];
> -         scissor_index = *udata;
> +         scissor_index = LP_CLAMP_SCISSOR(*udata);
>         }
>      }
>      else {
> diff --git a/src/gallium/drivers/llvmpipe/lp_setup_point.c b/src/gallium/drivers/llvmpipe/lp_setup_point.c
> index 30ce7490..b887420 100644
> --- a/src/gallium/drivers/llvmpipe/lp_setup_point.c
> +++ b/src/gallium/drivers/llvmpipe/lp_setup_point.c
> @@ -328,7 +328,7 @@ try_setup_point( struct lp_setup_context *setup,
>
>      if (setup->viewport_index_slot > 0) {
>         unsigned *udata = (unsigned*)v0[setup->viewport_index_slot];
> -      scissor_index = *udata;
> +      scissor_index = LP_CLAMP_SCISSOR(*udata);
>      }
>      /* Bounding rectangle (in pixels) */
>      {
> diff --git a/src/gallium/drivers/llvmpipe/lp_setup_tri.c b/src/gallium/drivers/llvmpipe/lp_setup_tri.c
> index c1ba52e..f44423b 100644
> --- a/src/gallium/drivers/llvmpipe/lp_setup_tri.c
> +++ b/src/gallium/drivers/llvmpipe/lp_setup_tri.c
> @@ -258,7 +258,7 @@ do_triangle_ccw(struct lp_setup_context *setup,
>         nr_planes = 7;
>         if (setup->viewport_index_slot > 0) {
>            unsigned *udata = (unsigned*)v0[setup->viewport_index_slot];
> -         scissor_index = *udata;
> +         scissor_index = LP_CLAMP_SCISSOR(*udata);
>         }
>      }
>      else {
> diff --git a/src/gallium/drivers/llvmpipe/lp_state_clip.c b/src/gallium/drivers/llvmpipe/lp_state_clip.c
> index 0e027fa..177e92a 100644
> --- a/src/gallium/drivers/llvmpipe/lp_state_clip.c
> +++ b/src/gallium/drivers/llvmpipe/lp_state_clip.c
> @@ -68,11 +68,27 @@ llvmpipe_set_scissor_states(struct pipe_context *pipe,
>                               const struct pipe_scissor_state *scissors)
>   {
>      struct llvmpipe_context *llvmpipe = llvmpipe_context(pipe);
> +   unsigned i;
>
>      draw_flush(llvmpipe->draw);
>
> +   if (start_slot >= PIPE_MAX_VIEWPORTS)
> +      return;

This seems like an error condition that shouldn't make its way down to 
the driver.  Maybe assert?


> +
> +   if ((start_slot + num_scissors) > PIPE_MAX_VIEWPORTS)
> +      num_scissors = PIPE_MAX_VIEWPORTS - start_slot;

Maybe assert here too?


> +
>      memcpy(llvmpipe->scissors + start_slot, scissors,
>             sizeof(struct pipe_scissor_state) * num_scissors);
> +
> +   /* Zero out the remaining scissor rects */
> +   for (i = start_slot + num_scissors; i < PIPE_MAX_VIEWPORTS; ++i) {
> +      llvmpipe->scissors[i].minx = 0;
> +      llvmpipe->scissors[i].miny = 0;
> +      llvmpipe->scissors[i].maxx = 0;
> +      llvmpipe->scissors[i].maxy = 0;
> +   }
> +
>      llvmpipe->dirty |= LP_NEW_SCISSOR;
>   }

Reviewed-by: Brian Paul <brianp at vmware.com>