diff --git a/CMakeLists.txt b/CMakeLists.txt index a8e35c0..d0bacc5 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -119,6 +119,8 @@ set (MacPList " CFBundleVersion ${BUILDVERSION} +NSLocalNetworkUsageDescription +SonoBus needs local network access to find and stream audio with peers on your network. CFBundleURLTypes @@ -170,7 +172,7 @@ function(sono_add_custom_plugin_target target_name product_name formats is_instr # mac settings HARDENED_RUNTIME_ENABLED TRUE - HARDENED_RUNTIME_OPTIONS "com.apple.security.device.audio-input" + HARDENED_RUNTIME_OPTIONS "com.apple.security.device.audio-input" "com.apple.security.network.client" "com.apple.security.network.server" PLIST_TO_MERGE "${MacPList}" AU_MAIN_TYPE "kAudioUnitType_MusicEffect" diff --git a/deps/aoo/lib/src/client.cpp b/deps/aoo/lib/src/client.cpp index f02e5ce..200d7ae 100644 --- a/deps/aoo/lib/src/client.cpp +++ b/deps/aoo/lib/src/client.cpp @@ -539,10 +539,17 @@ void client::do_connect(const std::string &host, int port) return; } - int err = try_connect(host, port); + std::string errmsg; + int err = try_connect(host, port, errmsg); if (err != 0){ // event - std::string errmsg = socket_strerror(err); + // NB: errmsg is always set by try_connect() on failure. We must not + // derive it from errno here: name resolution failures do not set + // errno at all, which used to surface as a bogus message + // (e.g. "Invalid argument") that hid the real cause. + if (errmsg.empty()){ + errmsg = socket_strerror(err); + } auto e = std::make_unique( AOONET_CLIENT_CONNECT_EVENT, 0, errmsg.c_str()); @@ -600,29 +607,61 @@ void client::do_disconnect(command_reason reason, int error){ state_ = client_state::disconnected; } -int client::try_connect(const std::string &host, int port){ +// Guard against a failing call that leaves errno at 0: do_connect() tests +// `err != 0`, so returning 0 from a failure path would be read as success and +// the client would proceed to the handshake holding an unconnected socket. +// Every failure return in try_connect() goes through here. +static inline int nonzero_err(int err){ + return err != 0 ? err : -1; +} + +int client::try_connect(const std::string &host, int port, std::string& errmsg){ + // Resolve the host name *before* creating the socket. + // + // We deliberately use getaddrinfo() rather than gethostbyname(): + // * gethostbyname() is not thread-safe, and this runs on the client + // network thread inside a host application (DAW) that may also be + // resolving names concurrently. + // * gethostbyname() reports failure via h_errno, NOT errno. The old + // code returned socket_errno() here, so a DNS failure was reported + // as whatever stale value errno happened to hold - typically + // "Invalid argument". getaddrinfo() gives us a real reason. + // + // NB: AF_INET is intentional and load-bearing. ip_address::name(), + // ip_address::port() and operator== in net_utils.hpp only understand + // AF_INET; an IPv6 result would be serialised to peers as an empty host + // with port -1 and would never compare equal. Adding IPv6 means changing + // that layer (and the wire format) first, so we must not return one here. + struct addrinfo hints; + memset(&hints, 0, sizeof(hints)); + hints.ai_family = AF_INET; + hints.ai_socktype = SOCK_STREAM; + hints.ai_protocol = IPPROTO_TCP; + + char portstr[16]; + snprintf(portstr, sizeof(portstr), "%d", port); + + struct addrinfo *ailist = nullptr; + int gaierr = getaddrinfo(host.c_str(), portstr, &hints, &ailist); + if (gaierr != 0 || ailist == nullptr){ + if (ailist){ + freeaddrinfo(ailist); + } + errmsg = "couldn't resolve '" + host + "': " + gai_strerror(gaierr); + LOG_ERROR("aoo_client: " << errmsg); + return -1; + } + + remote_addr_ = ip_address(ailist->ai_addr, (socklen_t)ailist->ai_addrlen); + freeaddrinfo(ailist); + tcpsocket_ = socket(AF_INET, SOCK_STREAM, 0); if (tcpsocket_ < 0){ int err = socket_errno(); + errmsg = socket_strerror(err); LOG_ERROR("aoo_client: couldn't create socket (" << err << ")"); - return err; + return nonzero_err(err); } - // resolve host name - struct hostent *he = gethostbyname(host.c_str()); - if (!he){ - int err = socket_errno(); - LOG_ERROR("aoo_client: couldn't connect (" << err << ")"); - return err; - } - - // copy IP address - struct sockaddr_in sa; - memset(&sa, 0, sizeof(sa)); - sa.sin_family = AF_INET; - sa.sin_port = htons(port); - memcpy(&sa.sin_addr, he->h_addr_list[0], he->h_length); - - remote_addr_ = ip_address((struct sockaddr *)&sa, sizeof(sa)); // set TCP_NODELAY int val = 1; @@ -634,8 +673,9 @@ int client::try_connect(const std::string &host, int port){ // try to connect (LATER make timeout configurable) if (socket_connect(tcpsocket_, remote_addr_, 5) < 0){ int err = socket_errno(); + errmsg = socket_strerror(err); LOG_ERROR("aoo_client: couldn't connect (" << err << ")"); - return err; + return nonzero_err(err); } // get local network interface @@ -643,8 +683,9 @@ int client::try_connect(const std::string &host, int port){ if (getsockname(tcpsocket_, (struct sockaddr *)&tmp.address, &tmp.length) < 0) { int err = socket_errno(); + errmsg = socket_strerror(err); LOG_ERROR("aoo_client: couldn't get socket name (" << err << ")"); - return err; + return nonzero_err(err); } local_addr_ = ip_address(tmp.name(), udpport_); @@ -657,8 +698,9 @@ int client::try_connect(const std::string &host, int port){ val = 1; if (ioctl(tcpsocket_, FIONBIO, (char *)&val) < 0){ int err = socket_errno(); + errmsg = socket_strerror(err); LOG_ERROR("aoo_client: couldn't set socket to non-blocking (" << err << ")"); - return err; + return nonzero_err(err); } #endif @@ -669,6 +711,7 @@ int client::try_connect(const std::string &host, int port){ return 0; } + void client::do_login(){ char buf[AOO_MAXPACKETSIZE]; osc::OutboundPacketStream msg(buf, sizeof(buf)); diff --git a/deps/aoo/lib/src/client.hpp b/deps/aoo/lib/src/client.hpp index a9d90f7..223f844 100644 --- a/deps/aoo/lib/src/client.hpp +++ b/deps/aoo/lib/src/client.hpp @@ -139,7 +139,7 @@ public: void do_connect(const std::string& host, int port); - int try_connect(const std::string& host, int port); + int try_connect(const std::string& host, int port, std::string& errmsg); void do_disconnect(command_reason reason = command_reason::none, int error = 0); diff --git a/deps/aoo/lib/src/net_utils.cpp b/deps/aoo/lib/src/net_utils.cpp index 5012b5e..69c1a6e 100644 --- a/deps/aoo/lib/src/net_utils.cpp +++ b/deps/aoo/lib/src/net_utils.cpp @@ -61,9 +61,6 @@ int socket_connect(int socket, const ip_address& addr, float timeout) if (connect(socket, (const struct sockaddr *)&addr.address, addr.length) < 0) { - int status; - struct timeval timeoutval; - fd_set writefds, errfds; #ifdef _WIN32 if (socket_errno() != WSAEWOULDBLOCK) #else @@ -71,19 +68,44 @@ int socket_connect(int socket, const ip_address& addr, float timeout) #endif return -1; // break on "real" error - // block with select using timeout + // Wait for the socket to become writable. + // + // NB: poll(), NOT select(). select() cannot handle a file descriptor + // at or above FD_SETSIZE (1024 on macOS): FD_SET() writes out of + // bounds and select() then fails with EINVAL. Inside a host + // application that keeps many files open — a DAW with a large + // session, for instance — our socket easily lands above that limit, + // so connecting failed with a bare "Invalid argument" that had + // nothing to do with the arguments. It also explains why the same + // build connects fine as a standalone app (few open descriptors) and + // intermittently as a plugin (depends how much the host has open). + // + // poll() has no such limit. The rest of this library already moved to + // poll() (see client.cpp and server.cpp); socket_connect() was the + // last place still using select(). if (timeout < 0) timeout = 0; - timeoutval.tv_sec = (int)timeout; - timeoutval.tv_usec = (timeout - timeoutval.tv_sec) * 1000000; - FD_ZERO(&writefds); - FD_SET(socket, &writefds); // socket is connected when writable - FD_ZERO(&errfds); - FD_SET(socket, &errfds); // catch exceptions + int timeout_ms = (int)(timeout * 1000.0f + 0.5f); + + #ifdef _WIN32 + WSAPOLLFD pfd; + #else + struct pollfd pfd; + #endif + pfd.fd = socket; + pfd.events = POLLOUT; + pfd.revents = 0; + + #ifdef _WIN32 + int status = WSAPoll(&pfd, 1, timeout_ms); + #else + int status; + do { + status = poll(&pfd, 1, timeout_ms); + } while (status < 0 && errno == EINTR); // a signal is not a failure + #endif - status = select(socket+1, NULL, &writefds, &errfds, &timeoutval); - if (status < 0) // select failed + if (status < 0) // poll failed { - fprintf(stderr, "socket_connect: select failed"); return -1; } else if (status == 0) // connection timed out @@ -96,10 +118,14 @@ int socket_connect(int socket, const ip_address& addr, float timeout) return -1; } - if (FD_ISSET(socket, &errfds)) // connection failed + // Writable can still mean "failed": ask the socket for the real error. + // POLLERR/POLLHUP are only ever set in revents, never requested. + if (pfd.revents & (POLLERR | POLLHUP)) { - int err; socklen_t len = sizeof(err); - getsockopt(socket, SOL_SOCKET, SO_ERROR, (char *)&err, &len); + int err = 0; socklen_t len = sizeof(err); + if (getsockopt(socket, SOL_SOCKET, SO_ERROR, (char *)&err, &len) < 0 || err == 0){ + err = ECONNREFUSED; // poll said failure; make sure we report one + } #ifdef _WIN32 WSASetLastError(err); #else diff --git a/deps/aoo/lib/src/net_utils.hpp b/deps/aoo/lib/src/net_utils.hpp index d878d2b..df30fd1 100644 --- a/deps/aoo/lib/src/net_utils.hpp +++ b/deps/aoo/lib/src/net_utils.hpp @@ -4,6 +4,7 @@ #ifdef _WIN32 #include +#include // getaddrinfo() / gai_strerror() typedef int socklen_t; #else #include @@ -45,13 +46,29 @@ struct ip_address { length = sizeof(sa); } ip_address(const std::string& host, int port){ + // NB: inet_pton() rather than inet_addr(). inet_addr() signals failure + // by returning INADDR_NONE, which is indistinguishable from the valid + // broadcast address 255.255.255.255 - so a malformed or hostile host + // string used to be silently turned into a broadcast address. Both + // call sites that reach here (client::handle_server_message_udp and + // server::handle_login) parse addresses out of untrusted network + // messages, so that mattered. + // + // For any well-formed dotted-quad address the result is byte-for-byte + // identical to before; only the failure case differs, and it now + // yields a clearly-invalid address (name() == "", port() == -1) + // instead of the broadcast address. + memset(&address, 0, sizeof(address)); struct sockaddr_in sa; memset(&sa, 0, sizeof(sa)); - sa.sin_family = AF_INET; - sa.sin_addr.s_addr = inet_addr(host.c_str()); - sa.sin_port = htons(port); - memcpy(&address, &sa, sizeof(sa)); - length = sizeof(sa); + if (inet_pton(AF_INET, host.c_str(), &sa.sin_addr) == 1){ + sa.sin_family = AF_INET; + sa.sin_port = htons(port); + memcpy(&address, &sa, sizeof(sa)); + length = sizeof(sa); + } else { + length = sizeof(address); + } } ip_address(const ip_address& other){