With Qt 5.15.2 (and 5.13.2), KArchive master on Debian 10 Buster amd64, running this code: ``` #include <QDebug> #include <QIODevice> #include <QString> #include <KZip> int main() { KZip zip{QStringLiteral("/unwritable_file.zip")}; if (!zip.open(QIODevice::ReadWrite)) { qCritical() << zip.errorString(); } } ``` Produces this memory leak: ``` 561 (16 direct, 545 indirect) bytes in 1 blocks are definitely lost in loss record 16 of 17 in main in /home/vincas/code/opensource/karchive_bug/karchive_leak/main.cpp:11 1: operator new(unsigned long) in ./coregrind/m_replacemalloc/vg_replace_malloc.c:334 2: KArchive::createDevice(QFlags<QIODevice::OpenModeFlag>) in /home/vincas/code/opensource/karchive_bug/karchive.git/src/karchive.cpp:198 3: KArchive::open(QFlags<QIODevice::OpenModeFlag>) in /home/vincas/code/opensource/karchive_bug/karchive.git/src/karchive.cpp:144 4: main in /home/vincas/code/opensource/karchive_bug/karchive_leak/main.cpp:11 ``` Looks like QFile is not deleted in this error case, after being create here: https://invent.kde.org/frameworks/karchive/-/blob/940558bcebee041d66bd75ec2d0839c281b31356/src/karchive.cpp#L198 Originally detected using leak sanitizer with GCC. Looping 100000 times, the application leaks ~70MB (instead of 4MB usage at the start, before loop). As a sidenote, maybe it would we worth refactoring to use std::unique_ptr..?
https://invent.kde.org/frameworks/karchive/-/merge_requests/36 Thanks for the detailed analysis :)
Git commit cdbae9e7d0780d9f00ad83720d9522dad3168a49 by David Faure, on behalf of Ahmad Samir. Committed on 22/05/2022 at 15:30. Pushed by dfaure into branch 'master'. Always delete device if we created it To test: KZip zip{QStringLiteral("/unwritable_file.zip")}; zip.open(QIODevice::ReadWrite); - createDevice() is called, which allocates a QFile and assigns it to d->dev - but the kArchive::open() call itself will fail, which means that in the KZip destructor isOpen() will return false, so close() is never called which leaks the d->dev object Calling delete on a nullptr isn't a problem. We can't assign a parent for d->dev because KArchive isn't a QObject; also we can't use a std::unique_ptr to manage it because KArchive doesn't always own the QIODevice object (users can construct a KArchive with a QIODevice that they own and pass it to KArchive by e.g. setDevice()). Thanks for the very detailed bug report. FIXED-IN: 5.95 M +5 -0 src/karchive_p.h https://invent.kde.org/frameworks/karchive/commit/cdbae9e7d0780d9f00ad83720d9522dad3168a49