Switch to using shared pointers for TagInfo

[vladisslav2011]

Avoid crash on Qt6 due to use after free.
Fixes #1143
Needs testing.

[yuvadm]

Pulled and tested this fix, looks good AFAICT. There are some preexisting bugs with the tag lists, but no more seg faults.

[pinkavaj]

I'm afraid this only hides the symptoms but does not solve the problem. The question is, how it is even possible the objects are destroyed, when eg. new tag is added. My guess is the QList stores the objects and relocates them once new object is added. So we cannot combine QList and reference its objects by pointers.

[vladisslav2011]

So we cannot combine QList and reference its objects by pointers.

That's exactly the issue. Qt6 QList copies it's members during internal housekeeping and invalidates direct pointers. Dereferencing invalidated pointers results in bookmarks corruption, segfaults, etc... And using shared pointer solves it. We can copy/create/move shared pointers to an object, but the object remains the same and other shared pointer does not get invalidated. So QList stops breaking things and everything starts working as expected.

[pinkavaj]

You are completely right, I have missed the important line, my claim above is invalid. Sorry for confusion.

Just thinking if it really makes sense to use relatively expansive shared_ptr. The objects should not be referenced after deletion from the list, if it is used it is likely a bug. Using shared_ptr will ensure the program will work, except it will use old data. I can imagine this might lead to wierd behaviouir (like using and displaying value from already deleted object).
With unique_ptr, the program will probably crash or it will be possible to identify such a think using valgrind

[pinkavaj]

@argilo Are we expected to also add line to news?

[pinkavaj]

Shared pointers are relatively expansive compared to raw pointers. usedTags is referenced only within this function and tagInfo cannot be deleted during the lifetime of this function, so using shared pointer is unnecessary. Consider using raw pointers here.

[vladisslav2011]

Using raw pointers here makes Gqrx larger and makes code more complicated. You may try it yourself...

[pinkavaj]

To make sure we understand each other, the proposal was to keep the old code (for now) except the necessary changes to make it work with shared pointer, eg. the only change necessary would be somethink like:

             BookmarkInfo& info = m_BookmarkList[iBookmark];
             for(int iTag = 0; iTag < info.tags.size(); ++iTag)
             {
-              TagInfo& tag = *info.tags[iTag];
-              usedTags.insert(&tag);
+              usedTags.insert(info.tags[iTag].get());
             }
         }

[vladisslav2011]

I can't guarantee, that QSet<TagInfo*> will work correctly here (collect tags with unique names). It may be better to change Bookmarks::m_TagList type form QList<TagInfo> to QMap<QString,TagInfo::sptr> and get rid of this loop.

[pinkavaj]

Yep, the non-uniqueness of tags should be solved too, but it seems to me this tries to solve more thinks in one MR. Mey-be separate commit if it is necessary to fix #1143 or separate MR if it can be done independently would help me.

[vladisslav2011]

Switching to a better container may be a different PR.
I do not know, how other people are using tags. Tag management should be improved if someone is using thousands of tags (adding a quick filter to help finding tags/bookmarks may help in this case).

[pinkavaj]

This is completely valid and straightforward but a bit inefficient due to the copy-construction of the shared pointer. This assignment here is for convenience only, to avoid more complicated expressions bellow. Using a reference style (as before the change) is IMHO most convenient. Please consider dereferencing the pointer to reference.

[vladisslav2011]

Thanks. Switching to using a const reference simplifies code a bit. Tests pending...
I forgot to rebase this on top of master. Done.
Qt5: works as before.
Qt6: works as before.

[pinkavaj]

By dereferencing I ment somethink like TagInfo &tag = *tags[iTag]; :)

[vladisslav2011]

No. My latest commit is wrong. I'll remove it.
We should never create a reference to a smart pointer/a reference to a managed object as it defeats the purpose of using smart pointers to prevent use-after-free.

[pinkavaj]

I does not see any way how use-after-free can happend within this function, is it?

[vladisslav2011]

1. I'm thinking about offloading plotter rendering to a background thread.
2. https://schneide.blog/2012/01/30/dont-mix-c-smart-pointers-with-references/

Mixing smart pointers and references is a bad pattern, that should be avoided.

[pinkavaj]

1. Unfortunately I cannot see how this might affect this, because there is no code yet, so I'm afraid it's hard to discuss.
2. Fine article, but it applies to situation where the smart pointer value is passed to another function/location. I belive bad usages like this can be caught during code-review, so using it strictly in local context is IMHO fine.

Nevertheless the situation still can be avoided just by replacing the tag by the whole tags[iTag], so this option can be considered too.

[argilo]

I rebased and resolved merge conflicts so I can test this out.

[argilo]

This seems like a sensible approach, and I verified that it resolves the invalid memory accesses.