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 <i...@freedesktop.org>
> Cc: Kenneth Graunke <kenn...@whitecape.org>
> Signed-off-by: Chad Versace <chad.vers...@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
>
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
