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.

Sign up for GitHub

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

Jump to bottom

Consistent race condition/use-after-free in ~MusixmatchLyricsProvider #1239

Closed
LebedevRI opened this issue Jul 18, 2023 · 4 comments
Closed

Consistent race condition/use-after-free in ~MusixmatchLyricsProvider #1239

LebedevRI opened this issue Jul 18, 2023 · 4 comments
Assignees
@jonaski
Labels
bug confirmed

Comments

@LebedevRI
Copy link
Contributor

Due to the lazy destructors, NetworkAccessManager may get actually fully destructed
before MusixmatchLyricsProvider will be (through LyricsProviders).

But if MusixmatchLyricsProvider had just sent an lyrics request,
and did not get a reply yet, it will have a QNetworkReply* in replies_,
and when NetworkAccessManager destructs, the object, to which that pointer points to,
is also destructed. And once ~MusixmatchLyricsProvider() runs later,
it tries to operate on freed memory...

Locally, this very consistently (100% of the strawberry runs) results in a crash.

I've looked at this, and i don't quite understand how this is supposed to be resolved.
LebedevRI added a commit to LebedevRI/strawberry that referenced this issue Jul 18, 2023
@LebedevRI
Application::~Application(): manually delete lyrics providers
f5ca237 
This must be done before the network manager is deleted.
Due to the lazy destruction during `Lazy<>.reset()`,
this appears to be one of the approaches that solves that.

Fixes strawberrymusicplayer#1239
@LebedevRI LebedevRI mentioned this issue Jul 18, 2023
Closed
@LebedevRI
Copy link
Contributor Author

LebedevRI commented Jul 18, 2023 •
edited
Loading

Actually, never mind, this appears to work: #1240
... but is rather intrusive. Is that really the right fix here?

@jonaski
Copy link
Member

jonaski commented Jul 20, 2023

I think everything under Application should be shared pointers so objects are deleted in the correct order, I'm working on it.

@jonaski jonaski self-assigned this Jul 20, 2023
@LebedevRI
Copy link
Contributor Author

Yes, that would be better indeed.
Thank you.

@LebedevRI
Copy link
Contributor Author

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@jonaski jonaski

Labels
bug confirmed
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Application::~Application(): manually delete lyrics providers LebedevRI/strawberry
2 participants
@LebedevRI @jonaski