From: Mike Brady Date: Sun, 21 Jul 2019 07:06:37 +0000 (+0200) Subject: Fix a bug that sometimes caused a crash when a service name was specified in the... X-Git-Tag: 3.3.2~4^2~3 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=175ae0a65031c78275d3bd2d024f9140ac1d0885;p=thirdparty%2Fshairport-sync.git Fix a bug that sometimes caused a crash when a service name was specified in the configuration file. The fix was to be more systematic in allocating and deallocating memory for termporary strings. Thanks to Chris Boot, Ari Sovijarvi and Jeroen Massar for the bug report. Fixes Debian Bug report #925577. Allow the rtsp_listen_loop to exit if the RTSP port (usually port 5000) is not available. This allows the system to exit cleanly rather than abort. --- diff --git a/rtsp.c b/rtsp.c index 20fc6b8b..4a72250a 100644 --- a/rtsp.c +++ b/rtsp.c @@ -2572,7 +2572,7 @@ void rtsp_listen_loop(void) { } else #endif family = "IPv4"; - debug(1, "Unable to listen on %s port %d. The error is: \"%s\".", family, config.port, + debug(1, "unable to listen on %s port %d. The error is: \"%s\".", family, config.port, strerror(errno)); continue; } @@ -2585,127 +2585,127 @@ void rtsp_listen_loop(void) { freeaddrinfo(info); - if (!nsock) - die("Could not establish a service on port %d -- program terminating. Is another instance of " - "Shairport Sync running on this device?", - config.port); - - int maxfd = -1; - fd_set fds; - FD_ZERO(&fds); - for (i = 0; i < nsock; i++) { - if (sockfd[i] > maxfd) - maxfd = sockfd[i]; - } - - mdns_register(); - - pthread_setcancelstate(oldState, NULL); - int acceptfd; - struct timeval tv; - pthread_cleanup_push(rtsp_listen_loop_cleanup_handler, (void *)sockfd); - do { - pthread_testcancel(); - tv.tv_sec = 60; - tv.tv_usec = 0; + if (nsock) { + int maxfd = -1; + fd_set fds; + FD_ZERO(&fds); + for (i = 0; i < nsock; i++) { + if (sockfd[i] > maxfd) + maxfd = sockfd[i]; + } - for (i = 0; i < nsock; i++) - FD_SET(sockfd[i], &fds); + mdns_register(); - ret = select(maxfd + 1, &fds, 0, 0, &tv); - if (ret < 0) { - if (errno == EINTR) - continue; - break; - } + pthread_setcancelstate(oldState, NULL); + int acceptfd; + struct timeval tv; + pthread_cleanup_push(rtsp_listen_loop_cleanup_handler, (void *)sockfd); + do { + pthread_testcancel(); + tv.tv_sec = 60; + tv.tv_usec = 0; - cleanup_threads(); + for (i = 0; i < nsock; i++) + FD_SET(sockfd[i], &fds); - acceptfd = -1; - for (i = 0; i < nsock; i++) { - if (FD_ISSET(sockfd[i], &fds)) { - acceptfd = sockfd[i]; + ret = select(maxfd + 1, &fds, 0, 0, &tv); + if (ret < 0) { + if (errno == EINTR) + continue; break; } - } - if (acceptfd < 0) // timeout - continue; - rtsp_conn_info *conn = malloc(sizeof(rtsp_conn_info)); - if (conn == 0) - die("Couldn't allocate memory for an rtsp_conn_info record."); - memset(conn, 0, sizeof(rtsp_conn_info)); - conn->connection_number = RTSP_connection_index++; - socklen_t slen = sizeof(conn->remote); - - conn->fd = accept(acceptfd, (struct sockaddr *)&conn->remote, &slen); - if (conn->fd < 0) { - debug(1, "Connection %d: New connection on port %d not accepted:", conn->connection_number, - config.port); - perror("failed to accept connection"); - free(conn); - } else { - SOCKADDR *local_info = (SOCKADDR *)&conn->local; - socklen_t size_of_reply = sizeof(*local_info); - memset(local_info, 0, sizeof(SOCKADDR)); - if (getsockname(conn->fd, (struct sockaddr *)local_info, &size_of_reply) == 0) { - - // IPv4: - if (local_info->SAFAMILY == AF_INET) { - char ip4[INET_ADDRSTRLEN]; // space to hold the IPv4 string - char remote_ip4[INET_ADDRSTRLEN]; // space to hold the IPv4 string - struct sockaddr_in *sa = (struct sockaddr_in *)local_info; - inet_ntop(AF_INET, &(sa->sin_addr), ip4, INET_ADDRSTRLEN); - unsigned short int tport = ntohs(sa->sin_port); - sa = (struct sockaddr_in *)&conn->remote; - inet_ntop(AF_INET, &(sa->sin_addr), remote_ip4, INET_ADDRSTRLEN); - unsigned short int rport = ntohs(sa->sin_port); - debug(2, "Connection %d: new connection from %s:%u to self at %s:%u.", - conn->connection_number, remote_ip4, rport, ip4, tport); - } -#ifdef AF_INET6 - if (local_info->SAFAMILY == AF_INET6) { - // IPv6: - - char ip6[INET6_ADDRSTRLEN]; // space to hold the IPv6 string - char remote_ip6[INET6_ADDRSTRLEN]; // space to hold the IPv6 string - struct sockaddr_in6 *sa6 = - (struct sockaddr_in6 *)local_info; // pretend this is loaded with something - inet_ntop(AF_INET6, &(sa6->sin6_addr), ip6, INET6_ADDRSTRLEN); - u_int16_t tport = ntohs(sa6->sin6_port); - - sa6 = (struct sockaddr_in6 *)&conn->remote; // pretend this is loaded with something - inet_ntop(AF_INET6, &(sa6->sin6_addr), remote_ip6, INET6_ADDRSTRLEN); - u_int16_t rport = ntohs(sa6->sin6_port); - debug(2, "Connection %d: new connection from [%s]:%u to self at [%s]:%u.", - conn->connection_number, remote_ip6, rport, ip6, tport); + cleanup_threads(); + + acceptfd = -1; + for (i = 0; i < nsock; i++) { + if (FD_ISSET(sockfd[i], &fds)) { + acceptfd = sockfd[i]; + break; } -#endif + } + if (acceptfd < 0) // timeout + continue; + rtsp_conn_info *conn = malloc(sizeof(rtsp_conn_info)); + if (conn == 0) + die("Couldn't allocate memory for an rtsp_conn_info record."); + memset(conn, 0, sizeof(rtsp_conn_info)); + conn->connection_number = RTSP_connection_index++; + socklen_t slen = sizeof(conn->remote); + + conn->fd = accept(acceptfd, (struct sockaddr *)&conn->remote, &slen); + if (conn->fd < 0) { + debug(1, "Connection %d: New connection on port %d not accepted:", conn->connection_number, + config.port); + perror("failed to accept connection"); + free(conn); } else { - debug(1, "Error figuring out Shairport Sync's own IP number."); - } - // usleep(500000); - // pthread_t rtsp_conversation_thread; - // conn->thread = rtsp_conversation_thread; - // conn->stop = 0; // record's memory has been zeroed - // conn->authorized = 0; // record's memory has been zeroed - // fcntl(conn->fd, F_SETFL, O_NONBLOCK); - - ret = pthread_create(&conn->thread, NULL, rtsp_conversation_thread_func, - conn); // also acts as a memory barrier - if (ret) { - char errorstring[1024]; - strerror_r(ret, (char *)errorstring, sizeof(errorstring)); - die("Connection %d: cannot create an RTSP conversation thread. Error %d: \"%s\".", - conn->connection_number, ret, (char *)errorstring); - } - debug(3, "Successfully created RTSP receiver thread %d.", conn->connection_number); - conn->running = 1; // this must happen before the thread is tracked - track_thread(conn); - } - } while (1); + SOCKADDR *local_info = (SOCKADDR *)&conn->local; + socklen_t size_of_reply = sizeof(*local_info); + memset(local_info, 0, sizeof(SOCKADDR)); + if (getsockname(conn->fd, (struct sockaddr *)local_info, &size_of_reply) == 0) { + + // IPv4: + if (local_info->SAFAMILY == AF_INET) { + char ip4[INET_ADDRSTRLEN]; // space to hold the IPv4 string + char remote_ip4[INET_ADDRSTRLEN]; // space to hold the IPv4 string + struct sockaddr_in *sa = (struct sockaddr_in *)local_info; + inet_ntop(AF_INET, &(sa->sin_addr), ip4, INET_ADDRSTRLEN); + unsigned short int tport = ntohs(sa->sin_port); + sa = (struct sockaddr_in *)&conn->remote; + inet_ntop(AF_INET, &(sa->sin_addr), remote_ip4, INET_ADDRSTRLEN); + unsigned short int rport = ntohs(sa->sin_port); + debug(2, "Connection %d: new connection from %s:%u to self at %s:%u.", + conn->connection_number, remote_ip4, rport, ip4, tport); + } + #ifdef AF_INET6 + if (local_info->SAFAMILY == AF_INET6) { + // IPv6: + + char ip6[INET6_ADDRSTRLEN]; // space to hold the IPv6 string + char remote_ip6[INET6_ADDRSTRLEN]; // space to hold the IPv6 string + struct sockaddr_in6 *sa6 = + (struct sockaddr_in6 *)local_info; // pretend this is loaded with something + inet_ntop(AF_INET6, &(sa6->sin6_addr), ip6, INET6_ADDRSTRLEN); + u_int16_t tport = ntohs(sa6->sin6_port); + + sa6 = (struct sockaddr_in6 *)&conn->remote; // pretend this is loaded with something + inet_ntop(AF_INET6, &(sa6->sin6_addr), remote_ip6, INET6_ADDRSTRLEN); + u_int16_t rport = ntohs(sa6->sin6_port); + debug(2, "Connection %d: new connection from [%s]:%u to self at [%s]:%u.", + conn->connection_number, remote_ip6, rport, ip6, tport); + } + #endif - pthread_cleanup_pop(1); // should never happen - debug(1, "Oops -- fell out of the RTSP select loop"); + } else { + debug(1, "Error figuring out Shairport Sync's own IP number."); + } + // usleep(500000); + // pthread_t rtsp_conversation_thread; + // conn->thread = rtsp_conversation_thread; + // conn->stop = 0; // record's memory has been zeroed + // conn->authorized = 0; // record's memory has been zeroed + // fcntl(conn->fd, F_SETFL, O_NONBLOCK); + + ret = pthread_create(&conn->thread, NULL, rtsp_conversation_thread_func, + conn); // also acts as a memory barrier + if (ret) { + char errorstring[1024]; + strerror_r(ret, (char *)errorstring, sizeof(errorstring)); + die("Connection %d: cannot create an RTSP conversation thread. Error %d: \"%s\".", + conn->connection_number, ret, (char *)errorstring); + } + debug(3, "Successfully created RTSP receiver thread %d.", conn->connection_number); + conn->running = 1; // this must happen before the thread is tracked + track_thread(conn); + } + } while (1); + pthread_cleanup_pop(1); // should never happen + } else { + warn("could not establish a service on port %d -- program terminating. Is another instance of " + "Shairport Sync running on this device?", + config.port); + } + // debug(1, "Oops -- fell out of the RTSP select loop"); } diff --git a/shairport.c b/shairport.c index 499b4c0b..b301aecf 100644 --- a/shairport.c +++ b/shairport.c @@ -200,11 +200,17 @@ void *soxr_time_check(__attribute__((unused)) void *arg) { size_t odone; + //int oldState; + //pthread_setcancelstate(PTHREAD_CANCEL_DISABLE, &oldState); // make this un-cancellable + soxr_oneshot(buffer_length, buffer_length + 1, 2, // Rates and # of chans. inbuffer, buffer_length, NULL, // Input. outbuffer, buffer_length + 1, &odone, // Output. &io_spec, // Input, output and transfer spec. NULL, NULL); // Default configuration. + //pthread_setcancelstate(oldState, NULL); + + pthread_testcancel(); io_spec.itype = SOXR_INT32_I; io_spec.otype = SOXR_INT32_I; @@ -212,11 +218,13 @@ void *soxr_time_check(__attribute__((unused)) void *arg) { io_spec.e = NULL; io_spec.flags = 0; + //pthread_setcancelstate(PTHREAD_CANCEL_DISABLE, &oldState); // make this un-cancellable soxr_oneshot(buffer_length, buffer_length - 1, 2, // Rates and # of chans. inbuffer, buffer_length, NULL, // Input. outbuffer, buffer_length - 1, &odone, // Output. &io_spec, // Input, output and transfer spec. NULL, NULL); // Default configuration. + //pthread_setcancelstate(oldState, NULL); } double soxr_execution_time_us = @@ -1162,25 +1170,27 @@ int parse_options(int argc, char **argv) { if (tdebuglev != 0) debuglev = tdebuglev; - /* if the Service Name wasn't specified, do it now */ - - if (raw_service_name == NULL) - raw_service_name = strdup("%H"); - // now, do the substitutions in the service name char hostname[100]; gethostname(hostname, 100); - char *i1 = str_replace(raw_service_name, "%h", hostname); - if (raw_service_name) { - free(raw_service_name); - raw_service_name = NULL; - } + + + + char *i0; + if (raw_service_name == NULL) + i0 = strdup("%H"); // this is the default it the Service Name wasn't specified + else + i0 = strdup(raw_service_name); + + // here, do the substitutions for %h, %H, %v and %V + char *i1 = str_replace(i0, "%h", hostname); if ((hostname[0] >= 'a') && (hostname[0] <= 'z')) hostname[0] = hostname[0] - 0x20; // convert a lowercase first letter into a capital letter char *i2 = str_replace(i1, "%H", hostname); char *i3 = str_replace(i2, "%v", PACKAGE_VERSION); char *vs = get_version_string(); - config.service_name = str_replace(i3, "%V", vs); + config.service_name = str_replace(i3, "%V", vs); // service name complete + free(i0); free(i1); free(i2); free(i3); @@ -1277,8 +1287,8 @@ const char *pid_file_proc(void) { #endif void main_cleanup_handler(__attribute__((unused)) void *arg) { - // it doesn't look like this is called when the main function is cancelled eith a pthread cancel. - debug(2, "main cleanup handler called."); + // it doesn't look like this is called when the main function is cancelled with a pthread cancel. + debug(1, "main cleanup handler called."); #ifdef CONFIG_MQTT if (config.mqtt_enabled) { // terminate_mqtt(); @@ -1320,6 +1330,11 @@ void main_cleanup_handler(__attribute__((unused)) void *arg) { config.output->deinit(); } +#ifdef CONFIG_SOXR + // be careful -- not sure if the thread can be cancelled cleanly, so wait for it to shut down + pthread_join(soxr_time_check_thread, NULL); +#endif + #ifdef CONFIG_LIBDAEMON // only do this if you are the daemon if (pid == 0) { @@ -1334,7 +1349,7 @@ void main_cleanup_handler(__attribute__((unused)) void *arg) { } void exit_function() { - debug(2, "exit function called..."); + debug(1, "exit function called..."); main_cleanup_handler(NULL); if (conns) free(conns); // make sure the connections have been deleted first @@ -1643,7 +1658,7 @@ int main(int argc, char **argv) { } config.output->init(argc - audio_arg, argv + audio_arg); - pthread_cleanup_push(main_cleanup_handler, NULL); + // pthread_cleanup_push(main_cleanup_handler, NULL); // daemon_log(LOG_NOTICE, "startup"); @@ -1854,15 +1869,6 @@ int main(int argc, char **argv) { #endif activity_monitor_start(); - - // debug(1, "Successful Startup"); rtsp_listen_loop(); - - // should not reach this... - // daemon_log(LOG_NOTICE, "Unexpected exit..."); - // daemon_retval_send(0); - // daemon_pid_file_remove(); - pthread_cleanup_pop(1); - debug(1, "Odd exit point"); - pthread_exit(NULL); + return 0; }