[Pixman] [PATCH] Fix arithmetic overflow in pointer arithmetic in ‘general_composite_rect’

[Siarhei Siamashka]

On Mon, 21 Sep 2015 17:10:36 +0200
ludo at gnu.org (Ludovic Courtès) wrote:

> Hello,
> 
> The patch below intends to fix an arithmetic overflow occurring in a
> pointer arithmetic context in ‘general_composite_rect’, as explained at:
> 
>   https://bugs.freedesktop.org/show_bug.cgi?id=92027#c6

Sorry, I forgot to mention
    http://cgit.freedesktop.org/pixman/tree/README?id=pixman-0.33.2#n46

We would also need a commit message for the patch. So it normally
should be created with "git format-patch" command and sent to the
mailing list using "git send-email".

> The bug can most likely lead to a crash.

Yes, I can confirm that the bug is reproducible on my x86-64 system:

    export CFLAGS="-O2 -m32" && ./autogen.sh
    ./configure --disable-libpng --disable-gtk && make
    setarch i686 -R test/stress-test

> In a preliminary review, Siarhei Siamashka notes that ‘width + 1’ is
> insufficient to take 16-byte alignment constraints into account.
> Indeed, AFAICS, it is sufficient when Bpp == 16 but probably not when
> Bpp == 4.
>
> Siarhei also suggests that more rewriting in needed in that part of the
> code, but I’ll leave that to you.  ;-)

Basically, I would probably do it in the following way:


diff --git a/pixman/pixman-general.c b/pixman/pixman-general.c
index 7cdea29..5ffa063 100644
--- a/pixman/pixman-general.c
+++ b/pixman/pixman-general.c
@@ -155,23 +155,21 @@ general_composite_rect  (pixman_implementation_t
*imp, #define
ALIGN(addr)							\
((uint8_t *)((((uintptr_t)(addr)) + 15) & (~15))) 
-    src_buffer = ALIGN (scanline_buffer);
-    mask_buffer = ALIGN (src_buffer + width * Bpp);
-    dest_buffer = ALIGN (mask_buffer + width * Bpp);
+    if (_pixman_multiply_overflows_int (width, Bpp * 3))
+	return;
 
-    if (ALIGN (dest_buffer + width * Bpp) >
-	    scanline_buffer + sizeof (stack_scanline_buffer))
+    if (width * Bpp * 3 > sizeof (stack_scanline_buffer) - 32 * 3)
     {
 	scanline_buffer = pixman_malloc_ab_plus_c (width, Bpp * 3, 32
* 3); 
 	if (!scanline_buffer)
 	    return;
-
-	src_buffer = ALIGN (scanline_buffer);
-	mask_buffer = ALIGN (src_buffer + width * Bpp);
-	dest_buffer = ALIGN (mask_buffer + width * Bpp);
     }
 
+    src_buffer = ALIGN (scanline_buffer);
+    mask_buffer = ALIGN (src_buffer + width * Bpp);
+    dest_buffer = ALIGN (mask_buffer + width * Bpp);
+
     if (width_flag == ITER_WIDE)
     {
 	/* To make sure there aren't any NANs in the buffers */



This bug is your find and you should get credit for it :-)
Please let me know if you:
1. are going to send an updated patch yourself.
2. want me to do this on your behalf (listing you as the patch author).
3. want me to submit a patch myself (listing you as the bug reporter).


Also this is an important bugfix for a non-obvious problem, which can
be really a PITA to debug. I would nominate it for a pixman-0.32.8
bugfix release.

-- 
Best regards,
Siarhei Siamashka