Properly protect player struct with mutex

The volatile keyword neither guarantees atomic access nor memory
visibility[1]. Although this is usually not a problem on x86, it is
incorrect to rely on this. Use mutex locks to protect all shared player
variables and enforce memory visibility.

[1] https://wiki.sei.cmu.edu/confluence/display/c/CON02-C.+Do+not+use+volatile+as+a+synchronization+primitive

6 files changed, 149 insertions, 82 deletions

diff --git a/src/main.c b/src/main.c
index 229c606..2a0e3f7 100644
--- a/src/main.c
+++ b/src/main.c
@@ -1,5 +1,5 @@
 /*
-Copyright (c) 2008-2013
+Copyright (c) 2008-2018
 	Lars-Dominik Braun <lars@6xq.net>
 
 Permission is hereby granted, free of charge, to any person obtaining a copy
@@ -252,15 +252,12 @@ static void BarMainStartPlayback (BarApp_t *app, pthread_t *playerThread) {
 			strncmp (curSong->audioUrl, httpPrefix, strlen (httpPrefix)) != 0) {
 		BarUiMsg (&app->settings, MSG_ERR, "Invalid song url.\n");
 	} else {
-		/* setup player */
-		memset (&app->player, 0, sizeof (app->player));
+		player_t * const player = &app->player;
+		BarPlayerReset (player);
 
 		app->player.url = curSong->audioUrl;
 		app->player.gain = curSong->fileGain;
-		app->player.settings = &app->settings;
 		app->player.songDuration = curSong->length;
-		pthread_mutex_init (&app->player.pauseMutex, NULL);
-		pthread_cond_init (&app->player.pauseCond, NULL);
 
 		assert (interrupted == &app->doQuit);
 		interrupted = &app->player.interrupted;
