Skip to content

Fix null-pointer dereference in SystemUtils::getSurroundingBarlines#559

Open
primoszj wants to merge 1 commit into
powertab:masterfrom
primoszj:fix-null-deref-surrounding-barlines
Open

Fix null-pointer dereference in SystemUtils::getSurroundingBarlines#559
primoszj wants to merge 1 commit into
powertab:masterfrom
primoszj:fix-null-deref-surrounding-barlines

Conversation

@primoszj
Copy link
Copy Markdown

Summary

SystemUtils::getSurroundingBarlines() guards next_bar with assert(next_bar) and then dereferences it unconditionally. In Release builds the assert is compiled out, so the null is dereferenced and the program crashes with SIGSEGV inside Barline::getPosition() the first time the MIDI player asks for the surrounding barlines at a position that has no strictly-later barline.

Reproduction

  1. Build in Release mode.
  2. Open a score where playback flow crosses a repeat / direction / alternate ending whose target lands on a system's end barline.
  3. Press Play.

Crash (Release build):

Barline::getPosition() const          source/score/barline.cpp:50
MidiFile::load(...)                   source/midi/midifile.cpp:174
MidiPlayer::playScore(...)            source/audio/midiplayer.cpp:332
QObject::event(QEvent*)               libQt6Core
...

Root cause

MidiFile::load() iterates with:

auto [current_bar, next_bar] =
    SystemUtils::getSurroundingBarlines(system, location.getPosition());

moveToNextBar() has an early-return path: if findPositionChange() handles a repeat / direction / alternate ending, the function returns before the "advance to next system" branch. The returned location can therefore sit exactly on a system's end barline. On the next iteration, System::getNextBarline() correctly returns nullptr (no barline strictly past the end), but getSurroundingBarlines() dereferences it anyway. In Debug builds the assert catches it; in Release builds it's a hard crash.

current_bar has the same shape of bug if position precedes all barlines (no exact match and no previous barline), though that path is harder to reach.

Fix

Treat "no strictly-later barline" as a legitimate end-of-system state rather than an invariant violation. Fall back to the system's end barline so that MidiFile::load() sees an empty [current, next) range, emits no events for that bar, and moveToNextBar() advances to the next system on the following iteration. Guard current_bar symmetrically.

Every System is constructed with a start and an end barline, so front() and back() are always valid.

Test plan

  • Reproduced the crash in a local Release build (Linux, Qt 6).
  • Patched, rebuilt, replayed the same score — playback proceeds without crashing.
  • (Maintainer) consider a unit test that calls getSurroundingBarlines(system, end_bar_position) on a fresh System and asserts the returned pair refers to the end barline rather than crashing.

On branch fix-null-deref-surrounding-barlines
 Changes to be committed:
	modified:   source/score/system.cpp
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant