input for fdo#45779 from a basegfx knowledgeable person needed

hello,
I have quickly investigated the crash in fdo#45779 when saving an
impress document.

The reason of the crash is in basegfx/inc/basegfx/point/b2dpoint.hxx
(line 82) where this is called:

2DPoint::B2DPoint (this=0xbfffc850, rPoint=...)
       :   B2DTuple(rPoint)
(from back-trace)

It turns out that in this case, rPoint is 0x0, the null pointer.
and B2DTuple does not support it

B2DTuple(const B2DTuple& rTup)
         :   mfX( rTup.mfX ),
             mfY( rTup.mfY )
{}

Here you dereference the null pointer, which crash.

Ok, the basic attitude would be to let B2DTuple be Null-pointer
consistent: (checking that rTup is not NULL), but is it really a good idea?

What is a NULL B2DTuple ?

Or should the caller (this is called due to
basegfx/source/polygon/b2dpolygon.cxx:1257) take care of the case,
returning either the value, ... or NULL ?

B2DPoint B2DPolygon::getB2DPoint(sal_uInt32 nIndex) const
     {
         OSL_ENSURE(nIndex < mpPolygon->count(), "B2DPolygon a
         return mpPolygon->getPoint(nIndex);
     }


Or should I look higher in the hierarchy, saying that a NULL point in a
B2DPolygon has nothing to do and disallow it ?

As far as I could seee, this polygon had 4 elements / points, all with
NULL data at the time of the crash :-/

What would be the right (and most meaningfull) approach ?

Thanks & regards
Pierre-André
_______________________________________________
LibreOffice mailing list
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/libreoffice

Pierre-André Jacquod wrote:
> B2DTuple(const B2DTuple& rTup)
> ...
> Here you dereference the null pointer, which crash.
>
> Ok, the basic attitude would be to let B2DTuple be Null-pointer
> consistent: (checking that rTup is not NULL), but is it really a
> good idea?
>
Hi Pierre-André,

no, what's passed is a reference, which by definition is always
de-referencable - so the bug is in the calling code.

> Or should I look higher in the hierarchy, saying that a NULL point
> in a B2DPolygon has nothing to do and disallow it ?
>
Yes, the calling code somehow accesses invalid elements. Let me have
a look.

Cheers,

-- Thorsten

_______________________________________________
LibreOffice mailing list
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/libreoffice

> > Or should I look higher in the hierarchy, saying that a NULL point
> > in a B2DPolygon has nothing to do and disallow it ?
> >
> Yes, the calling code somehow accesses invalid elements. Let me have
> a look.
>
Fixed with d37abad97d72bae0fd0269de12e94c7a7d3fd7e1 - but, if you
like, would be cool to chase down why in the first place the ppt
import creates polygons with empty sub-paths, that looks like a
worthwhile optimization - code is around
filter/source/msfilter/msdffimp.cxx probably.

Cheers,

-- Thorsten

_______________________________________________
LibreOffice mailing list
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/libreoffice

hello,
> Fixed with d37abad97d72bae0fd0269de12e94c7a7d3fd7e1 - but, if you
thanks

> import creates polygons with empty sub-paths, that looks like a
> worthwhile optimization - code is around
> filter/source/msfilter/msdffimp.cxx probably.

as soon as I have more spare time, I will have a try.
Regards
Pierre-André

_______________________________________________
LibreOffice mailing list
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/libreoffice

Hello,
back again after a while.


On 02/15/2012 11:30 AM, Thorsten Behrens wrote:
> Fixed with d37abad97d72bae0fd0269de12e94c7a7d3fd7e1 - but, if you
> like, would be cool to chase down why in the first place the ppt
> import creates polygons with empty sub-paths, that looks like a
> worthwhile optimization - code is around
> filter/source/msfilter/msdffimp.cxx probably.

It happens that basegfx::GetLineArrow(...) (also defined within
msdffimp.cxx, line 1102) does not create a valid polygon when eLineEnd
has the value mso_lineNoEnd...
In the switch(eLineEnd), this goes to
 >  default: break;
without creating any polygon, letting the return value undefined.

So I propose to skip the creation these incorrect polygons if eLineEnd
has the value mso_lineNoEnd. But since I do not understand well the
import / translation from msformat, I may also have missed a big point.
Hence thanks for reviewing this patch, before I push it.

I hope it helps.

Regards
Pierre-André

_______________________________________________
LibreOffice mailing list
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/libreoffice

Pierre-André Jacquod wrote:
> back again after a while.
>
Hi Pierre-André, welcome back! :)
there does not look too sensible. Canonical place that interprets
this for rendering is

 svx/source/sdr/primitive2d/sdrattributecreator.cxx:298

, which is already gated by the StartWidth item - so I'd think we
need to set at least that one, in any case.

Digging a bit deeper, though, e.g.

 sd/source/core/drawdoc4.cxx:175

sets a curious aNullPolyPolygon, so with a bit of bad luck, code
will rely on XLineStartItem/XLineEndItem being set to empty at other
places (counted some 90-odd places where XLineStartItem is used).

So what I suggest is a more defensive fix (or some larger review
across the code is in order): make GetLineArrow() return the
B2DPolyPolygon right away, and have *that one* be empty (i.e. not a
non-empty B2DPolyPolygon with an empty B2DPolygon, as it is now).

That matches the defaults the Impress core sets, so we should be
rather safe. ;)

Cheers,

-- Thorsten

_______________________________________________
LibreOffice mailing list
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/libreoffice

Hello,

On 05/03/2012 11:30 AM, Thorsten Behrens wrote:
> So what I suggest is a more defensive fix (or some larger review
> across the code is in order): make GetLineArrow() return the
> B2DPolyPolygon right away, and have *that one* be empty (i.e. not a

Since I do not intend to become a specialist of this kind of elements, I
choose your more defensive proposition.
So here the patch...
Thanks for a review and an ack before I push it.


Regards
Pierre-André

_______________________________________________
LibreOffice mailing list
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/libreoffice

Pierre-André Jacquod wrote:
> So here the patch...
> Thanks for a review and an ack before I push it.
>
Looks good, thanks a lot - pushed it right away.

Cheers,

-- Thorsten

_______________________________________________
LibreOffice mailing list
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/libreoffice