[Mesa-dev] texstore byteswap allocation bug

[Iago Toral]

Hey,

On Tue, 2015-08-18 at 12:52 +1000, Dave Airlie wrote:
> Hey,
> 
> while running CTS under valgrind I got to see a lot of
> 
> ==32256== Invalid read of size 2
> ==32256==    at 0x5B53F07: convert_ushort (format_utils.c:1155)
> ==32256==    by 0x5B8523A: _mesa_swizzle_and_convert (format_utils.c:1453)
> ==32256==    by 0x5B11151: _mesa_format_convert (format_utils.c:354)
> ==32256==    by 0x5C07054: texstore_rgba (texstore.c:806)
> ==32256==    by 0x5C073C8: _mesa_texstore (texstore.c:930)
> ==32256==    by 0x5C078B9: store_texsubimage (texstore.c:1068)
> ==32256==    by 0x5C07AC5: _mesa_store_texsubimage (texstore.c:1132)
> ==32256==    by 0x5C9A05F: st_TexSubImage (st_cb_texture.c:856)
> ==32256==    by 0x5C9A196: st_TexImage (st_cb_texture.c:880)
> ==32256==    by 0x5BF1BCC: teximage (teximage.c:3387)
> ==32256==    by 0x5BF1D67: _mesa_TexImage2D (teximage.c:3426)
> ==32256==    by 0x4CDCA15: glTexImage2D (glapi_mapi_tmp.h:2926)
> ==32256==  Address 0xa2b188a is 0 bytes after a block of size 42 alloc'd
> ==32256==    at 0x4A06C50: malloc (in
> /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
> ==32256==    by 0x5C06D97: texstore_rgba (texstore.c:734)
> ==32256==    by 0x5C073C8: _mesa_texstore (texstore.c:930)
> ==32256==    by 0x5C078B9: store_texsubimage (texstore.c:1068)
> ==32256==    by 0x5C07AC5: _mesa_store_texsubimage (texstore.c:1132)
> ==32256==    by 0x5C9A05F: st_TexSubImage (st_cb_texture.c:856)
> ==32256==    by 0x5C9A196: st_TexImage (st_cb_texture.c:880)
> ==32256==    by 0x5BF1BCC: teximage (teximage.c:3387)
> ==32256==    by 0x5BF1D67: _mesa_TexImage2D (teximage.c:3426)
> ==32256==    by 0x4CDCA15: glTexImage2D (glapi_mapi_tmp.h:2926)
> ==32256==    by 0x151483D: glwTexImage2D (glwImpl.inl:482)
> ==32256==    by 0xF1BB0B: packedPixelsPixelRectangleInner
> (GTFTestPackedPixels.c:3666)
> ==32256==
> 
> which lead to the malloc for the SwapBytes case, being too small. It
> appears the srcRowStride is worked out later at 16-bytes for a width 7
> ushort format, but the byte swap doesn't allocate enough space,
> 
> can you guys take a look and suggest a fix, I'm a bit lost there.

sorry for the late reply, I was on holidays...

If we see a srcRowStride of 16-bytes for a width of 7, I guess there are
some packing options involved. We create the swapped image like this:

if (srcPacking->SwapBytes) {
   int bytesPerPixel = _mesa_bytes_per_pixel(srcFormat, srcType);
   int swapsPerPixel = bytesPerPixel / swapSize;
   int elementCount = srcWidth * srcHeight * srcDepth;
   assert(bytesPerPixel % swapSize == 0);
   tempImage = malloc(elementCount * bytesPerPixel);
   if (!tempImage)
      return GL_FALSE;
   if (swapSize == 2)
      _mesa_swap2_copy(tempImage, (GLushort *) srcAddr,
                       elementCount * swapsPerPixel);
   else
      _mesa_swap4_copy(tempImage, (GLuint *) srcAddr,
                       elementCount * swapsPerPixel);
   srcAddr = tempImage;
}

I see that this is not considering the packing options of the original
image when allocating the swapped image (or when swapping its data), so
I guess that is the problem.

After that we will do something like:

srcRowStride =
   _mesa_image_row_stride(srcPacking, srcWidth, srcFormat, srcType);

which expects the swapped image to have the same packing options as the
original (i.e. it uses srcPacking).

I think that we need to use _mesa_image_row_stride() to compute the size
of the swapped image and then figure out how to incorporate that into
the las parameter in the calls to _mesa_swap_copy() too.

Dave, do you have any piglit tests or other samples that I can use to
reproduce the problem? (I don't have access to the conformance test
suite).

Iago