Issue 120040 - [From Symphony] There is a memory leak in function SvBaseLink::~SvBaseLink()
[From Symphony] There is a memory leak in function SvBaseLink::~SvBaseLink()
Status: CLOSED FIXED
Product: performance
Classification: Code
Component: code
AOO 3.4.0
All All
: P3 normal (vote)
: AOO 4.0
Assigned To: AOO issues mailing list
:
Depends on:
Blocks: 120975 121366
  Show dependency treegraph
 
Reported: 2012-06-20 07:07 UTC by ChaoHuang
Modified: 2013-02-16 09:17 UTC (History)
2 users (show)

See Also:
Issue Type: DEFECT
Latest Confirmation on: ---
Developer Difficulty: ---


Attachments
for file "main/sfx2/source/appl/lnkbase2.cxx" (769 bytes, patch)
2012-06-20 07:15 UTC, ChaoHuang
chao.dev.h: review?
Details | Diff

Note You need to log in before you can comment on or make changes to this issue.
Description ChaoHuang 2012-06-20 07:07:57 UTC
Steps to reproduce the defect:
1) Launch Aoo3.4
2) New a odt file, insert a picture in it
3) Save it, close it

Defect : There is a memory leak in function SvBaseLink::~SvBaseLink()
Comment 1 ChaoHuang 2012-06-20 07:09:37 UTC
The member data pImpl typed BaseLink_Impl is created in contructor. But it does not released in destructor. So it is a memory leak.
Comment 2 ChaoHuang 2012-06-20 07:15:31 UTC
Created attachment 78401 [details]
for file "main/sfx2/source/appl/lnkbase2.cxx"
Comment 3 Armin Le Grand 2012-06-20 14:14:09 UTC
ALG: SvBaseLink has three constructors, and only in two of them pImpl gets allocated and thus initialized. Thus, in the 3rd constructor, pImpl will point to a random address. Freeing it in the destructor could crash the office.

A minimal fix would have to init pImpl to zero and check before construction. But even then, the implementation makes use of pImpl without tests, so maybe in the 3rd constructor it should also be allocated. All in all, the whole class would need more rework, all members initialized in all constructors, and pImpl decided for 3rd constructor.
Comment 4 Armin Le Grand 2012-06-20 14:37:27 UTC
ALG: Added C++ style initialisations to all constructors, secured destruction of pImpl, secured all accesses to pImpl if not initialized, asserting if not.
Commited as r1352129.
Comment 5 ChaoHuang 2012-06-20 18:39:02 UTC
(In reply to comment #3)
> ALG: SvBaseLink has three constructors, and only in two of them pImpl gets
> allocated and thus initialized. Thus, in the 3rd constructor, pImpl will
> point to a random address. Freeing it in the destructor could crash the
> office.
> 
> A minimal fix would have to init pImpl to zero and check before
> construction. But even then,  the implementation makes use of pImpl without
> tests, so maybe in the 3rd constructor it should also be allocated. All in
> all, the whole class would need more rework, all members initialized in all
> constructors, and pImpl decided for 3rd constructor.

From my point of view, pImpl is a fundamental member in class SvBaseLink, which should be initialized in every constructor. There is no interface to set the value for pImpl. What's more, there is no checking for pImpl before use it, and pImpl is initialized in the first two constructors. I think that it should be also initialized in the third constructor.

Another concern I want to say is that it should be well to add an assert in the  beginning of the function than in the end.

I prefer code style
-------------------------------------------------------------
void SvBaseLink::SetLinkManager( LinkManager* _pMgr )
{
    OSL_ENSURE(pImpl!=NULL, "No pImpl (!)");
    pImpl->m_pLinkMgr = _pMgr;
}
-------------------------------------------------------------

to

-------------------------------------------------------------
void SvBaseLink::SetLinkManager( LinkManager* _pMgr )
{
    if(pImpl)
    {
        pImpl->m_pLinkMgr = _pMgr;
    }
    else
    {
        OSL_ENSURE(false, "No pImpl (!)");
    }
}
-------------------------------------------------------------
Comment 6 ChaoHuang 2012-10-17 08:12:56 UTC
Suggest to put it into AOO 3.5.0 release
Comment 7 Yan Ji 2012-11-30 04:47:06 UTC
Since last SVT(r1400866) shows there is no memory leak, so close this defect as resolved.