@@ -291,8 +288,6 @@ static void BarMainPlayerCleanup (BarApp_t *app, pthread_t *playerThread) {
 	/* FIXME: pthread_join blocks everything if network connection
 	 * is hung up e.g. */
 	pthread_join (*playerThread, &threadRet);
-	pthread_cond_destroy (&app->player.pauseCond);
-	pthread_mutex_destroy (&app->player.pauseMutex);
 
 	if (threadRet == (void *) PLAYER_RET_OK) {
 		app->playerErrors = 0;
@@ -306,10 +301,10 @@ static void BarMainPlayerCleanup (BarApp_t *app, pthread_t *playerThread) {
 		app->nextStation = NULL;
 	}
 
-	memset (&app->player, 0, sizeof (app->player));
-
 	assert (interrupted == &app->player.interrupted);
 	interrupted = &app->doQuit;
+
+	app->player.mode = PLAYER_DEAD;
 }
 
 /*	print song duration
@@ -317,19 +312,24 @@ static void BarMainPlayerCleanup (BarApp_t *app, pthread_t *playerThread) {
 static void BarMainPrintTime (BarApp_t *app) {
 	unsigned int songRemaining;
 	char sign;
+	player_t * const player = &app->player;
+
+	pthread_mutex_lock (&player->lock);
+	const unsigned int songDuration = player->songDuration;
+	const unsigned int songPlayed = player->songPlayed;
+	pthread_mutex_unlock (&player->lock);
 
-	if (app->player.songPlayed <= app->player.songDuration) {
-		songRemaining = app->player.songDuration - app->player.songPlayed;
+	if (songPlayed <= songDuration) {
+		songRemaining = songDuration - songPlayed;
 		sign = '-';
 	} else {
 		/* longer than expected */
-		songRemaining = app->player.songPlayed - app->player.songDuration;
+		songRemaining = songPlayed - songDuration;
 		sign = '+';
 	}
 	BarUiMsg (&app->settings, MSG_TIME, "%c%02u:%02u/%02u:%02u\r",
 			sign, songRemaining / 60, songRemaining % 60,
-			app->player.songDuration / 60,
-			app->player.songDuration % 60);
+			songDuration / 60, songDuration % 60);
 }
 
 /*	main loop
@@ -351,14 +351,12 @@ static void BarMainLoop (BarApp_t *app) {
 
 	BarMainGetInitialStation (app);
 
-	/* little hack, needed to signal: hey! we need a playlist, but don't
-	 * free anything (there is nothing to be freed yet) */
-	memset (&app->player, 0, sizeof (app->player));
+	player_t * const player = &app->player;
 
 	while (!app->doQuit) {
 		/* song finished playing, clean up things/scrobble song */
-		if (app->player.mode == PLAYER_FINISHED) {
-			if (app->player.interrupted != 0) {
+		if (BarPlayerGetMode (player) == PLAYER_FINISHED) {
+			if (player->interrupted != 0) {
 				app->doQuit = 1;
 			}
 			BarMainPlayerCleanup (app, &playerThread);
@@ -366,7 +364,7 @@ static void BarMainLoop (BarApp_t *app) {
 
 		/* check whether player finished playing and start playing new
 		 * song */
-		if (app->player.mode == PLAYER_DEAD) {
+		if (BarPlayerGetMode (player) == PLAYER_DEAD) {
 			/* what's next? */
 			if (app->playlist != NULL) {
 				PianoSong_t *histsong = app->playlist;
@@ -389,12 +387,12 @@ static void BarMainLoop (BarApp_t *app) {
 		BarMainHandleUserInput (app);
 
 		/* show time */
-		if (app->player.mode == PLAYER_PLAYING) {
+		if (BarPlayerGetMode (player) == PLAYER_PLAYING) {
 			BarMainPrintTime (app);
 		}
 	}
 
-	if (app->player.mode != PLAYER_DEAD) {
+	if (BarPlayerGetMode (player) != PLAYER_DEAD) {
 		pthread_join (playerThread, NULL);
 	}
 }
@@ -433,7 +431,7 @@ int main (int argc, char **argv) {
 	gcry_check_version (NULL);
 	gcry_control (GCRYCTL_DISABLE_SECMEM, 0);
 	gcry_control (GCRYCTL_INITIALIZATION_FINISHED, 0);
-	BarPlayerInit ();
+	BarPlayerInit (&app.player, &app.settings);
 
 	BarSettingsInit (&app.settings);
 	BarSettingsRead (&app.settings);
@@ -502,7 +500,7 @@ int main (int argc, char **argv) {
 	PianoDestroyPlaylist (app.playlist);
 	curl_easy_cleanup (app.http);
 	curl_global_cleanup ();
-	BarPlayerDestroy ();
+	BarPlayerDestroy (&app.player);
 	BarSettingsDestroy (&app.settings);
 
 	/* restore terminal attributes, zsh doesn't need this, bash does... */
diff --git a/src/player.c b/src/player.c
index 9cbee69..977007d 100644
--- a/src/player.c
+++ b/src/player.c
@@ -1,5 +1,5 @@
 /*
-Copyright (c) 2008-2014
+Copyright (c) 2008-2018
 	Lars-Dominik Braun <lars@6xq.net>
 
 Permission is hereby granted, free of charge, to any person obtaining a copy
@@ -66,19 +66,46 @@ static void printError (const BarSettings_t * const settings,
  *	XXX: in theory we can select the filters/formats we want to support, but
  *	this does not work in practise.
  */
-void BarPlayerInit () {
+void BarPlayerInit (player_t * const p, const BarSettings_t * const settings) {
 	ao_initialize ();
 	av_log_set_level (AV_LOG_FATAL);
 	av_register_all ();
 	avfilter_register_all ();
 	avformat_network_init ();
+
+	pthread_mutex_init (&p->lock, NULL);
+	pthread_cond_init (&p->cond, NULL);
+	BarPlayerReset (p);
+	p->settings = settings;
 }
 
-void BarPlayerDestroy () {
+void BarPlayerDestroy (player_t * const p) {
+	pthread_cond_destroy (&p->cond);
+	pthread_mutex_destroy (&p->lock);
+
 	avformat_network_deinit ();
 	ao_shutdown ();
 }
 
+void BarPlayerReset (player_t * const p) {
+	p->doQuit = false;
+	p->doPause = false;
+	p->songDuration = 0;
+	p->songPlayed = 0;
+	p->mode = PLAYER_DEAD;
+	p->fvolume = NULL;
+	p->fgraph = NULL;
+	p->fctx = NULL;
+	p->st = NULL;
+	p->cctx = NULL;
+	p->fbufsink = NULL;
+	p->fabuf = NULL;
+	p->streamIdx = -1;
+	p->lastTimestamp = 0;
+	p->interrupted = 0;
+	p->aoDev = NULL;
+}
+
 /*	Update volume filter
  */
 void BarPlayerSetVolume (player_t * const player) {
@@ -122,7 +149,9 @@ static int intCb (void * const data) {
 	assert (player != NULL);
 	if (player->interrupted > 1) {
 		/* got a sigint multiple times, quit pianobar (handled by main.c). */
+		pthread_mutex_lock (&player->lock);
 		player->doQuit = true;
+		pthread_mutex_unlock (&player->lock);
 		return 1;
 	} else if (player->interrupted != 0) {
 		/* the request is retried with the same player context */
@@ -190,9 +219,12 @@ static bool openStream (player_t * const player) {
 		av_seek_frame (player->fctx, player->streamIdx, player->lastTimestamp, 0);
 	}
 
-	player->songPlayed = 0;
-	player->songDuration = av_q2d (player->st->time_base) *
+	const unsigned int songDuration = av_q2d (player->st->time_base) *
 			(double) player->st->duration;
+	pthread_mutex_lock (&player->lock);
+	player->songPlayed = 0;
+	player->songDuration = songDuration;
+	pthread_mutex_unlock (&player->lock);
 
 	return true;
 }
@@ -283,6 +315,29 @@ static bool openDevice (player_t * const player) {
 	return true;
 }
 
+/*	Operating on shared variables and must be protected by mutex
+ */
+
+static bool shouldQuit (player_t * const player) {
+	pthread_mutex_lock (&player->lock);
+	const bool ret = player->doQuit;
+	pthread_mutex_unlock (&player->lock);
+	return ret;
+}
+
+static void changeMode (player_t * const player, unsigned int mode) {
+	pthread_mutex_lock (&player->lock);
+	player->mode = mode;
+	pthread_mutex_unlock (&player->lock);
+}
+
+BarPlayerMode BarPlayerGetMode (player_t * const player) {
+	pthread_mutex_lock (&player->lock);
+	const BarPlayerMode ret = player->mode;
+	pthread_mutex_unlock (&player->lock);
+	return ret;
+}
+
 /*	decode and play stream. returns 0 or av error code.
  */
 static int play (player_t * const player) {
@@ -302,7 +357,7 @@ static int play (player_t * const player) {
 
 	enum { FILL, DRAIN, DONE } drainMode = FILL;
 	int ret = 0;
-	while (!player->doQuit && drainMode != DONE) {
+	while (!shouldQuit (player) && drainMode != DONE) {
 		if (drainMode == FILL) {
 			ret = av_read_frame (player->fctx, &pkt);
 			if (ret == AVERROR_EOF) {
@@ -323,17 +378,17 @@ static int play (player_t * const player) {
 		}
 
 		/* pausing */
-		pthread_mutex_lock (&player->pauseMutex);
+		pthread_mutex_lock (&player->lock);
 		if (player->doPause) {
 			av_read_pause (player->fctx);
 			do {
-				pthread_cond_wait (&player->pauseCond, &player->pauseMutex);
+				pthread_cond_wait (&player->cond, &player->lock);
 			} while (player->doPause);
 			av_read_play (player->fctx);
 		}
-		pthread_mutex_unlock (&player->pauseMutex);
+		pthread_mutex_unlock (&player->lock);
 
-		while (!player->doQuit) {
+		while (!shouldQuit (player)) {
 			ret = avcodec_receive_frame (cctx, frame);
 			if (ret == AVERROR_EOF) {
 				/* done draining */
@@ -367,7 +422,10 @@ static int play (player_t * const player) {
 			}
 		}
 
-		player->songPlayed = av_q2d (player->st->time_base) * (double) pkt.pts;
+		const unsigned int songPlayed = av_q2d (player->st->time_base) * (double) pkt.pts;
+		pthread_mutex_lock (&player->lock);
+		player->songPlayed = songPlayed;
+		pthread_mutex_unlock (&player->lock);
 		player->lastTimestamp = pkt.pts;
 
 		av_packet_unref (&pkt);
@@ -410,7 +468,7 @@ void *BarPlayerThread (void *data) {
 		retry = false;
 		if (openStream (player)) {
 			if (openFilter (player) && openDevice (player)) {
-				player->mode = PLAYER_PLAYING;
+				changeMode (player, PLAYER_PLAYING);
 				BarPlayerSetVolume (player);
 				retry = play (player) == AVERROR_INVALIDDATA &&
 						!player->interrupted;
@@ -422,11 +480,11 @@ void *BarPlayerThread (void *data) {
 			/* stream not found */
 			pret = PLAYER_RET_SOFTFAIL;
 		}
-		player->mode = PLAYER_WAITING;
+		changeMode (player, PLAYER_WAITING);
 		finish (player);
 	} while (retry);
 
-	player->mode = PLAYER_FINISHED;
+	changeMode (player, PLAYER_FINISHED);
 
 	return (void *) pret;
 }
diff --git a/src/player.h b/src/player.h
index 0213fde..30107f1 100644
--- a/src/player.h
+++ b/src/player.h
@@ -1,5 +1,5 @@
 /*
-Copyright (c) 2008-2014
+Copyright (c) 2008-2018
 	Lars-Dominik Braun <lars@6xq.net>
 
 Permission is hereby granted, free of charge, to any person obtaining a copy
@@ -39,23 +39,31 @@ THE SOFTWARE.
 
 #include "settings.h"
 
+typedef enum {
+	/* not running */
+	PLAYER_DEAD = 0,
+	/* running, but not ready to play music yet */
+	PLAYER_WAITING,
+	/* currently playing a song */
+	PLAYER_PLAYING,
+	/* finished playing a song */
+	PLAYER_FINISHED,
+} BarPlayerMode;
+
 typedef struct {
-	/* protected by pauseMutex */
-	volatile bool doQuit;
-	volatile bool doPause;
-	pthread_mutex_t pauseMutex;
-	pthread_cond_t pauseCond;
-
-	enum {
-		/* not running */
-		PLAYER_DEAD = 0,
-		/* running, but not ready to play music yet */
-		PLAYER_WAITING,
-		/* currently playing a song */
-		PLAYER_PLAYING,
-		/* finished playing a song */
-		PLAYER_FINISHED,
-	} mode;
+	/* public attributes protected by mutex */
+	pthread_mutex_t lock;
+	pthread_cond_t cond; /* broadcast changes to doPause */
+
+	bool doQuit, doPause;
+
+	/* measured in seconds */
+	unsigned int songDuration;
+	unsigned int songPlayed;
+
+	BarPlayerMode mode;
+
+	/* private attributes _not_ protected by mutex */
 
 	/* libav */
 	AVFilterContext *fvolume;
@@ -70,20 +78,18 @@ typedef struct {
 
 	ao_device *aoDev;
 
-	/* settings */
+	/* settings (must be set before starting the thread) */
 	double gain;
 	char *url;
 	const BarSettings_t *settings;
-
-	/* measured in seconds */
-	volatile unsigned int songDuration;
-	volatile unsigned int songPlayed;
 } player_t;
 
 enum {PLAYER_RET_OK = 0, PLAYER_RET_HARDFAIL = 1, PLAYER_RET_SOFTFAIL = 2};
 
 void *BarPlayerThread (void *data);
 void BarPlayerSetVolume (player_t * const player);
-void BarPlayerInit ();
-void BarPlayerDestroy ();
+void BarPlayerInit (player_t * const p, const BarSettings_t * const settings);
+void BarPlayerReset (player_t * const p);
+void BarPlayerDestroy (player_t * const p);
+BarPlayerMode BarPlayerGetMode (player_t * const player);
 
diff --git a/src/ui.c b/src/ui.c
index 0512f36..5c4e42d 100644
--- a/src/ui.c
+++ b/src/ui.c
@@ -1,5 +1,5 @@
 /*
-Copyright (c) 2008-2013
+Copyright (c) 2008-2018
 	Lars-Dominik Braun <lars@6xq.net>
 
 Permission is hereby granted, free of charge, to any person obtaining a copy
@@ -814,7 +814,7 @@ size_t BarUiListSongs (const BarApp_t * const app,
  */
 void BarUiStartEventCmd (const BarSettings_t *settings, const char *type,
 		const PianoStation_t *curStation, const PianoSong_t *curSong,
-		const player_t * const player, PianoStation_t *stations,
+		player_t * const player, PianoStation_t *stations,
 		PianoReturn_t pRet, CURLcode wRet) {
 	pid_t chld;
 	int pipeFd[2];
@@ -855,6 +855,11 @@ void BarUiStartEventCmd (const BarSettings_t *settings, const char *type,
 			songStation = PianoFindStationById (stations, curSong->stationId);
 		}
 
+		pthread_mutex_lock (&player->lock);
+		const unsigned int songDuration = player->songDuration;
+		const unsigned int songPlayed = player->songPlayed;
+		pthread_mutex_unlock (&player->lock);
+
 		fprintf (pipeWriteFd,
 				"artist=%s\n"
 				"title=%s\n"
@@ -880,8 +885,8 @@ void BarUiStartEventCmd (const BarSettings_t *settings, const char *type,
 				PianoErrorToStr (pRet),
 				wRet,
 				curl_easy_strerror (wRet),
-				player->songDuration,
-				player->songPlayed,
+				songDuration,
+				songPlayed,
 				curSong == NULL ? PIANO_RATE_NONE : curSong->rating,
 				curSong == NULL ? "" : curSong->detailUrl
 				);
diff --git a/src/ui.h b/src/ui.h
index 9081b71..da6bd8e 100644
--- a/src/ui.h
+++ b/src/ui.h
@@ -1,5 +1,5 @@
 /*
-Copyright (c) 2008-2011
+Copyright (c) 2008-2018
 	Lars-Dominik Braun <lars@6xq.net>
 
 Permission is hereby granted, free of charge, to any person obtaining a copy
@@ -48,7 +48,7 @@ void BarUiPrintSong (const BarSettings_t *, const PianoSong_t *,
 size_t BarUiListSongs (const BarApp_t * const app,
 		const PianoSong_t *song, const char *filter);
 void BarUiStartEventCmd (const BarSettings_t *, const char *,
-		const PianoStation_t *, const PianoSong_t *, const player_t *,
+		const PianoStation_t *, const PianoSong_t *, player_t *,
 		PianoStation_t *, PianoReturn_t, CURLcode);
 bool BarUiPianoCall (BarApp_t * const, const PianoRequestType_t,
 		void *, PianoReturn_t *, CURLcode *);
diff --git a/src/ui_act.c b/src/ui_act.c
index 2c5f264..d112749 100644
--- a/src/ui_act.c
+++ b/src/ui_act.c
@@ -1,5 +1,5 @@
 /*
-Copyright (c) 2008-2013
+Copyright (c) 2008-2018
 	Lars-Dominik Braun <lars@6xq.net>
 
 Permission is hereby granted, free of charge, to any person obtaining a copy
@@ -52,11 +52,11 @@ THE SOFTWARE.
 static inline void BarUiDoSkipSong (player_t * const player) {
 	assert (player != NULL);
 
-	pthread_mutex_lock (&player->pauseMutex);
+	pthread_mutex_lock (&player->lock);
 	player->doQuit = true;
 	player->doPause = false;
-	pthread_cond_broadcast (&player->pauseCond);
-	pthread_mutex_unlock (&player->pauseMutex);
+	pthread_cond_broadcast (&player->cond);
+	pthread_mutex_unlock (&player->lock);
 }
 
 /*	transform station if necessary to allow changes like rename, rate, ...
@@ -419,28 +419,28 @@ BarUiActCallback(BarUiActSkipSong) {
 /*	play
  */
 BarUiActCallback(BarUiActPlay) {
-	pthread_mutex_lock (&app->player.pauseMutex);
+	pthread_mutex_lock (&app->player.lock);
 	app->player.doPause = false;
-	pthread_cond_broadcast (&app->player.pauseCond);
-	pthread_mutex_unlock (&app->player.pauseMutex);
+	pthread_cond_broadcast (&app->player.cond);
+	pthread_mutex_unlock (&app->player.lock);
 }
 
 /*	pause
  */
 BarUiActCallback(BarUiActPause) {
-	pthread_mutex_lock (&app->player.pauseMutex);
+	pthread_mutex_lock (&app->player.lock);
 	app->player.doPause = true;
-	pthread_cond_broadcast (&app->player.pauseCond);
-	pthread_mutex_unlock (&app->player.pauseMutex);
+	pthread_cond_broadcast (&app->player.cond);
+	pthread_mutex_unlock (&app->player.lock);
 }
 
 /*	toggle pause
  */
 BarUiActCallback(BarUiActTogglePause) {
-	pthread_mutex_lock (&app->player.pauseMutex);
+	pthread_mutex_lock (&app->player.lock);
 	app->player.doPause = !app->player.doPause;
-	pthread_cond_broadcast (&app->player.pauseCond);
-	pthread_mutex_unlock (&app->player.pauseMutex);
+	pthread_cond_broadcast (&app->player.cond);
+	pthread_mutex_unlock (&app->player.lock);
 }
 
 /*	rename current station