Merge pull request #58 from mr-light-show/fix/reconnect-playlist-advance-race

Fix heap corruption when reconnect races playlist advance

Author: mr-light-show

src/bar_state.c

@@ -264,6 +264,24 @@ void BarStateSetPlaylist(BarApp_t *app, PianoSong_t *playlist) {
 	BarStateSignalPlaybackManager(app);
 }
 
+/*	Advance playlist (pop head for history) (thread-safe)
+ *	Returns the finished song detached from the list, or NULL if empty.
+ */
+PianoSong_t *BarStateAdvancePlaylist(BarApp_t *app) {
+	assert(app != NULL);
+
+	PianoSong_t *finished = NULL;
+	WITH_STATE_LOCK(app, "AdvancePlaylist", NULL) {
+		if (app->playlist != NULL) {
+			finished = app->playlist;
+			app->playlist = PianoListNextP(app->playlist);
+			finished->head.next = NULL;
+		}
+	}
+	BarStateSignalPlaybackManager(app);
+	return finished;
+}
+
 /*	Drain playlist (destroy and clear) (thread-safe)
  */
 void BarStateDrainPlaylist(BarApp_t *app) {

src/bar_state.h

@@ -185,6 +185,8 @@ PianoStation_t *BarStateGetStationList(const BarApp_t *app);
 /* Playlist access (thread-safe) */
 PianoSong_t *BarStateGetPlaylist(const BarApp_t *app);
 void BarStateSetPlaylist(BarApp_t *app, PianoSong_t *playlist);
+/* Pop head song for history; NULL if playlist empty or already drained. */
+PianoSong_t *BarStateAdvancePlaylist(BarApp_t *app);
 void BarStateDrainPlaylist(BarApp_t *app);
 void BarStateSwitchStation(BarApp_t *app, PianoStation_t *station);
 

src/main.c

@@ -524,14 +524,11 @@ static void BarMainLoop (BarApp_t *app) {
 		 * song */
 		if (BarPlayerGetMode (player) == PLAYER_DEAD) {
 			/* what's next? */
-			PianoSong_t *playlist = BarStateGetPlaylist(app);
-			if (playlist != NULL) {
-				PianoSong_t *histsong = playlist;
-				BarStateSetPlaylist(app, PianoListNextP (playlist));
-				histsong->head.next = NULL;
-				BarUiHistoryPrepend (app, histsong);
+			PianoSong_t *finished = BarStateAdvancePlaylist(app);
+			if (finished != NULL) {
+				BarUiHistoryPrepend (app, finished);
 			}
-			playlist = BarStateGetPlaylist(app);
+			PianoSong_t *playlist = BarStateGetPlaylist(app);
 			PianoStation_t *nextStation = BarStateGetNextStation(app);
 			if (playlist == NULL && nextStation != NULL && !app->doQuit) {
 				PianoStation_t *curStation = BarStateGetCurrentStation(app);

src/playback_manager.c

@@ -270,17 +270,14 @@ static void *BarPlaybackManagerThread(void *data) {
 				log_write(DEBUG_UI, "PlaybackMgr: Player idle\n");
 			}
 
-			/* Advance playlist */
-			PianoSong_t *playlist = BarStateGetPlaylist(app);
-			if (playlist != NULL) {
-				PianoSong_t *histsong = playlist;
-				BarStateSetPlaylist(app, PianoListNextP(playlist));
-				histsong->head.next = NULL;
-				BarUiHistoryPrepend(app, histsong);
+			/* Advance playlist (atomic under state lock — safe vs DrainPlaylist) */
+			PianoSong_t *finished = BarStateAdvancePlaylist(app);
+			if (finished != NULL) {
+				BarUiHistoryPrepend(app, finished);
 			}
-			
+
 			/* Fetch more songs if needed */
-			playlist = BarStateGetPlaylist(app);
+			PianoSong_t *playlist = BarStateGetPlaylist(app);
 			PianoStation_t *nextStation = BarStateGetNextStation(app);
 
 			if (playlist == NULL && nextStation != NULL && !app->doQuit) {

test/unit/test_bar_state.c

@@ -331,6 +331,104 @@ START_TEST(test_bar_state_drain_playlist) {
 }
 END_TEST
 
+START_TEST(test_bar_state_advance_playlist) {
+	BarApp_t app;
+	PianoSong_t song1, song2;
+	memset(&song1, 0, sizeof(song1));
+	memset(&song2, 0, sizeof(song2));
+	song1.head.next = &song2.head;
+	song1.title = (char *)"first";
+	song2.title = (char *)"second";
+	bar_state_test_setup(&app, BAR_UI_MODE_WEB);
+
+	BarStateSetPlaylist(&app, &song1);
+
+	PianoSong_t *finished = BarStateAdvancePlaylist(&app);
+	ck_assert_ptr_eq(finished, &song1);
+	ck_assert_ptr_null(finished->head.next);
+	ck_assert_ptr_eq(BarStateGetPlaylist(&app), &song2);
+
+	finished = BarStateAdvancePlaylist(&app);
+	ck_assert_ptr_eq(finished, &song2);
+	ck_assert_ptr_null(BarStateGetPlaylist(&app));
+
+	finished = BarStateAdvancePlaylist(&app);
+	ck_assert_ptr_null(finished);
+
+	bar_state_test_teardown(&app);
+}
+END_TEST
+
+START_TEST(test_bar_state_advance_playlist_both) {
+	BarApp_t app;
+	PianoSong_t song;
+	memset(&song, 0, sizeof(song));
+	song.title = (char *)"both-mode";
+	bar_state_test_setup(&app, BAR_UI_MODE_BOTH);
+
+	BarStateSetPlaylist(&app, &song);
+	PianoSong_t *finished = BarStateAdvancePlaylist(&app);
+	ck_assert_ptr_eq(finished, &song);
+	ck_assert_ptr_null(BarStateGetPlaylist(&app));
+
+	bar_state_test_teardown(&app);
+}
+END_TEST
+
+typedef struct {
+	BarApp_t *app;
+	int iterations;
+} bar_state_playlist_thread_args_t;
+
+static void *advance_playlist_worker(void *arg) {
+	const bar_state_playlist_thread_args_t *args = arg;
+	for (int i = 0; i < args->iterations; ++i) {
+		PianoSong_t *finished = BarStateAdvancePlaylist(args->app);
+		(void) finished;
+	}
+	return NULL;
+}
+
+static void *drain_playlist_worker(void *arg) {
+	const bar_state_playlist_thread_args_t *args = arg;
+	for (int i = 0; i < args->iterations; ++i) {
+		BarStateDrainPlaylist(args->app);
+	}
+	return NULL;
+}
+
+/* Regression: reconnect/disconnect DrainPlaylist vs playback-manager advance. */
+START_TEST(test_bar_state_advance_playlist_race_with_drain) {
+	BarApp_t app;
+	bar_state_test_setup(&app, BAR_UI_MODE_WEB);
+
+	for (int round = 0; round < 50; ++round) {
+		PianoSong_t *s1 = calloc(1, sizeof(*s1));
+		PianoSong_t *s2 = calloc(1, sizeof(*s2));
+		ck_assert_ptr_nonnull(s1);
+		ck_assert_ptr_nonnull(s2);
+		s1->head.next = &s2->head;
+		BarStateSetPlaylist(&app, s1);
+
+		bar_state_playlist_thread_args_t args = { .app = &app, .iterations = 200 };
+		pthread_t advance_tid;
+		pthread_t drain_tid;
+		ck_assert_int_eq(pthread_create(&advance_tid, NULL,
+		                                advance_playlist_worker, &args), 0);
+		ck_assert_int_eq(pthread_create(&drain_tid, NULL,
+		                                drain_playlist_worker, &args), 0);
+		ck_assert_int_eq(pthread_join(advance_tid, NULL), 0);
+		ck_assert_int_eq(pthread_join(drain_tid, NULL), 0);
+
+		if (BarStateGetPlaylist(&app) != NULL) {
+			BarStateDrainPlaylist(&app);
+		}
+	}
+
+	bar_state_test_teardown(&app);
+}
+END_TEST
+
 /* SwitchStation: drains playlist and sets nextStation (test with playlist NULL) */
 START_TEST(test_bar_state_switch_station) {
 	BarApp_t app;
@@ -657,6 +755,9 @@ Suite *bar_state_suite(void) {
 	tcase_add_test(tc_playlist, test_bar_state_playlist_get_set);
 	tcase_add_test(tc_playlist, test_bar_state_playlist_get_set_web);
 	tcase_add_test(tc_playlist, test_bar_state_drain_playlist);
+	tcase_add_test(tc_playlist, test_bar_state_advance_playlist);
+	tcase_add_test(tc_playlist, test_bar_state_advance_playlist_both);
+	tcase_add_test(tc_playlist, test_bar_state_advance_playlist_race_with_drain);
 	tcase_add_test(tc_playlist, test_bar_state_switch_station);
 	suite_add_tcase(s, tc_playlist);
 

test/unit/test_playback_manager.c

@@ -193,6 +193,40 @@ START_TEST(test_manager_thread_one_loop_iteration) {
 }
 END_TEST
 
+/* Covers idle PLAYER_DEAD path: BarStateAdvancePlaylist + BarUiHistoryPrepend. */
+START_TEST(test_manager_idle_advances_playlist) {
+	BarApp_t app;
+	PianoSong_t song;
+
+	memset(&song, 0, sizeof(song));
+	song.title = (char *)"idle-advance";
+
+	memset(&app, 0, sizeof(app));
+	app.settings.uiMode = BAR_UI_MODE_WEB;
+	app.settings.history = 10;
+	BarStateInit(&app);
+	pthread_mutex_init(&app.player.lock, NULL);
+	pthread_cond_init(&app.player.cond, NULL);
+	app.player.mode = PLAYER_DEAD;
+	BarStateSetPlaylist(&app, &song);
+
+	ck_assert(BarPlaybackManagerStart(&app));
+	pthread_mutex_lock(&app.player.lock);
+	pthread_cond_broadcast(&app.player.cond);
+	pthread_mutex_unlock(&app.player.lock);
+	usleep(100000);
+
+	BarPlaybackManagerStop(&app);
+
+	ck_assert_ptr_null(BarStateGetPlaylist(&app));
+	ck_assert_ptr_eq(app.songHistory, &song);
+
+	pthread_mutex_destroy(&app.player.lock);
+	pthread_cond_destroy(&app.player.cond);
+	BarStateDestroy(&app);
+}
+END_TEST
+
 START_TEST(test_should_park_idle_when_dead_and_empty) {
 	BarApp_t app;
 	memset(&app, 0, sizeof(app));
@@ -301,6 +335,7 @@ Suite *playback_manager_suite(void) {
 	tcase_add_test(tc, test_complete_song_cleanup_interrupt_on_quit);
 	tcase_add_test(tc, test_complete_song_cleanup_no_interrupt_log_when_not_quitting);
 	tcase_add_test(tc, test_manager_thread_one_loop_iteration);
+	tcase_add_test(tc, test_manager_idle_advances_playlist);
 	tcase_add_test(tc, test_should_park_idle_when_dead_and_empty);
 	tcase_add_test(tc, test_should_not_park_when_next_station_set);
 	tcase_add_test(tc, test_should_not_park_when_not_dead);