summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorLars-Dominik Braun <lars@6xq.net>2018-03-15 12:53:56 +0100
committerLars-Dominik Braun <lars@6xq.net>2018-03-15 12:53:56 +0100
commit3c4d8f65896253a82e19adcbe2808a863a99f74f (patch)
treefc7c3fbe15027e752c8a7da3d42c9a37d4d72c87
parent2e51a13fe816c0c0b02f7d7a19a4c739dcb66119 (diff)
downloadpianobar-3c4d8f65896253a82e19adcbe2808a863a99f74f.tar.gz
pianobar-3c4d8f65896253a82e19adcbe2808a863a99f74f.tar.bz2
pianobar-3c4d8f65896253a82e19adcbe2808a863a99f74f.zip
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
-rw-r--r--src/main.c48
-rw-r--r--src/player.c86
-rw-r--r--src/player.h54
-rw-r--r--src/ui.c13
-rw-r--r--src/ui.h4
-rw-r--r--src/ui_act.c26
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