[Mesa-dev] [PATCH 2/4] i965: Fix intel_miptree_map() signature to be more 64-bit safe

Ian Romanick idr at freedesktop.org
Wed Nov 19 14:13:03 PST 2014


On 11/18/2014 09:11 PM, Chad Versace wrote:
> This patch should diminish the likelihood of pointer arithmetic overflow
> bugs, like the one fixed by b69c7c5dac.
> 
> Change the type of parameter 'out_stride' from int to ptrdiff_t. The
> logic is that if you call intel_miptree_map() and use the value of
> 'out_stride', then you must be doing pointer arithmetic on 'out_ptr'.
> Using ptrdiff_t instead of int should make a little bit harder to hit
> overflow bugs.

So... we talked about this a little bit on Monday, and I don't think we
ever had a conclusion.  What happens if you have a 32-bit application
running on a platform with 48-bit GPU address space?

> As a side-effect, some function-scope variables needed to be retyped to
> avoid compilation errors.
> 
> Cc: Ian Romanick <idr at freedesktop.org>
> Cc: Kenneth Graunke <kenneth at whitecape.org>
> Signed-off-by: Chad Versace <chad.versace at linux.intel.com>
> ---
>  src/mesa/drivers/dri/i965/intel_copy_image.c  |  4 ++--
>  src/mesa/drivers/dri/i965/intel_fbo.c         |  4 ++--
>  src/mesa/drivers/dri/i965/intel_mipmap_tree.c | 17 ++++++++++++++---
>  src/mesa/drivers/dri/i965/intel_mipmap_tree.h |  2 +-
>  src/mesa/drivers/dri/i965/intel_tex.c         |  7 +++++--
>  5 files changed, 24 insertions(+), 10 deletions(-)
> 
> diff --git a/src/mesa/drivers/dri/i965/intel_copy_image.c b/src/mesa/drivers/dri/i965/intel_copy_image.c
> index cb44474..f4c7eff 100644
> --- a/src/mesa/drivers/dri/i965/intel_copy_image.c
> +++ b/src/mesa/drivers/dri/i965/intel_copy_image.c
> @@ -145,7 +145,7 @@ copy_image_with_memcpy(struct brw_context *brw,
>  {
>     bool same_slice;
>     void *mapped, *src_mapped, *dst_mapped;
> -   int src_stride, dst_stride, i, cpp;
> +   ptrdiff_t src_stride, dst_stride, cpp;
>     int map_x1, map_y1, map_x2, map_y2;
>     GLuint src_bw, src_bh;
>  
> @@ -197,7 +197,7 @@ copy_image_with_memcpy(struct brw_context *brw,
>     src_width /= (int)src_bw;
>     src_height /= (int)src_bh;
>  
> -   for (i = 0; i < src_height; ++i) {
> +   for (int i = 0; i < src_height; ++i) {
>        memcpy(dst_mapped, src_mapped, src_width * cpp);
>        src_mapped += src_stride;
>        dst_mapped += dst_stride;
> diff --git a/src/mesa/drivers/dri/i965/intel_fbo.c b/src/mesa/drivers/dri/i965/intel_fbo.c
> index 4a03b57..96e7b9e 100644
> --- a/src/mesa/drivers/dri/i965/intel_fbo.c
> +++ b/src/mesa/drivers/dri/i965/intel_fbo.c
> @@ -127,7 +127,7 @@ intel_map_renderbuffer(struct gl_context *ctx,
>     struct intel_renderbuffer *irb = intel_renderbuffer(rb);
>     struct intel_mipmap_tree *mt;
>     void *map;
> -   int stride;
> +   ptrdiff_t stride;
>  
>     if (srb->Buffer) {
>        /* this is a malloc'd renderbuffer (accum buffer), not an irb */
> @@ -189,7 +189,7 @@ intel_map_renderbuffer(struct gl_context *ctx,
>        stride = -stride;
>     }
>  
> -   DBG("%s: rb %d (%s) mt mapped: (%d, %d) (%dx%d) -> %p/%d\n",
> +   DBG("%s: rb %d (%s) mt mapped: (%d, %d) (%dx%d) -> %p/%"PRIdPTR"\n",
>         __FUNCTION__, rb->Name, _mesa_get_format_name(rb->Format),
>         x, y, w, h, map, stride);
>  
> diff --git a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> index 7081f1d..f815fbe 100644
> --- a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> +++ b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> @@ -1111,7 +1111,7 @@ intel_miptree_copy_slice_sw(struct brw_context *brw,
>                              int height)
>  {
>     void *src, *dst;
> -   int src_stride, dst_stride;
> +   ptrdiff_t src_stride, dst_stride;
>     int cpp = dst_mt->cpp;
>  
>     intel_miptree_map(brw, src_mt,
> @@ -1129,7 +1129,7 @@ intel_miptree_copy_slice_sw(struct brw_context *brw,
>                       BRW_MAP_DIRECT_BIT,
>                       &dst, &dst_stride);
>  
> -   DBG("sw blit %s mt %p %p/%d -> %s mt %p %p/%d (%dx%d)\n",
> +   DBG("sw blit %s mt %p %p/%"PRIdPTR" -> %s mt %p %p/%"PRIdPTR" (%dx%d)\n",
>         _mesa_get_format_name(src_mt->format),
>         src_mt, src, src_stride,
>         _mesa_get_format_name(dst_mt->format),
> @@ -2259,6 +2259,17 @@ can_blit_slice(struct intel_mipmap_tree *mt,
>     return true;
>  }
>  
> +/**
> + * Parameter \a out_stride has type ptrdiff_t not because the buffer stride may
> + * exceed 32 bits but to diminish the likelihood subtle bugs in pointer
> + * arithmetic overflow.
> + *
> + * If you call this function and use \a out_stride, then you're doing pointer
> + * arithmetic on \a out_ptr. The type of \a out_stride doesn't prevent all
> + * bugs.  The caller must still take care to avoid 32-bit overflow errors in
> + * all arithmetic expressions that contain buffer offsets and pixel sizes,
> + * which usually have type uint32_t or GLuint.
> + */
>  void
>  intel_miptree_map(struct brw_context *brw,
>                    struct intel_mipmap_tree *mt,
> @@ -2270,7 +2281,7 @@ intel_miptree_map(struct brw_context *brw,
>                    unsigned int h,
>                    GLbitfield mode,
>                    void **out_ptr,
> -                  int *out_stride)
> +                  ptrdiff_t *out_stride)
>  {
>     struct intel_miptree_map *map;
>  
> diff --git a/src/mesa/drivers/dri/i965/intel_mipmap_tree.h b/src/mesa/drivers/dri/i965/intel_mipmap_tree.h
> index f0f6814..44ddc60 100644
> --- a/src/mesa/drivers/dri/i965/intel_mipmap_tree.h
> +++ b/src/mesa/drivers/dri/i965/intel_mipmap_tree.h
> @@ -733,7 +733,7 @@ intel_miptree_map(struct brw_context *brw,
>  		  unsigned int h,
>  		  GLbitfield mode,
>  		  void **out_ptr,
> -		  int *out_stride);
> +		  ptrdiff_t *out_stride);
>  
>  void
>  intel_miptree_unmap(struct brw_context *brw,
> diff --git a/src/mesa/drivers/dri/i965/intel_tex.c b/src/mesa/drivers/dri/i965/intel_tex.c
> index 549d9b8..3be72d5 100644
> --- a/src/mesa/drivers/dri/i965/intel_tex.c
> +++ b/src/mesa/drivers/dri/i965/intel_tex.c
> @@ -205,11 +205,12 @@ intel_map_texture_image(struct gl_context *ctx,
>  			GLuint x, GLuint y, GLuint w, GLuint h,
>  			GLbitfield mode,
>  			GLubyte **map,
> -			GLint *stride)
> +			GLint *out_stride)
>  {
>     struct brw_context *brw = brw_context(ctx);
>     struct intel_texture_image *intel_image = intel_texture_image(tex_image);
>     struct intel_mipmap_tree *mt = intel_image->mt;
> +   ptrdiff_t stride;
>  
>     /* Our texture data is always stored in a miptree. */
>     assert(mt);
> @@ -228,7 +229,9 @@ intel_map_texture_image(struct gl_context *ctx,
>                       tex_image->Level + tex_image->TexObject->MinLevel,
>                       slice + tex_image->TexObject->MinLayer,
>                       x, y, w, h, mode,
> -                     (void **)map, stride);
> +                     (void **)map, &stride);
> +
> +   *out_stride = stride;
>  }
>  
>  static void
> 



More information about the mesa-dev mailing list