From ac23bec14450a8f38ead09860564e42737ac047e Mon Sep 17 00:00:00 2001 From: Romain Vimont Date: Tue, 26 Oct 2021 22:49:45 +0200 Subject: [PATCH] Expose socket interruption On Linux, socket functions are unblocked by shutdown(), but on Windows they are unblocked by closesocket(). Expose net_interrupt() and net_close() to abstract these differences: - net_interrupt() calls shutdown() on Linux and closesocket() on Windows (if not already called); - net_close() calls close() on Linux and closesocket() on Windows (if not already called). This simplifies the server code, and prevents a data race on close (reported by TSAN) on Linux (but does not fix it on Windows): WARNING: ThreadSanitizer: data race (pid=836124) Write of size 8 at 0x7ba0000000d0 by main thread: #0 close ../../../../src/libsanitizer/tsan/tsan_interceptors_posix.cpp:1690 (libtsan.so.0+0x359d8) #1 net_close ../app/src/util/net.c:211 (scrcpy+0x1c76b) #2 close_socket ../app/src/server.c:330 (scrcpy+0x19442) #3 server_stop ../app/src/server.c:522 (scrcpy+0x19e33) #4 scrcpy ../app/src/scrcpy.c:532 (scrcpy+0x156fc) #5 main ../app/src/main.c:92 (scrcpy+0x622a) Previous read of size 8 at 0x7ba0000000d0 by thread T6: #0 recv ../../../../src/libsanitizer/sanitizer_common/sanitizer_common_interceptors.inc:6603 (libtsan.so.0+0x4f4a6) #1 net_recv ../app/src/util/net.c:167 (scrcpy+0x1c5a7) #2 run_receiver ../app/src/receiver.c:76 (scrcpy+0x12819) #3 (libSDL2-2.0.so.0+0x84f40) --- app/src/server.c | 68 ++++++++++++++++++++++++---------------------- app/src/server.h | 1 - app/src/util/net.c | 23 ++++++++++++++-- app/src/util/net.h | 12 ++++---- 4 files changed, 62 insertions(+), 42 deletions(-) diff --git a/app/src/server.c b/app/src/server.c index f786f62d..76d2910b 100644 --- a/app/src/server.c +++ b/app/src/server.c @@ -323,21 +323,10 @@ connect_to_server(uint16_t port, uint32_t attempts, uint32_t delay) { return SC_INVALID_SOCKET; } -static void -close_socket(sc_socket socket) { - assert(socket != SC_INVALID_SOCKET); - net_shutdown(socket, SHUT_RDWR); - if (!net_close(socket)) { - LOGW("Could not close socket"); - } -} - bool server_init(struct server *server) { server->serial = NULL; server->process = PROCESS_NONE; - atomic_flag_clear_explicit(&server->server_socket_closed, - memory_order_relaxed); bool ok = sc_mutex_init(&server->mutex); if (!ok) { @@ -376,12 +365,11 @@ run_wait_server(void *data) { // no need for synchronization, server_socket is initialized before this // thread was created - if (server->server_socket != SC_INVALID_SOCKET - && !atomic_flag_test_and_set(&server->server_socket_closed)) { - // On Linux, accept() is unblocked by shutdown(), but on Windows, it is - // unblocked by closesocket(). Therefore, call both (close_socket()). - close_socket(server->server_socket); + if (server->server_socket != SC_INVALID_SOCKET) { + // Unblock any accept() + net_interrupt(server->server_socket); } + LOGD("Server terminated"); return 0; } @@ -430,14 +418,8 @@ server_start(struct server *server, const struct server_params *params) { return true; error: - if (!server->tunnel_forward) { - bool was_closed = - atomic_flag_test_and_set(&server->server_socket_closed); - // the thread is not started, the flag could not be already set - assert(!was_closed); - (void) was_closed; - close_socket(server->server_socket); - } + // The server socket (if any) will be closed on server_destroy() + disable_tunnel(server); return false; @@ -479,11 +461,11 @@ server_connect_to(struct server *server, char *device_name, struct size *size) { } // we don't need the server socket anymore - if (!atomic_flag_test_and_set(&server->server_socket_closed)) { - // close it from here - close_socket(server->server_socket); - // otherwise, it is closed by run_wait_server() + if (!net_close(server->server_socket)) { + LOGW("Could not close server socket on connect"); } + // Do not attempt to close it again on server_destroy() + server->server_socket = SC_INVALID_SOCKET; } else { uint32_t attempts = 100; uint32_t delay = 100; // ms @@ -511,15 +493,20 @@ server_connect_to(struct server *server, char *device_name, struct size *size) { void server_stop(struct server *server) { - if (server->server_socket != SC_INVALID_SOCKET - && !atomic_flag_test_and_set(&server->server_socket_closed)) { - close_socket(server->server_socket); + if (server->server_socket != SC_INVALID_SOCKET) { + if (!net_interrupt(server->server_socket)) { + LOGW("Could not interrupt server socket"); + } } if (server->video_socket != SC_INVALID_SOCKET) { - close_socket(server->video_socket); + if (!net_interrupt(server->video_socket)) { + LOGW("Could not interrupt video socket"); + } } if (server->control_socket != SC_INVALID_SOCKET) { - close_socket(server->control_socket); + if (!net_interrupt(server->control_socket)) { + LOGW("Could not interrupt control socket"); + } } assert(server->process != PROCESS_NONE); @@ -556,6 +543,21 @@ server_stop(struct server *server) { void server_destroy(struct server *server) { + if (server->server_socket != SC_INVALID_SOCKET) { + if (!net_close(server->server_socket)) { + LOGW("Could not close server socket"); + } + } + if (server->video_socket != SC_INVALID_SOCKET) { + if (!net_close(server->video_socket)) { + LOGW("Could not close video socket"); + } + } + if (server->control_socket != SC_INVALID_SOCKET) { + if (!net_close(server->control_socket)) { + LOGW("Could not close control socket"); + } + } free(server->serial); sc_cond_destroy(&server->process_terminated_cond); sc_mutex_destroy(&server->mutex); diff --git a/app/src/server.h b/app/src/server.h index b6d2255a..141075f7 100644 --- a/app/src/server.h +++ b/app/src/server.h @@ -18,7 +18,6 @@ struct server { char *serial; process_t process; sc_thread wait_server_thread; - atomic_flag server_socket_closed; sc_mutex mutex; sc_cond process_terminated_cond; diff --git a/app/src/util/net.c b/app/src/util/net.c index 1e74214b..cfc60433 100644 --- a/app/src/util/net.c +++ b/app/src/util/net.c @@ -1,5 +1,6 @@ #include "net.h" +#include #include #include @@ -55,6 +56,7 @@ wrap(sc_raw_socket sock) { } socket->socket = sock; + socket->closed = (atomic_flag) ATOMIC_FLAG_INIT; return socket; #else @@ -195,18 +197,33 @@ net_send_all(sc_socket socket, const void *buf, size_t len) { } bool -net_shutdown(sc_socket socket, int how) { +net_interrupt(sc_socket socket) { + assert(socket != SC_INVALID_SOCKET); + sc_raw_socket raw_sock = unwrap(socket); - return !shutdown(raw_sock, how); + +#ifdef __WINDOWS__ + if (!atomic_flag_test_and_set(&socket->closed)) { + return !closesocket(raw_sock); + } + return true; +#else + return !shutdown(raw_sock, SHUT_RDWR); +#endif } +#include bool net_close(sc_socket socket) { sc_raw_socket raw_sock = unwrap(socket); #ifdef __WINDOWS__ + bool ret = true; + if (!atomic_flag_test_and_set(&socket->closed)) { + ret = !closesocket(raw_sock); + } free(socket); - return !closesocket(raw_sock); + return ret; #else return !close(raw_sock); #endif diff --git a/app/src/util/net.h b/app/src/util/net.h index f40f0bb5..c742c00e 100644 --- a/app/src/util/net.h +++ b/app/src/util/net.h @@ -10,12 +10,11 @@ #ifdef __WINDOWS__ # include -# define SHUT_RD SD_RECEIVE -# define SHUT_WR SD_SEND -# define SHUT_RDWR SD_BOTH +# include # define SC_INVALID_SOCKET NULL typedef struct sc_socket_windows { SOCKET socket; + atomic_flag closed; } *sc_socket; #else // not __WINDOWS__ @@ -53,10 +52,13 @@ net_send(sc_socket socket, const void *buf, size_t len); ssize_t net_send_all(sc_socket socket, const void *buf, size_t len); -// how is SHUT_RD (read), SHUT_WR (write) or SHUT_RDWR (both) +// Shutdown the socket (or close on Windows) so that any blocking send() or +// recv() are interrupted. bool -net_shutdown(sc_socket socket, int how); +net_interrupt(sc_socket socket); +// Close the socket. +// A socket must always be closed, even if net_interrupt() has been called. bool net_close(sc_socket socket);