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

[Søren Sandmann]

Sure. The extra width check can't harm.


Søren



On Mon, Sep 21, 2015 at 10:10 PM, Siarhei Siamashka <
siarhei.siamashka at gmail.com> wrote:

> On Mon, 21 Sep 2015 16:43:46 -0400
> Søren Sandmann <soren.sandmann at gmail.com> wrote:
>
> > Regardless of who ends up listed as the patch author, this is
> >
> > Reviewed-by: soren.sandmann at gmail.com
> >
> > Søren
>
> Thanks! Is your Reviewed-by still valid after adding an extra
> "width <= 0" check to the patch?
>
> Best regards,
> Siarhei Siamashka
>
> > On Sep 21, 2015 3:07 PM, "Siarhei Siamashka" <
> siarhei.siamashka at gmail.com>
> > wrote:
> >
> > > 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.
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/pixman/attachments/20150922/f2ba2130/attachment-0001.html>