Fix remaining mpv thread safety gaps causing SIGSEGV#138
Merged
Conversation
The previous fix (#137) added mutex protection to all public Engine methods but missed two GetProperty calls inside the event handler and left a shutdown race where TerminateDestroy could run while other goroutines were still using the mpv handle. Event handler: wrap the two unprotected e.handle.GetProperty calls at EndFileError and EndFileEOF/EndFileStop with the engine mutex so they serialise against concurrent callers from the ticker, D-Bus, and frontend goroutines. Shutdown race: Engine.Close now holds the mutex through TerminateDestroy so no concurrent caller can access the handle mid-destroy. All public methods check a closed flag and bail out early after Close has run. ServiceShutdown waits for the ticker goroutine to fully exit before calling engine.Close.
Scrobble state: add scrobbleMu to protect scrobbleTrackID, scrobbleElapsed, and scrobbled fields which are written from event callbacks (startScrobbleTracking) and read/written from the ticker goroutine (checkScrobble). Snapshot elapsed time before releasing the lock so DB/network calls happen outside the critical section. MPRIS: add closed flag checked by setProp, UpdatePosition, exportArtwork, SeekOffset, and SetPosition. Close() sets the flag under the mutex before closing the D-Bus connection, preventing concurrent conn.Emit() calls from racing with conn.Close(). Protect m.artPath writes and snapshot m.artDir in exportArtwork. Notifier: add closed flag checked by Notify(). Close() sets the flag under the mutex before closing the D-Bus connection, preventing concurrent Notify() calls from operating on a closed connection.
Go 1.25.7 has two stdlib vulnerabilities (GO-2026-4602 in os, GO-2026-4601 in net/url) that cause the Security CI job to fail. Both are fixed in 1.25.8.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
mpv engine (commit 1)
e.handle.GetProperty("playlist-pos", ...)calls in the event handler (EndFileErrorandEndFileEOF/EndFileStopbranches) with the engine mutex, closing a race against concurrent callers from the ticker, D-Bus, and frontend goroutinesEngine.Close()now holds the mutex throughTerminateDestroy()so no concurrent caller can access the handle mid-destroyclosedflag and bail out early afterClose()has runServiceShutdownwaits for the ticker goroutine to fully exit (viatickerDonechannel) before callingengine.Close()Scrobble state, MPRIS, notifier (commit 2)
scrobbleMuprotectingscrobbleTrackID,scrobbleElapsed, andscrobbledfields - these were read/written from both the event callback goroutine and the ticker goroutine with no synchronisationclosedflag checked bysetProp,UpdatePosition,exportArtwork,SeekOffset, andSetPosition- preventsconn.Emit()racing withconn.Close()during shutdownclosedflag checked byNotify()- prevents D-Bus calls on a closed connection during shutdownFollows up on #137 which added mutex protection to public Engine methods but missed these internal paths and adjacent components.
Test plan
go test -tags nocgo ./...passesgolangci-lint run --build-tags nocgoreports 0 issuesnpm run checkpasses (no frontend changes, sanity check)