D7285: pim-sieve-editor: sieveeditormainwindow: use QPointer to protect crash of ImportImapSettingWizard

Tue Aug 15 23:55:51 BST 2017

dvratil added a comment.

    I spoke about a real test case not a hypothetical testcase.
    It's a wizard modal to apps => we can"t close it.
    "qdbus org.kde.sieveeditor /MainApplication org.qtproject.Qt.QCoreApplication.quit" close apps + wizard without crash.

  Quitting the QApplication does not produce a crash in this particular case because of how things are ordered. But how about just destroying the parent window?

  Start the sieveeditor, then click Import Sieve Settings. Now run

    qdbus org.kde.sieveeditor /sieveeditor/MainWindow_1 org.qtproject.Qt.QWidget.close

  and observe a double-free corruption crash:

    *** Error in `/data/install/kde/bin/sieveeditor': double free or corruption (out): 0x00007fffffffc810 ***
    ...

  GDB:

    #0  0x00007fe26c4fc66b in __GI_raise (sig=sig at entry=6) at ../sysdeps/unix/sysv/linux/raise.c:51                                                                                                                                     
    #1  0x00007fe26c4fe470 in __GI_abort () at abort.c:89    
    #2  0x00007fe26c5428b1 in __libc_message (do_abort=do_abort at entry=2, fmt=fmt at entry=0x7fe26c65f100 "*** Error in `%s': %s: 0x%s ***\n") at ../sysdeps/posix/libc_fatal.c:175                                                         
    #3  0x00007fe26c54d759 in malloc_printerr (ar_ptr=<optimized out>, ptr=<optimized out>, str=0x7fe26c65f4f8 "double free or corruption (out)", action=<optimized out>) at malloc.c:5077                                              
    #4  0x00007fe26c54d759 in _int_free (av=<optimized out>, p=<optimized out>, have_lock=0) at malloc.c:3873         
    #5  0x00007fe26c5530be in __GI___libc_free (mem=<optimized out>) at malloc.c:2947                                 
    #6  0x00007fe26fba5c50 in ImportImapSettingWizard::~ImportImapSettingWizard() (this=0x7fffffffc5b0, __in_chrg=<optimized out>) at /data/KDE/src/kde/pim/pim-sieve-editor/src/importwizard/importimapsettingwizard.cpp:69            
    #7  0x00007fe26d3fc94b in QObjectPrivate::deleteChildren() () at /lib64/libQt5Core.so.5                                 
    #8  0x00007fe26e1c72ac in QWidget::~QWidget() () at /lib64/libQt5Widgets.so.5                                           
    #9  0x00007fe26f8a81cb in KMainWindow::~KMainWindow() (this=0x9f5de0, __in_chrg=<optimized out>) at /data/KDE/src/frameworks/kxmlgui/src/kmainwindow.cpp:395                                                                        
    #10 0x00007fe26f8ebb54 in KXmlGuiWindow::~KXmlGuiWindow() (this=0x9f5de0, __vtt_parm=0x7fe26fdd4200 <VTT for SieveEditorMainWindow+8>, __in_chrg=<optimized out>) at /data/KDE/src/frameworks/kxmlgui/src/kxmlguiwindow.cpp:111     
    #11 0x00007fe26fb7d823 in SieveEditorMainWindow::~SieveEditorMainWindow() (this=0x9f5de0, __in_chrg=<optimized out>, __vtt_parm=<optimized out>) at /data/KDE/src/kde/pim/pim-sieve-editor/src/sieveeditormainwindow.cpp:74         
    #12 0x00007fe26fb7d874 in SieveEditorMainWindow::~SieveEditorMainWindow() (this=0x9f5de0, __in_chrg=<optimized out>, __vtt_parm=<optimized out>) at /data/KDE/src/kde/pim/pim-sieve-editor/src/sieveeditormainwindow.cpp:82
    ...

  Valgrind:

    ==32250== Invalid free() / delete / delete[] / realloc()
    ==32250==    at 0x4C301E8: operator delete(void*) (vg_replace_malloc.c:576)
    ==32250==    by 0x5097C4F: ImportImapSettingWizard::~ImportImapSettingWizard() (importimapsettingwizard.cpp:69)
    ==32250==    by 0x7A9694A: QObjectPrivate::deleteChildren() (in /usr/lib64/libQt5Core.so.5.9.1)
    ==32250==    by 0x694D2AB: QWidget::~QWidget() (in /usr/lib64/libQt5Widgets.so.5.9.1)
    ==32250==    by 0x53521CA: KMainWindow::~KMainWindow() (kmainwindow.cpp:395)
    ==32250==    by 0x5395B53: KXmlGuiWindow::~KXmlGuiWindow() (kxmlguiwindow.cpp:111)
    ==32250==    by 0x506F822: SieveEditorMainWindow::~SieveEditorMainWindow() (sieveeditormainwindow.cpp:74)
    ==32250==    by 0x506F873: SieveEditorMainWindow::~SieveEditorMainWindow() (sieveeditormainwindow.cpp:82)
    ==32250==    by 0x7A99587: QObject::event(QEvent*) (in /usr/lib64/libQt5Core.so.5.9.1)
    ==32250==    by 0x69519E2: QWidget::event(QEvent*) (in /usr/lib64/libQt5Widgets.so.5.9.1)
    ...
    ==32250==  Address 0x1ffeffe570 is on thread 1's stack
    ==32250==  in frame #24, created by SieveEditorMainWindow::slotImportImapSettings() (sieveeditormainwindow.cpp:265)

  Now apply the attached patch and the crash is gone :-)

  > So if it's forbidden to use a element which has a QEventLoops on stack we need to have a policy for it as when I look at kdepim source code we use it in several place. I never see in Qt doc that it's forbidden.

  You are right, I over-generalized my statement. There are certainly valid cases where you can create an object with nested event loop on the stack: for example, it's OK to create a QApplication on the stack in main(). It's also OK to do so for any QObject-derived class with a nested event loop that does not have an explicit parent - in the code in this review, if `this` wasn't passed as a parent to  `ImportImapSettingsWizard` constructor, then creating `ImportImapSettingsWizard` on the stack would be OK. It's also OK for a non-widget QObject-derived class with nested event loop where you can guarantee that the parent object will not be deleted before the child (we do this on some places in Akonadi).

  Therefore there's no real policy, as it's hard to precisely list all the circumstances when you should use QPointer and when you are fine with just a stack allocation. The blog post linked by Alan describes a thing to watch out for and be mindful of when writing code, especially with nested modal dialogs.

  Also, this is actually documented in Qt docs to some extent, although not as explicit as what we are discussing here: http://doc.qt.io/qt-5/objecttrees.html

  > The parent's destructor is called first because it was created last. It then calls the destructor of its child, quit, which is incorrect because quit is a local variable. When quit subsequently goes out of scope, its destructor is called again, this time correctly, but the damage has already been done.

  (this does not apply exactly to the problem at hand, but describes the same kind of a problem: double deletion due to the object being deleted first by its parent, and then when the variable on the stack goes out-of-scope)

REPOSITORY
  R213 PIM Sieve Editor

REVISION DETAIL
  https://phabricator.kde.org/D7285

To: winterz, mlaurent
Cc: dvratil, dkurz, #kde_pim, dvasin, winterz, vkrause, mlaurent, knauss
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-pim/attachments/20170815/b24207f7/attachment.html>