[Mesa-dev] [PATCH] i965: Fix segfault in WebGL Conformance on Ivybridge

[Ian Romanick]

On 11/10/2014 04:02 PM, Chad Versace wrote:
> Fixes regression of WebGL Conformance test texture-size-limit [1] on
> Ivybridge Mobile GT2 0x0166 with Google Chrome R38.
> 
> Regression introduced by
> 
>     commit 6c044231535b93c5d16404528946cad618d96bd9
>     Author: Kenneth Graunke <kenneth at whitecape.org>
>     Date:   Sun Feb 2 02:58:42 2014 -0800
> 
>         i965: Bump GL_MAX_CUBE_MAP_TEXTURE_SIZE to 8192.
> 
> The test regressed because the pointer offset arithmetic in
> intel_miptree_map_gtt() overflows for large textures. The pointer
> arithmetic is not 64-bit safe.
> 
> This patch fixes the bugzilla ticket below on Ivybridge. This patch
> doesn't close the ticket, though, because the bug report is against
> Sandybridge, and QA cannot confirm the fix on that hardware.
> 
> [1] https://github.com/KhronosGroup/WebGL/blob/52f0dc240f04dce31b1b8e2b8107fe2b8332dc90/sdk/tests/conformance/textures/texture-size-limit.html
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=78770
> Fixes: Intel CHRMOS-1377
> Reported-by: Lu Hua <huax.lu at intel.com>
> Signed-off-by: Chad Versace <chad at kiwitree.net>
> ---
>  src/mesa/drivers/dri/i965/intel_mipmap_tree.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> index 8fda25d..24e217c 100644
> --- a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> +++ b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> @@ -1769,7 +1769,16 @@ intel_miptree_map_gtt(struct brw_context *brw,
>        y += image_y;
>  
>        map->stride = mt->pitch;
> -      map->ptr = base + y * map->stride + x * mt->cpp;
> +
> +      /* The variables in below pointer arithmetic are 32-bit. The arithmetic
> +       * overflows for large textures.  Therefore the cast to intptr_t is
> +       * needed.
> +       *
> +       * TODO(chadv): Fix this everywhere in i965 by fixing the signature of
> +       * intel_miptree_get_image_offset() to use intptr_t.
> +       */
> +      map->ptr = base + (intptr_t) y * (intptr_t) map->stride
> +                      + (intptr_t) x * (intptr_t) mt->cpp;

I don't even want to know how long it took to track that down. :(

Is it sufficient to just use unsigned?  y and map->stride are the
problematic values here.  Isn't y bounded by (basically) 2*8192 and
map->stride bounded by 16*8192?  That sounds like 0x80000000, which is
negative.

It seems like we could have similar problems in many places, and I'd
like to figure out what the correct coding practices are to avoid it.

>     }
>  
>     DBG("%s: %d,%d %dx%d from mt %p (%s) %d,%d = %p/%d\n", __FUNCTION__,
>