memory leaks fixes in non-gui tests #1213

michalkaptur · 2017-11-22T21:21:34Z

Fixed memory leaks non-gui tests

Description

Fixed 2 memory leaks in production code and a few in testcases. As a result leak_check_at_exit ASAN option does not need to turned off for non-gui tests.
Smart pointers should be used elsewhere for consistency, but the sooner this fixes are delivered, the lesser memory leaks are introduced.

Motivation and context

Memory leaks are bugs in short, so motivation is pretty straightforward :)

How has this been tested?

No logic was altered, thus existing testcases are sufficient

Screenshots (if appropriate):

Types of changes

✅ Bug fix (non-breaking change which fixes an issue)

Checklist:

✅ I have read the CONTRIBUTING document. [REQUIRED]
✅ My code follows the code style of this project. [REQUIRED]
✅ All new and existing tests passed. [REQUIRED]
✅ I have compiled and verified my code with -DWITH_ASAN=ON. [REQUIRED]

adolfogc · 2017-11-22T23:10:17Z

@michalkaptur For consistency with the rest of the codebase, I think it would it be better to use QScopedPointer instead of std::unique_ptr.

phoerious · 2017-11-22T23:51:20Z

I was about to suggest the same. Not for consistency, but for safety reasons. Don't use STL smart pointers, though. Use Qt smartpointers.

phoerious

Nice findings. Thanks for sharing them! You should use QScopedPointers, though.

phoerious · 2017-11-22T23:53:59Z

src/crypto/SymmetricCipherGcrypt.h

@@ -49,7 +49,7 @@ class SymmetricCipherGcrypt : public SymmetricCipherBackend
    static int gcryptMode(SymmetricCipher::Mode mode);
    void setErrorString(gcry_error_t err);

-    gcry_cipher_hd_t m_ctx;
+    gcry_cipher_hd_t m_ctx{nullptr};


This is redundant. m_ctx is already initialized to nullptr within the initialization list of the constructor.

phoerious · 2017-11-22T23:56:42Z

src/format/KeePass2Repair.cpp

@@ -29,8 +29,14 @@ KeePass2Repair::KeePass2Repair()
 {
 }

