Document Foundation Mail Archive › LibreOffice › Dev

[REVIEW-3-5-5] Use after delete in oox

Classic

List

Threaded

5 messages Options

Lubos Lunak

[REVIEW-3-5-5] Use after delete in oox

Hello,

please review and backport to 3-5 and 3-5-5 the attached patch.

It is a stripped down version of a revert
(http://cgit.freedesktop.org/libreoffice/core/commit/?id=831c2d9528) and
rewrite (http://cgit.freedesktop.org/libreoffice/core/commit/?id=886e29cff7)
in master, but I don't feel confident enough about the rewrite for a backport
at this point, so the patch is just a band-aid. The current 3-5 code has a
use-after-delete problem caused by the fact that it's not possible to take
out a pointer out of a shared_ptr based container.

--
Lubos Lunak
[hidden email]

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

0001-avoid-a-crash-because-of-shared_ptr-ownership.patch (2K) Download Attachment

Eike Rathke-2

Re: [PUSHED 3-5][REVIEW-3-5-5] Use after delete in oox

Hi Lubos,

On Tuesday, 2012-06-19 15:20:40 +0200, Lubos Lunak wrote:

> please review and backport to 3-5 and 3-5-5 the attached patch.

Looks sensible, pushed to 3-5
Two more reviews needed for 3-5-5

Btw, this ...

> -const ShapeBase* ShapeContainer::takeLastShape()
> +boost::shared_ptr< ShapeBase > ShapeContainer::takeLastShape()
> {
> assert( mrDrawing.getType() == VMLDRAWING_WORD );
> if( maShapes.empty())
> - return NULL;
> - const ShapeBase* ret = maShapes.back().get();
> + return boost::shared_ptr< ShapeBase >();
> + boost::shared_ptr< ShapeBase > ret = maShapes.back();
> maShapes.pop_back();
> return ret;
> }

... could be written as

boost::shared_ptr< ShapeBase > ShapeContainer::takeLastShape()
{
boost::shared_ptr< ShapeBase > ret;
assert( mrDrawing.getType() == VMLDRAWING_WORD );
if( !maShapes.empty())
{
ret.reset( maShapes.back());
maShapes.pop_back();
}
return ret;
}

to help compilers optimize the return to the caller if the return type
is defined at the start of the method (or so I have read ...)

Eike

--
LibreOffice Calc developer. Number formatter stricken i18n transpositionizer.
GnuPG key 0x293C05FD : 997A 4C60 CE41 0149 0DB3 9E96 2F1A D073 293C 05FD

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

attachment0 (205 bytes) Download Attachment

Fridrich Strba-3

Re: [PUSHED 3-5][REVIEW-3-5-5] Use after delete in oox

+1 from me too, one more person neede.

F.

On 19/06/12 22:32, Eike Rathke wrote:

> Hi Lubos,
>
> On Tuesday, 2012-06-19 15:20:40 +0200, Lubos Lunak wrote:
>
>> please review and backport to 3-5 and 3-5-5 the attached patch.
>
> Looks sensible, pushed to 3-5
> Two more reviews needed for 3-5-5
>
> Btw, this ...
>
>> -const ShapeBase* ShapeContainer::takeLastShape()
>> +boost::shared_ptr< ShapeBase > ShapeContainer::takeLastShape()
>> {
>> assert( mrDrawing.getType() == VMLDRAWING_WORD );
>> if( maShapes.empty())
>> - return NULL;
>> - const ShapeBase* ret = maShapes.back().get();
>> + return boost::shared_ptr< ShapeBase >();
>> + boost::shared_ptr< ShapeBase > ret = maShapes.back();
>> maShapes.pop_back();
>> return ret;
>> }
>
> ... could be written as
>
> boost::shared_ptr< ShapeBase > ShapeContainer::takeLastShape()
> {
> boost::shared_ptr< ShapeBase > ret;
> assert( mrDrawing.getType() == VMLDRAWING_WORD );
> if( !maShapes.empty())
> {
> ret.reset( maShapes.back());
> maShapes.pop_back();
> }
> return ret;
> }
>
> to help compilers optimize the return to the caller if the return type
> is defined at the start of the method (or so I have read ...)
>
> Eike
>
>
>
> _______________________________________________
> LibreOffice mailing list
> [hidden email]
> http://lists.freedesktop.org/mailman/listinfo/libreoffice
>

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

Miklos Vajna-2

Re: [PUSHED 3-5][REVIEW-3-5-5] Use after delete in oox

In reply to this post by Eike Rathke-2

On Tue, Jun 19, 2012 at 10:32:38PM +0200, Eike Rathke <[hidden email]> wrote:
> Two more reviews needed for 3-5-5

Looks reasonable, +1.
_______________________________________________
LibreOffice mailing list
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/libreoffice

Miklos Vajna-2

Re: [PUSHED 3-5][PUSHED-3-5-5] Use after delete in oox

On Wed, Jun 20, 2012 at 10:03:58AM +0200, Miklos Vajna <[hidden email]> wrote:
> On Tue, Jun 19, 2012 at 10:32:38PM +0200, Eike Rathke <[hidden email]> wrote:
> > Two more reviews needed for 3-5-5
>
> Looks reasonable, +1.

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