yonlu/yellowjacket
1
1
Fork 0
Code Issues 55 Pull Requests 6 Actions Packages Projects Releases 12 Wiki Activity

Queue.OnPlaybackFinished indexes q.tracks[currentIndex] without checking the index, which is -1 after the queue is exhausted #126

New Issue
Closed
opened 2026-08-19 12:38:27 +00:00 by yonlu · 1 comment
yonlu commented 2026-08-19 12:38:27 +00:00
Owner
Copy Link
Copy Source

Findings

OnPlaybackFinished reads the finished track before checking the index (backend/queue/handlers.go:9-15):

if len(q.tracks) == 0 {
	q.mu.Unlock()

	return
}

// Capture the track that just finished before advancing.
finishedID := q.tracks[q.currentIndex].AudioFileID

The guard is on the queue being empty, not on currentIndex being valid, and currentIndex == -1 with a non-empty queue is a state this package produces deliberately: onQueueExhausted(false) sets it (queue.go:1394+) and explicitly does not unload the player, so the finished track stays loaded on the now-playing bar. Press play on it from there and a natural finish re-enters this function with currentIndex == -1 against a non-empty q.tracks — an out-of-range index, which panics the process rather than returning.

playFromStart and PlayIndex bounds-check before indexing; loadCurrentTrack bounds-checks both ends (:1285-1292). This path checks neither, and it is the one path reached from a goroutine dispatched by the audio callback, so the panic has no caller to recover it.

Direction

Guard currentIndex the way loadCurrentTrack does — < 0 || >= len(q.tracks) — and return. A table test over (tracks, currentIndex) including (non-empty, -1) and (non-empty, len) pins it.

**Findings** `OnPlaybackFinished` reads the finished track before checking the index (`backend/queue/handlers.go:9-15`): ```go if len(q.tracks) == 0 { q.mu.Unlock() return } // Capture the track that just finished before advancing. finishedID := q.tracks[q.currentIndex].AudioFileID ``` The guard is on the queue being empty, not on `currentIndex` being valid, and `currentIndex == -1` with a non-empty queue is a state this package produces deliberately: `onQueueExhausted(false)` sets it (`queue.go:1394+`) and explicitly does *not* unload the player, so the finished track stays loaded on the now-playing bar. Press play on it from there and a natural finish re-enters this function with `currentIndex == -1` against a non-empty `q.tracks` — an out-of-range index, which panics the process rather than returning. `playFromStart` and `PlayIndex` bounds-check before indexing; `loadCurrentTrack` bounds-checks both ends (`:1285-1292`). This path checks neither, and it is the one path reached from a goroutine dispatched by the audio callback, so the panic has no caller to recover it. **Direction** Guard `currentIndex` the way `loadCurrentTrack` does — `< 0 || >= len(q.tracks)` — and return. A table test over (`tracks`, `currentIndex`) including `(non-empty, -1)` and `(non-empty, len)` pins it.
yonlu self-assigned this 2026-08-19 12:39:10 +00:00
yonlu added the
Status
In Progress
 label 2026-08-19 12:39:10 +00:00
yonlu commented 2026-08-19 12:39:10 +00:00
Author
Owner
Copy Link
Copy Source

Working this as part of a batch off fix/player-playing-state (#122, #123, #124, #125, #126) — all five are in backend/player and backend/queue, and #124 and #125 are the two halves of the same fallback expression, so splitting them across branches would mean three passes over the same twenty lines.

Approach:

  • #122 — done = true on every readAhead exit; bound the underrun fill and return 0, false past it; give the finished path a way to tell "drained" from "failed" so PlaybackFailed is emitted instead of a silent auto-advance.
  • #123 — move the two emits back under p.mu, and capture the chain's trackChangeID in the beep.Callback closure so a stale callback returns without touching the player.
  • #124/#125 — assign p.format in loadFileLocked, reset p.trackLengthMs there, take the replay path's rate from p.format.
  • #126 — bounds-check currentIndex the way loadCurrentTrack already does.

Verified with make test (all three build configurations) plus new unit tests per exit path; the audible half of #124 needs a 48 kHz fixture.

Working this as part of a batch off `fix/player-playing-state` (#122, #123, #124, #125, #126) — all five are in `backend/player` and `backend/queue`, and #124 and #125 are the two halves of the same fallback expression, so splitting them across branches would mean three passes over the same twenty lines. Approach: - **#122** — `done = true` on every `readAhead` exit; bound the underrun fill and return `0, false` past it; give the finished path a way to tell "drained" from "failed" so `PlaybackFailed` is emitted instead of a silent auto-advance. - **#123** — move the two emits back under `p.mu`, and capture the chain's `trackChangeID` in the `beep.Callback` closure so a stale callback returns without touching the player. - **#124/#125** — assign `p.format` in `loadFileLocked`, reset `p.trackLengthMs` there, take the replay path's rate from `p.format`. - **#126** — bounds-check `currentIndex` the way `loadCurrentTrack` already does. Verified with `make test` (all three build configurations) plus new unit tests per exit path; the audible half of #124 needs a 48 kHz fixture.
logan closed this issue 2026-08-19 14:53:46 +00:00
gitea-actions bot removed the
Status
In Progress
 label 2026-08-19 14:55:06 +00:00
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
Area/Design
Area/Downloads
Area/Explore
Area/Library-UI
Area/Metadata
Area/Packaging
Area/Player
Area/Queue
Area/Settings
Area/Shell-Nav
Compat/Breaking
Breaking change that won't be backward compatible
 Kind/Bug
Something is not working
 Kind/Documentation
Documentation changes
 Kind/Enhancement
Improve existing functionality
 Kind/Feature
New functionality
 Kind/Security
This is security issue
 Kind/Testing
Issue or pull request related to testing
 Platform/Android
Platform/Desktop
Priority
Critical
1
The priority is critical
Priority
High
2
The priority is high
Priority
Medium
3
The priority is medium
Priority
Low
4
The priority is low
Reviewed
Confirmed
1
Issue has been confirmed
Reviewed
Duplicate
2
This issue or pull request already exists
Reviewed
Invalid
3
Invalid issue
Reviewed
Won't Fix
3
This issue won't be fixed
Status
Blocked
1
Something is blocking this issue or pull request
Status
Need More Info
2
Feedback is required to reproduce issue or to continue work
Status
Abandoned
3
Somebody has started to work on this but abandoned work
Status
In Progress
Somebody is actively working on this right now
No labels Area/Queue Kind/Bug Platform/Android Platform/Desktop
Priority
Medium
3
Reviewed
Confirmed
1
Milestone
No items
No Milestone
Projects
Clear projects
No projects
Assignees
Clear assignees
logan yonlu
No Assignees yonlu
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: yonlu/yellowjacket#126