Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Return a local copy of m_cuePoints to avaoid a race condition #10976

Merged
merged 1 commit into from Oct 19, 2022

Conversation

daschuer
Copy link
Member

The QList copy constructor seems to be not thread-safe in case it is not already shared.
https://github.com/qt/qtbase/blob/d8efc8d718e3b3a0464f321e740541f5b221a5d6/src/corelib/tools/qlist.h#L809

This may be fix
#10956
#10689
With two backtraces, where a crash originates to the same function.

@daschuer daschuer added this to the 2.3.4 milestone Oct 17, 2022
@uklotzde
Copy link
Contributor

Please note that there are more occurrences were this illegal assumption about implicit sharing was used, e.g. ControlDoublePrivate::getControlAliases().

@uklotzde
Copy link
Contributor

Probably also the access to QFileInfo in Track requires a mutex guard.

@daschuer
Copy link
Member Author

QFileInfo is implemented by QSharedDataPointer which is thread-safe, see: https://doc.qt.io/qt-5/qshareddatapointer.html#details

@uklotzde
Copy link
Contributor

QFileInfo is implemented by QSharedDataPointer which is thread-safe, see: https://doc.qt.io/qt-5/qshareddatapointer.html#details

Thanks for investigating. The Qt documentation is lacking.

@uklotzde
Copy link
Contributor

Nitpick: Explicitly assigning to a temporary, local variable is both redundant and uncommon when using a scoped lock primitive. This is legacy C-style and only causes noise.

@uklotzde
Copy link
Contributor

I recommend to leave comments about the thread-unsafe copy constructors of QList and QHash.

@daschuer
Copy link
Member Author

Done

@ronso0 ronso0 changed the title Return a local copy of m_cuePoints to avaoid a race consition Return a local copy of m_cuePoints to avaoid a race condition Oct 18, 2022
@uklotzde
Copy link
Contributor

This PR should be merged and released asap.

Copy link
Member

@Holzhaus Holzhaus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thank you.

@Holzhaus Holzhaus merged commit 0f63426 into mixxxdj:2.3 Oct 19, 2022
@daschuer daschuer deleted the gh10689 branch January 3, 2023 08:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants