koffice/krita/image

[Cyrille Berger]

On Monday 28 December 2009, you wrote:
> On Mon, Dec 28, 2009 at 2:02 PM, Cyrille Berger <cberger at cberger.net> wrote:
> > Hi,
> >
> > I am not sure if I get the problem, but if the dataRect is invalid or
> > empty,
> > then no convolution should happen, and the correct way is to return. Or
> > am I
> > missing something ?
> 
> Without workaround: if dataRect is empty or invalid applyMatrix simply
> crashes inside iterators.
> The problem is applyMatrix does NOT guarantee successful execution and does
> not report any fails, so it prepares bad dataRects for itself during
> consecutive execution. In addition it has quite strange checks inside those
> lead to fail when everything is ok, i mean

The only reason why applyMatrix should return is when there is nothing to 
convolve.

> if (areaSize.width() < kw || areaSize.height() < kh || ...) return;
When you see something that looks suspicious, I can only suggest to look in 
the history, to find when the change was introduced and if there is a given 
reason. I have done it for this one, it was introduced in [1] without an 
explantion, so if you get better result without it, then you can remove that 
part. (the (kw&1) == 0 || (kh&1) == 0 || kernel->factor() == 0 part should 
probably made an assert)

[1] 
http://websvn.kde.org/trunk/koffice/krita/image/kis_convolution_painter.cc?revision=417714&view=markup
 
> 
> To ensure we don't forget about this i added a ticket:
> https://bugs.kde.org/show_bug.cgi?id=220310
> 
> > On Sunday 27 December 2009, Dmitry Kazakov wrote:
> > > SVN commit 1066740 by dkazakov:
> > >
> > > Workaround for consequent convolutions crash
> > >
> > >
> > > FIXME: Implementation can return empty destination device
> > > on faults and has no way to report this. This will cause a crash
> > > on sequential convolutions inside iteratiors.
> > >
> > > o implementation should do it's work or assert otherwise
> > >   (or report the issue somehow)
> > > o check other cases of the switch for the vulnerability
> > >
> > > CCBUG:220310
> > > CCMAIL:cberger at cberger.net <CCMAIL%3Acberger at cberger.net>
> > >
> > >
> > >  M  +13 -1     kis_convolution_painter.cc
> > >
> > >
> > > --- trunk/koffice/krita/image/kis_convolution_painter.cc
> > > #1066739:1066740 @@ -82,7 +82,19 @@
> > >      switch (borderOp) {
> > >      case BORDER_REPEAT: {
> > >          QRect dataRect = src->exactBounds();
> > > -        applyMatrixImpl<RepeatIteratorFactory>(kernel, src, srcPos,
> > >  dstPos, areaSize, dataRect); +
> > > +        /**
> > > +         * FIXME: Implementation can return empty destination device
> > > +         * on faults and has no way to report this. This will cause a
> > >  crash +         * on sequential convolutions inside iteratiors.
> > > +         *
> > > +         * o implementation should do it's work or assert otherwise
> > > +         *   (or report the issue somehow)
> > > +         * o check other cases of the switch for the vulnerability
> > > +         */
> > > +
> > > +        if(dataRect.isValid())
> > > +            applyMatrixImpl<RepeatIteratorFactory>(kernel, src,
> > > srcPos, dstPos, areaSize, dataRect); }
> > >      return;
> > >      case BORDER_DEFAULT_FILL : {
> >
> > --
> > Cyrille Berger
> 


-- 
Cyrille Berger