From 19ff6042cf979837149e8745265df65840432411 Mon Sep 17 00:00:00 2001 From: Belbo Date: Sun, 16 Jul 2017 18:31:24 +0200 Subject: [PATCH 1/5] Prevent buffer overflow in log functions die(), warn(), debug() and inform() write the string into a limited internal buffer first. They now use vsnprintf() instead of vsprintf() to prevent an overflow of that buffer. --- common.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/common.c b/common.c index 1bea3a91..2949ebbb 100644 --- a/common.c +++ b/common.c @@ -101,7 +101,7 @@ void die(const char *format, ...) { s[0] = 0; va_list args; va_start(args, format); - vsprintf(s, format, args); + vsnprintf(s, sizeof(s), format, args); va_end(args); daemon_log(LOG_EMERG, "fatal error: %s", s); shairport_shutdown(); @@ -113,7 +113,7 @@ void warn(const char *format, ...) { s[0] = 0; va_list args; va_start(args, format); - vsprintf(s, format, args); + vsnprintf(s, sizeof(s), format, args); va_end(args); daemon_log(LOG_WARNING, "%s", s); } @@ -125,7 +125,7 @@ void debug(int level, const char *format, ...) { s[0] = 0; va_list args; va_start(args, format); - vsprintf(s, format, args); + vsnprintf(s, sizeof(s), format, args); va_end(args); daemon_log(LOG_DEBUG, "%s", s); } @@ -135,7 +135,7 @@ void inform(const char *format, ...) { s[0] = 0; va_list args; va_start(args, format); - vsprintf(s, format, args); + vsnprintf(s, sizeof(s), format, args); va_end(args); daemon_log(LOG_INFO, "%s", s); } From a704b2fe0b436a8fd4c3e1d6f1e137daaa84ae01 Mon Sep 17 00:00:00 2001 From: Belbo Date: Mon, 7 Aug 2017 23:42:51 +0200 Subject: [PATCH 2/5] handle_announce() waits for playing_conn to stop if it's already shutting down - If playing_conn is already stopping, handle_announce() waits one second for it to stop. This gives the current RTSP connection a better chance to get the player, if it has been started very quickly after the previous RTSP connection had decided to stop. iTunes can sometimes be very quick in starting a new RTSP connection after tearing down the previous one, but shairport_sync unfortunately needs at least 100ms to shut down the old connection. - cleanup_threads() sets playing_conn to NULL if it points to the removed thread, otherwise playing_conn would be a dangling pointer, which is bad, because handle_announce() dereferences it unless it is NULL. --- rtsp.c | 55 +++++++++++++++++++++++++++++-------------------------- 1 file changed, 29 insertions(+), 26 deletions(-) diff --git a/rtsp.c b/rtsp.c index 07f25bef..1213fa49 100644 --- a/rtsp.c +++ b/rtsp.c @@ -268,6 +268,8 @@ static void cleanup_threads(void) { conns[i]->connection_number); pthread_join(conns[i]->thread, &retval); debug(3, "RTSP connection thread %d deleted...", conns[i]->connection_number); + if (conns[i] == playing_conn) + playing_conn = NULL; free(conns[i]); nconns--; if (nconns) @@ -1395,36 +1397,37 @@ static void handle_announce(rtsp_conn_info *conn, rtsp_message *req, rtsp_messag if (pthread_mutex_trylock(&play_lock) == 0) { have_the_player = 1; } else { - if (playing_conn) { - debug(1, "RTSP Conversation thread %d already playing when asked by thread %d.", - playing_conn->connection_number, conn->connection_number); - } else { - debug(1, "play_lock locked but no playing_conn."); - } - if (config.allow_session_interruption == 1) { + int should_wait = 0; + + if (! playing_conn) + die("Non existent playing_conn with play_lock enabled."); + debug(1, "RTSP Conversation thread %d already playing when asked by thread %d.", + playing_conn->connection_number, conn->connection_number); + if (playing_conn->stop) { + debug(1,"Playing connection is already shutting down; waiting for it..."); + should_wait = 1; + } else if (config.allow_session_interruption == 1) { // some other thread has the player ... ask it to relinquish the thread - if (playing_conn) { - debug(1, "ANNOUNCE: playing connection %d being interrupted by connection %d.", - playing_conn->connection_number, conn->connection_number); - if (playing_conn == conn) { - debug(1, "ANNOUNCE asking to stop itself."); - } else { - playing_conn->stop = 1; - memory_barrier(); - pthread_kill(playing_conn->thread, SIGUSR1); - usleep( - 1000000); // here, it is possible for other connections to come in and nab the player. - } + debug(1, "ANNOUNCE: playing connection %d being interrupted by connection %d.", + playing_conn->connection_number, conn->connection_number); + if (playing_conn == conn) { + debug(1, "ANNOUNCE asking to stop itself."); } else { - die("Non existent the_playing_conn with play_lock enabled."); + playing_conn->stop = 1; + memory_barrier(); + pthread_kill(playing_conn->thread, SIGUSR1); + should_wait = 1; } - debug(1, "Try to get the player now"); - // pthread_mutex_lock(&play_lock); - if (pthread_mutex_trylock(&play_lock) == 0) - have_the_player = 1; - else - debug(1, "ANNOUNCE failed to get the player"); } + + if (should_wait) { + usleep(1000000); // here, it is possible for other connections to come in and nab the player. + debug(1, "Try to get the player now"); + } + if (pthread_mutex_trylock(&play_lock) == 0) + have_the_player = 1; + else + debug(1,"ANNOUNCE failed to get the player"); } if (have_the_player) { From 39e122a4ffd5ac6bf0ec338cbd13cf159440e46a Mon Sep 17 00:00:00 2001 From: Belbo Date: Mon, 7 Aug 2017 23:49:19 +0200 Subject: [PATCH 3/5] Fixed dangling pointer to random state array The C library function initstate(), at least in macOS El Capitan, not only sets the state array given to it as the second argument, but also accesses the state array given to it in a previous call! (I don't know why, or if that behaviour is correct, but macOS seems to do that anyhow). Therefore, we cannot pass a local variable to it. As soon as the player thread stops, it will be a dangling pointer, and when a new player thread is started afterwards, initstate() will dereference that dangling pointer with unforeseeable consequences. --- player.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/player.c b/player.c index c6a20d7c..92d7e575 100644 --- a/player.c +++ b/player.c @@ -1481,7 +1481,7 @@ static void *player_thread_func(void *arg) { // I think it's useful to keep this prime to prevent it from falling into a pattern with some // other process. - char rnstate[256]; + static char rnstate[256]; initstate(time(NULL), rnstate, 256); signed short *inbuf, *tbuf, *silence; From afa3238829e0d2532e575e7c080e11a9638dd6d9 Mon Sep 17 00:00:00 2001 From: Belbo Date: Tue, 15 Aug 2017 21:48:13 +0200 Subject: [PATCH 4/5] Fixed EINTR handling in rtsp_read_request() EINTR is an error code written to errno, not returned by read(). --- rtsp.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/rtsp.c b/rtsp.c index 1213fa49..792600a9 100644 --- a/rtsp.c +++ b/rtsp.c @@ -548,9 +548,9 @@ static enum rtsp_read_request_response rtsp_read_request(rtsp_conn_info *conn, reply = rtsp_read_request_response_error; goto shutdown; } - if (nread == EINTR) - continue; if (nread < 0) { + if (errno == EINTR) + continue; perror("read failure"); reply = rtsp_read_request_response_error; goto shutdown; From 3e54765193083926873b55172db0282a2ff3eeee Mon Sep 17 00:00:00 2001 From: Belbo Date: Tue, 15 Aug 2017 21:51:47 +0200 Subject: [PATCH 5/5] Fixed loop condition in non_blocking_write() The while loop should be repeated, if the last write attempt was a partial write only. In this case, however, rc is greater than 0 (should be 1), not equal to 0. --- common.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/common.c b/common.c index 2949ebbb..36fa183c 100644 --- a/common.c +++ b/common.c @@ -723,9 +723,9 @@ uint64_t get_absolute_time_in_fp() { ssize_t non_blocking_write(int fd, const void *buf, size_t count) { void *ibuf = (void *)buf; size_t bytes_remaining = count; - int rc = 0; + int rc = 1; struct pollfd ufds[1]; - while ((bytes_remaining > 0) && (rc == 0)) { + while ((bytes_remaining > 0) && (rc > 0)) { // check that we can do some writing ufds[0].fd = fd; ufds[0].events = POLLOUT;