| Summary: | Memory leak on "Could not set device mode to 3" error when trying to open unwritable archive | ||
|---|---|---|---|
| Product: | [Frameworks and Libraries] frameworks-karchive | Reporter: | Vincas Dargis <vindrg> |
| Component: | general | Assignee: | David Faure <faure> |
| Status: | RESOLVED FIXED | ||
| Severity: | normal | CC: | a.samirh78, kdelibs-bugs-null, vindrg |
| Priority: | NOR | ||
| Version First Reported In: | unspecified | ||
| Target Milestone: | --- | ||
| Platform: | Debian stable | ||
| OS: | Linux | ||
| Latest Commit: | https://invent.kde.org/frameworks/karchive/commit/cdbae9e7d0780d9f00ad83720d9522dad3168a49 | Version Fixed/Implemented In: | 5.95 |
| Sentry Crash Report: | |||
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
|
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..?