[Mesa-dev] texstore byteswap allocation bug

[Iago Toral]

On Tue, 2015-09-01 at 12:34 +0200, Iago Toral wrote:
> 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).

Or maybe an apitrace of a test reproducing the issue...

Iago