<div dir="ltr"><div>Sure. The extra width check can't harm. </div>Søren </div><div class="gmail_extra"> <div class="gmail_quote">On Mon, Sep 21, 2015 at 10:10 PM, Siarhei Siamashka <<a href="mailto:siarhei.siamashka@gmail.com" target="_blank">siarhei.siamashka@gmail.com</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">On Mon, 21 Sep 2015 16:43:46 -0400 Søren Sandmann <<a href="mailto:soren.sandmann@gmail.com">soren.sandmann@gmail.com</a>> wrote: > Regardless of who ends up listed as the patch author, this is > > Reviewed-by: <a href="mailto:soren.sandmann@gmail.com">soren.sandmann@gmail.com</a> > > Søren Thanks! Is your Reviewed-by still valid after adding an extra "width <= 0" check to the patch? Best regards, Siarhei Siamashka <div class="HOEnZb"><div class="h5"> > On Sep 21, 2015 3:07 PM, "Siarhei Siamashka" <<a href="mailto:siarhei.siamashka@gmail.com">siarhei.siamashka@gmail.com</a>> > wrote: > > > On Mon, 21 Sep 2015 17:10:36 +0200 > > <a href="mailto:ludo@gnu.org">ludo@gnu.org</a> (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: > > > > > > <a href="https://bugs.freedesktop.org/show_bug.cgi?id=92027#c6" rel="noreferrer" target="_blank">https://bugs.freedesktop.org/show_bug.cgi?id=92027#c6</a> > > > > Sorry, I forgot to mention > > <a href="http://cgit.freedesktop.org/pixman/tree/README?id=pixman-0.33.2#n46" rel="noreferrer" target="_blank">http://cgit.freedesktop.org/pixman/tree/README?id=pixman-0.33.2#n46</a> > > > > 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. </div></div></blockquote></div> </div>