Memory leaks caught by ASAN

[nyanpasu64]

While investigating #242, I tried running kImageAnnotator's example program in Address Sanitizer (I took the quick-and-dirty approach of passing -DCMAKE_CXX_FLAGS=-fsanitize=address -DCMAKE_LINKER_FLAGS=-fsanitize=address into CMake). After fixing the initial destruction crash by removing FontPicker's destructor entirely (along with all delete statements), Address Sanitizer reported some memory leaks in the program.

The largest leak (5 kilobytes) was in ToolPicker::initGui(), where you call new QMenu() 6 times, then pass the result into createButton(menu). This in turn invokes CustomToolButton::setMenu(menu), which delegates to QToolButton::setMenu(menu). However, reading the QToolButton docs, it turns out that QToolButton::setMenu() does not take ownership of the pointer you pass in. So you have to delete the QMenu by hand… hopefully in a way that avoids a use-after-free though.

I've observed that Qt methods are highly variable in whether they take ownership or not, and you have to read the docs on a case-by-case basis. Sometimes I remained unclear on whether a method took ownership, or whether deleting variables in the wrong order could cause a use-after-free (it usually doesn't, since Qt disconnects references from other objects to a deleted object), so I had to read the Qt source code, which is (to its credit) more readable than the usual C++ standard libraries.

There's lots of small leaks that ASAN catches, but most of them have ASAN stack traces that terminate in Qt's internal functions or KDE's theming/libraries (which weren't built with ASAN), so I don't know what causes them. One small leak is that ModifyCanvasWidget::mSelectionHandler doesn't seem to be freed by either ~ModifyCanvasView or ~ModifyCanvasWidget. Adding a delete mSelectionHandler; to ~ModifyCanvasWidget() cleans it up nicely (hopefully it doesn't double-free or use-after-free). Another one is AnnotationTabWidget::mTabCloser which I didn't dig deep enough to find out what to delete to resolve the bug without introducing UAF or double-frees.

All in all, these memory leaks don't actually matter all that much, since they're on the order of kilobytes or <1 KB, and never grow even if a user opens Spectacle, repeatedly takes screenshots, and repeatedly opens and closes the annotator tool. And additionally, trying to fix memory leaks runs the risk of adding double-frees or UAF bugs (I don't know how likely). I just don't know whether there was a conscious decision made to leak these variables, or if you just didn't track ownership carefully (and will choose to either fix these leaks or not).

Personally I wouldn't write manual delete calls in most application code, but instead rely on either Qt's ownership and child-deletion system, or std::unique_ptr to replace raw pointers not owned and deleted by a QObject.

[DamirPorobic]

Thanks for sharing this, I'll have a detailed view into it in next couple of days and before releasing patch 0.5.1.

One user already pointed out a crash caused by our manual deletes (#242) so that is definitely something that we need to take more care of.

[DamirPorobic]

Sorry, initially I just quickly read through your post and totally missed that you were already looking into the #242 issue. I'm removing now those deletes with parents, afterwards I'll have a look into the leaks.

[nyanpasu64]

• I see some objects (AnnotationArea, SelectionHandler, AnnotationItemFactory...) inheriting from QObject, but where the subclass doesn't take a "parent" field. The "idiomatic Qt" way to memory-manage class Foo : public QObject is to add a Foo::Foo(... QObject *parent = nullptr) pointer as the last constructor argument, then thread it from the subclass all the way down to QObject, then remove delete mFoo calls from destructors and let Qt's parent-child system take care of deleting Foo. It's more idiomatic in Qt code, but I haven't investigated the performance or memory advantages or disadvantages.
  • In my refactoring branch, I decided to remove the = nullptr default, so I could use compiler errors to find all the locations I forgot to pass in a parent, so I could convert those places from delete mFoo to QObject parent-childs. But this causes extra work (having to type in nullptr manually) if you want to keep manual delete mFoo and avoid using parent-child in some places.
• When you construct a new QMenu() in src/widgets/ToolPicker.cpp, you should replace with new QMenu(this) instead of letting it leak. It's the easiest way to fix the issue.
• If you create an object of your own interface (IDevicePixelRatioScaler, and more but I didn't check) not inheriting from QObject, the interface should have a virtual destructor, and not having one is technically UB under the C++ spec (and will probably malfunction/leak in practice if the subclass has more members or destructors than the parent).

I have a branch locally for fixing the leak (which actually worked, no leaks blamed on this program in asan, and no use-after-free caught, hopefully none that exist but weren't caught). But it started out as a mix of adding manual deletes and refactoring to add parent-child relationships, and I ended up finding and fixing more leaks not caught upon merely opening and closing the program, and adding more and more parent-child relationships, which takes more time to refactor than merely adding a delete call...

So my branch is kinda unfinished and not in shape to send a PR (and I don't know whether to submit the minimal patch, or put in more effort and perform a comprehensive code review complete with a planning document to parent-child everything I can find). But I've learned the above things about the codebase while trying to write my branch.

Unfortunately C++ memory management is not compositional, raw pointers don't express responsibility for who has to free them, or how long they're valid for, and there are multiple valid ways (new/delete, new/QObject, unique_ptr RAII) to manage memory self-consistently, and mixing these ways results in a non-self-consistent system causing memory leaks or use-after-free. So I don't know if my branch is actually self-consistent or not, since I didn't document all usages of every class I touched to ensure it's not being misused. ASAN doesn't report any errors, but that just means no errors have been triggered so far.

[DamirPorobic]

Thanks for looking into this. You can provide a PR and I'll have a look and eventually pick up from there.

[nyanpasu64]

I've pushed my unfinished code to https://github.com/nyanpasu64/kImageAnnotator/tree/fix-memory-leaks.

Note that it may not necessarily compile or be logically correct, since it's in the middle of a refactor that I never finished (and don't plan to work on right now). And also the formatting is broken since I replaced tabs with spaces in one file. Nonetheless, some of the changes, such as nyanpasu64@91c2c25#diff-d560f5780ec25e3f6dc40faf966b4742194221f3f93e3cbee017bdfa381364f1, should be correct.

[DamirPorobic]

I have fixed the leaks that have been found by Valgrind. I haven't fixed all deletes and missing parent relations, I'll do that when I come across those places. Thanks for pointing me to the issue and providing a detailed explanation, it gave me a better understanding :)