From 3c4d8f65896253a82e19adcbe2808a863a99f74f Mon Sep 17 00:00:00 2001 From: Lars-Dominik Braun Date: Thu, 15 Mar 2018 12:53:56 +0100 Subject: 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 --- src/main.c | 48 ++++++++++++++++----------------- src/player.c | 86 ++++++++++++++++++++++++++++++++++++++++++++++++++---------- src/player.h | 54 +++++++++++++++++++++----------------- src/ui.c | 13 ++++++--- src/ui.h | 4 +-- src/ui_act.c | 26 +++++++++--------- 6 files changed, 149 insertions(+), 82 deletions(-) (limited to 'src') 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 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 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 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 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 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 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 -- cgit v1.2.3