[Mesa-dev] [PATCH] draw: fix infinite loop in line stippling

[Jose Fonseca]

On 23/11/2018 01:34, sroland at vmware.com wrote:
> From: Roland Scheidegger <sroland at vmware.com>
> 
> The calculated length of a line may be infinite, if the coords we
> get are bogus. This leads to an infinite loop in line stippling.
> To prevent this test for this explicitly (although technically
> on at least x86 sse it would actually work without the explicit
> test, as long as we use the int-converted length value).
> While here also get rid of some always-true condition.
> 
> Note this does not actually solve the root cause, which is that
> the coords we receive are bogus after clipping. This seems a difficult
> problem to solve. One issue is that due to float arithmetic, clip w
> may become 0 after clipping if the incoming geometry is
> "sufficiently degenerate", hence x/y/z ndc (and window) coords will
> be all inf (or nan). Even with w not quite 0, I believe it's possible
> we produce values which are actually outside the view volume.
> (Also, x=y=z=w=0 coords in clipspace would be not considered subject
> to clipping, and similarly result in all NaN coords.) We just hope for
> now other draw stages (and rasterizers) can handle those relatively
> safely (llvmpipe itself should be sort of robust against this, certainly
> converstion to fixed point will produce garbage, it might fail a couple
> assertions but should neither hang nor crash otherwise).
> ---
>   .../auxiliary/draw/draw_pipe_stipple.c        | 26 +++++++++++--------
>   1 file changed, 15 insertions(+), 11 deletions(-)
> 
> diff --git a/src/gallium/auxiliary/draw/draw_pipe_stipple.c b/src/gallium/auxiliary/draw/draw_pipe_stipple.c
> index d30572cc61..386b7649e4 100644
> --- a/src/gallium/auxiliary/draw/draw_pipe_stipple.c
> +++ b/src/gallium/auxiliary/draw/draw_pipe_stipple.c
> @@ -48,8 +48,8 @@
>   struct stipple_stage {
>      struct draw_stage stage;
>      float counter;
> -   uint pattern;
> -   uint factor;
> +   ushort pattern;
> +   ushort factor;
>      bool smooth;
>   };
>   
> @@ -110,7 +110,7 @@ emit_segment(struct draw_stage *stage, struct prim_header *header,
>   
>   
>   static inline bool
> -stipple_test(int counter, ushort pattern, int factor)
> +stipple_test(int counter, ushort pattern, ushort factor)
>   {
>      int b = (counter / factor) & 0xf;
>      return !!((1 << b) & pattern);
> @@ -136,6 +136,10 @@ stipple_line(struct draw_stage *stage, struct prim_header *header)
>   
>      float length;
>      int i;
> +   int intlength;
> +
> +   if (header->flags & DRAW_PIPE_RESET_STIPPLE)
> +      stipple->counter = 0;
>   
>      if (stipple->smooth) {
>         float dx = x1 - x0;
> @@ -147,21 +151,21 @@ stipple_line(struct draw_stage *stage, struct prim_header *header)
>         length = MAX2(dx, dy);
>      }
>   
> -   if (header->flags & DRAW_PIPE_RESET_STIPPLE)
> -      stipple->counter = 0;
> +   if (util_is_inf_or_nan(length))
> +      intlength = 0;
> +   else
> +      intlength = ceilf(length);
>   
>      /* XXX ToDo: instead of iterating pixel-by-pixel, use a look-up table.
>       */
> -   for (i = 0; i < length; i++) {
> +   for (i = 0; i < intlength; i++) {
>         bool result = stipple_test((int)stipple->counter + i,
> -                                 (ushort)stipple->pattern, stipple->factor);
> +                                 stipple->pattern, stipple->factor);
>         if (result != state) {
>            /* changing from "off" to "on" or vice versa */
>            if (state) {
> -            if (start != i) {
> -               /* finishing an "on" segment */
> -               emit_segment(stage, header, start / length, i / length);
> -            }
> +            /* finishing an "on" segment */
> +            emit_segment(stage, header, start / length, i / length);
>            }
>            else {
>               /* starting an "on" segment */
> 

Reviewed-by: Jose Fonseca <jfonseca at vmware.com>