+KeePass2Repair::~KeePass2Repair()
+{
+    delete m_db;


Make m_db a QScopedPointer and remove this code.

phoerious · 2017-11-22T23:57:48Z

src/format/KeePass2Repair.cpp

 KeePass2Repair::RepairResult KeePass2Repair::repairDatabase(QIODevice* device, const CompositeKey& key)
 {
+    delete m_db;


Call

m_db.reset(reader.readDatabase(device, key, true));

instead. Replace further deletions of m_db within this file with QScopedPointer::reset() as well.

done, but kept temporary db object not to store the database after repair failed

phoerious · 2017-11-22T23:59:09Z

tests/TestCsvParser.h

@@ -63,7 +63,7 @@ private slots:

 private:
    QTemporaryFile* file;


Make this a QScopedPointer

phoerious · 2017-11-22T23:59:47Z

tests/TestCsvParser.h

@@ -63,7 +63,7 @@ private slots:

 private:
    QTemporaryFile* file;
-    CsvParser* parser;
+    std::unique_ptr<CsvParser> parser;


And this as well.

phoerious · 2017-11-23T00:00:21Z

tests/TestGroup.cpp

@@ -339,7 +340,7 @@ void TestGroup::testCopyCustomIcon()

 void TestGroup::testClone()
 {
-    Database* db = new Database();
+    std::unique_ptr<Database> db(new Database());


Replace all std::unique_ptrs in this file with QScopedPointers.

done, replaced all

phoerious · 2017-11-23T00:05:16Z

Could you also retarget this at release/2.2.3? Thanks!

adolfogc · 2017-11-23T00:06:05Z

Not for consistency, but for safety reasons.

@phoerious Why is it safer to use Qt's smartpointers than STL's?

phoerious · 2017-11-23T00:08:37Z

I think I just read you slightly wrong there. The safety part is about using smart pointers over raw pointers. I somehow read that you would choose smart pointers in general for consistency. Choosing Qt smart pointers over the STL ones is of course just about consistency. 😉

michalkaptur · 2017-11-23T08:24:58Z

Ok, I'll replace the std::unique_ptrs with QScopedPointerfor consistency and fix review remarks in the evening.
On the other hand, is it a more general rule for this project to use Qt library over std, even if the code is separated from Qt gui parts?

phoerious · 2017-11-23T08:33:36Z

Yes, use Qt classes where possible. Qt is not just a GUI toolkit.

michalkaptur · 2017-11-23T08:51:07Z

Qt is not just a GUI toolkit.

Definitely.

use Qt classes where possible

I'm more like "use std wherever possible", but for the sake of consistency - I'll adapt :)

phoerious · 2017-11-23T09:11:28Z

Qt and STL are mostly compatible, but not 100%.They also have two very distinct coding styles. Using Qt everywhere keeps the code consistent and prevents any compatibility problems from the start. I don't think we use STL containers or algorithms anywhere in KeePassXC

phoerious · 2017-11-23T18:07:08Z

src/format/KeePass2Repair.cpp

 {
-    return m_db;
+    return m_db.take();


Sorry to nag again, but I didn't see that you need to share ownership of the pointer. In this case, this should be a QSharedPointer instead of a QScopedPointer.
This method should stay const and return the QSharedPointer instead of a raw pointer.

I agree, but I wanted to leave DatabaseOpenWidget intact for now.

If we change something, we should do it properly from the start.

Ok, got it. My goal was to enable leak_check_at_exit ASAN check for non-gui tests, without breaking too many interfaces, and then work on other memory management issues.
Considering shared ownership: There's actually no need for KeePass2Repair to store the database (pointer, to be exact). What do you think about returning a tuple from repairDatabase(), consisting of RepairResult and Database*?

I understand that. I would do the same. But KeePassXC has inherited quite some baggage from KeePassX that needs cleaning up, so in cases like this we should do things properly instead of producing more patchwork.
I had a deeper look into the repair class and agree with you: there is no need to store the database. Returning a QPair<RepairResult, QSharedPointer<Database>> sounds like a decent solution to me.

While you are at it could you also please add

Copyright (C) 2017 KeePassXC Team <[email protected]>

to the license headers? This is missing somehow.

Ok, thanks for assistance and understanding :) I've just pushed fixed KeePass2Repair. Just to be clear: should this PR be merged to release/2.2.2 branch only or to both, develop and release/2.2.2? I'm not familiar with release/branching policy yet.

Only to release/2.2.3 (not 2.2.2). That branch will be merged back into develop then. Just rebase against it and change the target of this PR, I'll take care of the rest.
I hope you also don't mind if I squash your commits into one?

I'm a bit stuck in leaky TestGroup.cpp unit tests after rebase. I'll deliver it soon.

Alright, the changes are rebased against release/2.2.3, squashed and green on CI 😃

phoerious · 2017-11-23T18:12:04Z

src/format/KeePass2Repair.h

    QString errorString() const;

 private:
-    Database* m_db;
+    QScopedPointer<Database> m_db{nullptr};


You should initialize members in the initialization list of the constructor.

Why do you think so? User defined constructors for member default initialization only are considered not-the-best practice: https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#Rc-in-class-initializer

For consistency reasons. But if you must initialize things like this, at least use the assignment syntax.
In this case, however, you could (and should) skip initialization completely, which will default-initialize the smart pointer.

Agree, QScopedPointer is initialized to Q_NULLPTR by default. Fixed

Fixed 2 memory leaks in production code and a few in testcases. As a result leak_check_at_exit ASAN option does not need to turned off for non-gui tests. Smart pointers should be used elsewhere for consistency, but the sooner this fixes are delivered, the lesser memory leaks are introduced.

- Prevent database corruption when locked [#1219] - Fixes apply button not saving new entries [#1141] - Switch to Consolas font on Windows for password edit [#1229] - Multiple fixes to AppImage deployment [#1115, #1228] - Fixes multiple memory leaks [#1213] - Resize message close to 16x16 pixels [#1253]

michalkaptur force-pushed the leaks_fixes branch from 98f4096 to cb31fd8 Compare November 22, 2017 21:36

michalkaptur closed this Nov 22, 2017

michalkaptur reopened this Nov 22, 2017

phoerious requested changes Nov 23, 2017

View reviewed changes

phoerious added this to the v2.2.3 milestone Nov 23, 2017

phoerious added the bug label Nov 23, 2017

michalkaptur force-pushed the leaks_fixes branch from c4730f3 to 8af631b Compare November 23, 2017 17:38

phoerious requested changes Nov 23, 2017

View reviewed changes

michalkaptur force-pushed the leaks_fixes branch from 13fe50e to b1e2158 Compare November 27, 2017 20:03

phoerious approved these changes Nov 27, 2017

View reviewed changes

michalkaptur force-pushed the leaks_fixes branch from b1e2158 to b204e8a Compare November 27, 2017 22:09

michalkaptur changed the base branch from develop to release/2.2.3 November 27, 2017 22:09

phoerious merged commit 0ff75e7 into keepassxreboot:release/2.2.3 Nov 27, 2017

c4rlo mentioned this pull request Dec 1, 2018

Make Entry::m_tmpHistoryItem a QScopedPointer #2524

Merged

phoerious added pr: bugfix Pull request that fixes a bug and removed bug labels Nov 22, 2024

		@@ -63,7 +63,7 @@ private slots:

		private:
		QTemporaryFile* file;

memory leaks fixes in non-gui tests #1213

memory leaks fixes in non-gui tests #1213

Conversation

michalkaptur commented Nov 22, 2017 • edited Loading

Description

Motivation and context

How has this been tested?

Screenshots (if appropriate):

Types of changes

Checklist:

adolfogc commented Nov 22, 2017

phoerious commented Nov 22, 2017

phoerious left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phoerious Nov 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phoerious Nov 23, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phoerious commented Nov 23, 2017

adolfogc commented Nov 23, 2017

phoerious commented Nov 23, 2017 • edited Loading

michalkaptur commented Nov 23, 2017

phoerious commented Nov 23, 2017

michalkaptur commented Nov 23, 2017

phoerious commented Nov 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michalkaptur commented Nov 22, 2017 •

edited

Loading

phoerious Nov 22, 2017 •

edited

Loading

phoerious Nov 23, 2017 •

edited

Loading

phoerious commented Nov 23, 2017 •

edited

Loading