Review Request 108770: Fix double-free in ~KCompositeJobPrivate

[David Faure]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/108770/#review26717
-----------------------------------------------------------


I'd add the setParent(0), document it, and then this looks ok to me. But I'd like Kévin to be able to review it too.


kdecore/jobs/kcompositejob.h
<http://git.reviewboard.kde.org/r/108770/#comment20288>

    Right (but the dtor will, unlike before, due to the reparenting)....
    I guess you're right, this means a setParent(0) would be better, to ensure behavior 
    compatibility with before. I don't think the subjobs can ever be dangling at that point, in the code I've seen:
    
    The use case for this, in KIO and a few other places, is "kill quietly all subjobs, then clear the list" (so the jobs still exist at that point, the autodeletion is a deleteLater). Other code (found via lxr) kills all subjobs with EmitResult and then clear the list, this works as well.
    



kdecore/tests/kcompositejobtest.cpp
<http://git.reviewboard.kde.org/r/108770/#comment20284>

    Use QVERIFY instead of Q_ASSERT, and then you can even combine the two lines:
    QVERIFY(compositeJob->addSubjob(job));


- David Faure


On Feb. 5, 2013, 3:39 p.m., Kevin Funk wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/108770/
> -----------------------------------------------------------
> 
> (Updated Feb. 5, 2013, 3:39 p.m.)
> 
> 
> Review request for kdelibs.
> 
> 
> Description
> -------
> 
> Fix double-free in ~KCompositeJobPrivate
> 
> In case a subjob of KCompositeJob has been deleted, this KCompositeJob
> instance will crash as soon as it is being destructed, trying to delete
> this subjob again. The reason for this is that KCompositeJob::addSubjob()
> does not change the ownership of @p job. So, this job could be still
> deleted by ~QObject() by the original parent.
> 
> Add tests for this corner case.
> 
> This fixes a bug in KDevelop.
> 
> Backtrace:
> 1 0x00007ffff7a3f28e in qDeleteAll<QList<KJob*>::const_iterator>
> (end=..., begin=...) at /usr/include/qt4/QtCore/qalgorithms.h:322
> 2 qDeleteAll<QList<KJob*> > (c=QList<KJob *> = {...}) at
> /usr/include/qt4/QtCore/qalgorithms.h:330 #3
> KCompositeJobPrivate::~KCompositeJobPrivate (this=0x8849850,
> __in_chrg=<optimized out>) at ../../kdecore/jobs/kcompositejob.cpp:29
> 4 0x00007ffff7a3f2c9 in KCompositeJobPrivate::~KCompositeJobPrivate
> (this=0x8849850, __in_chrg=<optimized out>) at
> ../../kdecore/jobs/kcompositejob.cpp:30
> 5 0x00007ffff7a3fd70 in KJob::~KJob (this=0x880b030,
> __in_chrg=<optimized out>) at
> ../../kdecore/jobs/kjob.cpp:73
> 6 0x00007ffff1a8e5d9 in KDevelop::BuilderJob::~BuilderJob
> (this=0x880b030, __in_chrg=<optimized
> out>) at /home/krf/devel/src/kdevplatform/project/builderjob.cpp:158
> 
> BUG: 230692
> REVIEW: 108770
> FIXED-IN: 4.11
> 
> 
> This addresses bug 230692.
>     http://bugs.kde.org/show_bug.cgi?id=230692
> 
> 
> Diffs
> -----
> 
>   kdecore/jobs/kcompositejob.h 6ca8eed3ebf8c6f0f5c68d8843bd09a3ea928bbd 
>   kdecore/jobs/kcompositejob.cpp 5ddabd71e5bbb5f0a555a201223a52950b85e786 
>   kdecore/tests/CMakeLists.txt f19e563d5d99ad2f2806140c5b21e38b20dbde0d 
>   kdecore/tests/kcompositejobtest.h PRE-CREATION 
>   kdecore/tests/kcompositejobtest.cpp PRE-CREATION 
> 
> Diff: http://git.reviewboard.kde.org/r/108770/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Kevin Funk
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-core-devel/attachments/20130206/414ac3c3/attachment.htm>