From fd3e2fe18a101e2bbfd5935263183e25ab26497b Mon Sep 17 00:00:00 2001 From: cirakg Date: Tue, 3 Nov 2026 00:17:50 +0100 Subject: network: fix room socket races and share the room network check bsd:s, bsd:u and bsd:a share one file descriptor table, but each service bound its own proxy packet callback, so every room packet was queued three times on each socket. Bind the callback once. The room thread walked the descriptor table without a lock while bsd created and closed sockets, so it could reach a destroyed or not yet created socket. Guard the table with a mutex, and publish a socket only after it is created and initialized. Check airplane mode before an fd is taken, so a refused socket no longer leaks its slot. ProxySocket now initializes protocol, decompresses packets outside the packet lock without copying the compressed data first, and keeps a read offset for partial TCP reads instead of erasing from the front. Add Network::GetRoomIPv4Address(), set only in the Joined and Moderator states. NIFM uses it for network availability and for every reported address, including EnumerateNetworkInterfaces and GetCurrentIpAddress, so no call reports the room address while another rejects the network. Assisted-by: Claude Opus 5.5 (investigation and implementation) --- src/core/internal_network/network.cpp | 11 +++++++++++ 1 file changed, 11 insertions(+) (limited to 'src/core/internal_network/network.cpp') diff --git a/src/core/internal_network/network.cpp b/src/core/internal_network/network.cpp index 01d37fb35b..d1c436aa08 100644 --- a/src/core/internal_network/network.cpp +++ b/src/core/internal_network/network.cpp @@ -722,6 +722,17 @@ std::optional GetHostIPv4Address() { return TranslateIPv4(network_interface->ip_address); } +std::optional GetRoomIPv4Address() { + if (const auto room_member = GetRoomMember().lock()) { + // The room host and moderators are in the Moderator state instead of Joined + const auto state = room_member->GetState(); + if (state == RoomMember::State::Joined || state == RoomMember::State::Moderator) { + return room_member->GetFakeIpAddress(); + } + } + return std::nullopt; +} + std::string IPv4AddressToString(IPv4Address ip_addr) { std::array buf = {}; ASSERT(inet_ntop(AF_INET, &ip_addr, buf.data(), sizeof(buf)) == buf.data()); -- cgit v1.2.3 From 0b927d171c6d918a675c25f57b2bc0121b7f889d Mon Sep 17 00:00:00 2001 From: cirakg Date: Tue, 3 Nov 2026 01:39:54 +0100 Subject: network: close each bsd fd once and drop unwanted or malformed room packets Two threads closing the same fd both copied the descriptor and both called Close(), so a host socket could be closed twice, and the second closesocket could hit an unrelated socket that had reused the host fd. CloseImpl now frees the slot under file_descriptors_mutex first and only that thread closes the socket; the other gets EBADF. OnProxyPacketReceived decompressed every room packet when any fd had a socket, including native sockets and proxy sockets on other ports. Sockets now report through AcceptsProxyPacket whether they want a packet, and only those are collected, so an unwanted packet is not decompressed. DecompressDataZSTD passed the frame's declared size straight to the vector constructor, so a malformed or unsized frame from a room member threw on the room thread. The new TryDecompressDataZSTD rejects invalid, unsized and oversized frames, and bsd drops such packets with a warning. DecompressDataZSTD now returns an empty vector for them instead of throwing. Assisted-by: Claude Opus 5.5 (investigation and implementation) --- src/common/zstd_compression.cpp | 20 +++++++++---- src/common/zstd_compression.h | 13 +++++++++ src/core/hle/service/sockets/bsd.cpp | 46 ++++++++++++++++++------------ src/core/hle/service/sockets/bsd.h | 2 ++ src/core/internal_network/network.cpp | 4 +++ src/core/internal_network/socket_proxy.cpp | 28 ++++++++++++------ src/core/internal_network/socket_proxy.h | 5 ++++ src/core/internal_network/sockets.h | 5 ++++ 8 files changed, 91 insertions(+), 32 deletions(-) (limited to 'src/core/internal_network/network.cpp') diff --git a/src/common/zstd_compression.cpp b/src/common/zstd_compression.cpp index 7665482ee6..9bb8d7b919 100644 --- a/src/common/zstd_compression.cpp +++ b/src/common/zstd_compression.cpp @@ -35,16 +35,26 @@ std::vector CompressDataZSTDDefault(const u8* source, std::size_t source_siz } std::vector DecompressDataZSTD(std::span compressed) { - const std::size_t decompressed_size = + return TryDecompressDataZSTD(compressed, std::vector{}.max_size()) + .value_or(std::vector{}); +} + +std::optional> TryDecompressDataZSTD(std::span compressed, + std::size_t max_size) { + // The size comes from the frame header, so check it before allocating + const unsigned long long decompressed_size = ZSTD_getFrameContentSize(compressed.data(), compressed.size()); - std::vector decompressed(decompressed_size); + if (decompressed_size == ZSTD_CONTENTSIZE_ERROR || + decompressed_size == ZSTD_CONTENTSIZE_UNKNOWN || decompressed_size > max_size) { + return std::nullopt; + } + std::vector decompressed(static_cast(decompressed_size)); const std::size_t uncompressed_result_size = ZSTD_decompress( decompressed.data(), decompressed.size(), compressed.data(), compressed.size()); - if (decompressed_size != uncompressed_result_size || ZSTD_isError(uncompressed_result_size)) { - // Decompression failed - return {}; + if (ZSTD_isError(uncompressed_result_size) || uncompressed_result_size != decompressed_size) { + return std::nullopt; } return decompressed; } diff --git a/src/common/zstd_compression.h b/src/common/zstd_compression.h index 819d7d201a..c20d96adb0 100644 --- a/src/common/zstd_compression.h +++ b/src/common/zstd_compression.h @@ -6,6 +6,7 @@ #pragma once +#include #include #include @@ -45,6 +46,18 @@ namespace Common::Compression { */ [[nodiscard]] std::vector DecompressDataZSTD(std::span compressed); +/** + * Decompresses a single Zstandard frame that declares its content size. + * + * @param compressed the compressed source memory region. + * @param max_size the largest content size to accept. + * + * @return the decompressed data, or nullopt if the frame is invalid, does not declare its size, + * or declares more than max_size bytes. + */ +[[nodiscard]] std::optional> TryDecompressDataZSTD(std::span compressed, + std::size_t max_size); + /** * Decompresses a single Zstandard frame into a caller-provided buffer of known size. * diff --git a/src/core/hle/service/sockets/bsd.cpp b/src/core/hle/service/sockets/bsd.cpp index e6d23b7bf6..730a7edcc1 100644 --- a/src/core/hle/service/sockets/bsd.cpp +++ b/src/core/hle/service/sockets/bsd.cpp @@ -965,24 +965,24 @@ std::pair BSD_USA::SendToImpl(s32 fd, u32 flags, std::span } Errno BSD_USA::CloseImpl(s32 fd) { - const auto descriptor = CopyDescriptor(fd); - if (!descriptor) { - return Errno::BADF; - } - - const Errno bsd_errno = Translate(descriptor->socket->Close()); - if (bsd_errno != Errno::SUCCESS) { - return bsd_errno; + std::shared_ptr socket; + { + // Free the slot first, so of two threads closing the same fd only one closes the socket. + // Like POSIX close(), the fd is released even if closing the socket fails. + std::scoped_lock lock{file_descriptors_mutex}; + if (!IsFileDescriptorValid(fd)) { + return Errno::BADF; + } + if (!file_descriptors[fd]->socket) { + LOG_WARNING(Service, "Uninitialized socket"); + return Errno::BADF; + } + socket = std::move(file_descriptors[fd]->socket); + file_descriptors[fd].reset(); } LOG_INFO(Service, "Close socket fd={}", fd); - - // Another thread may have closed this fd and reused the slot meanwhile - std::scoped_lock lock{file_descriptors_mutex}; - if (file_descriptors[fd] && file_descriptors[fd]->socket == descriptor->socket) { - file_descriptors[fd].reset(); - } - return bsd_errno; + return Translate(socket->Close()); } std::variant BSD_USA::DuplicateSocketImpl(s32 fd) { @@ -1070,7 +1070,8 @@ void BSD_USA::BuildErrnoResponse(HLERequestContext& ctx, Errno bsd_errno) const void BSD_USA::OnProxyPacketReceived(const Network::ProxyPacket& packet) { // Runs on the room thread. The copies keep each socket alive if a bsd thread closes it, so - // the bsd threads do not wait for the decompression below. + // the bsd threads do not wait for the decompression below. Only sockets that accept the + // packet are collected, so an unwanted packet is never decompressed. std::array, MAX_FD> sockets{}; size_t num_sockets = 0; { @@ -1081,7 +1082,8 @@ void BSD_USA::OnProxyPacketReceived(const Network::ProxyPacket& packet) { } // A duplicated fd shares its socket, which must still get each packet only once const auto sockets_end = sockets.begin() + num_sockets; - if (std::find(sockets.begin(), sockets_end, descriptor->socket) == sockets_end) { + if (std::find(sockets.begin(), sockets_end, descriptor->socket) == sockets_end && + descriptor->socket->AcceptsProxyPacket(packet)) { sockets[num_sockets++] = descriptor->socket; } } @@ -1090,13 +1092,19 @@ void BSD_USA::OnProxyPacketReceived(const Network::ProxyPacket& packet) { return; } - // Decompress once for all sockets that share the port + // Decompress once for all sockets that share the port. The room does not check the frame, so + // drop a malformed or oversized one instead of throwing on the room thread. + auto data = Common::Compression::TryDecompressDataZSTD(packet.data, MAX_ROOM_PACKET_SIZE); + if (!data) { + LOG_WARNING(Service, "Dropped a room packet with an invalid or oversized payload"); + return; + } const Network::ProxyPacket decompressed{ .local_endpoint = packet.local_endpoint, .remote_endpoint = packet.remote_endpoint, .protocol = packet.protocol, .broadcast = packet.broadcast, - .data = Common::Compression::DecompressDataZSTD(packet.data), + .data = std::move(*data), }; for (size_t i = 0; i < num_sockets; ++i) { sockets[i]->HandleProxyPacket(decompressed); diff --git a/src/core/hle/service/sockets/bsd.h b/src/core/hle/service/sockets/bsd.h index 559830d7a1..bff80d7c00 100644 --- a/src/core/hle/service/sockets/bsd.h +++ b/src/core/hle/service/sockets/bsd.h @@ -44,6 +44,8 @@ public: private: /// Maximum number of file descriptors static constexpr size_t MAX_FD = 128; + /// Largest decompressed room packet. A UDP datagram cannot be larger. + static constexpr size_t MAX_ROOM_PACKET_SIZE = 0x10000; struct FileDescriptor { std::shared_ptr socket; diff --git a/src/core/internal_network/network.cpp b/src/core/internal_network/network.cpp index d1c436aa08..806562076d 100644 --- a/src/core/internal_network/network.cpp +++ b/src/core/internal_network/network.cpp @@ -1095,6 +1095,10 @@ bool Socket::IsOpened() const { return fd != INVALID_SOCKET; } +bool Socket::AcceptsProxyPacket(const ProxyPacket& packet) { + return false; +} + void Socket::HandleProxyPacket(const ProxyPacket& packet) { LOG_WARNING(Network, "ProxyPacket received, but not in Proxy mode!"); } diff --git a/src/core/internal_network/socket_proxy.cpp b/src/core/internal_network/socket_proxy.cpp index 95f64b9bd0..f12891e8f5 100644 --- a/src/core/internal_network/socket_proxy.cpp +++ b/src/core/internal_network/socket_proxy.cpp @@ -30,17 +30,29 @@ ProxySocket::~ProxySocket() { fd = INVALID_SOCKET; } +bool ProxySocket::MatchesPacket(const ProxyPacket& packet) const { + if (protocol != packet.protocol || local_endpoint.portno != packet.remote_endpoint.portno || + closed) { + return false; + } + if (!broadcast && packet.broadcast) { + LOG_INFO(Network, "Received broadcast packet, but not configured for broadcast mode"); + return false; + } + return true; +} + +bool ProxySocket::AcceptsProxyPacket(const ProxyPacket& packet) { + std::lock_guard guard(packets_mutex); + return MatchesPacket(packet); +} + void ProxySocket::HandleProxyPacket(const ProxyPacket& packet) { { - // Runs on the room thread, while the game thread may bind, close or reconfigure the socket + // Runs on the room thread, while the game thread may bind, close or reconfigure the socket. + // Check again: the socket may have changed since AcceptsProxyPacket. std::lock_guard guard(packets_mutex); - if (protocol != packet.protocol || local_endpoint.portno != packet.remote_endpoint.portno || - closed) { - return; - } - - if (!broadcast && packet.broadcast) { - LOG_INFO(Network, "Received broadcast packet, but not configured for broadcast mode"); + if (!MatchesPacket(packet)) { return; } received_packets.push(packet); diff --git a/src/core/internal_network/socket_proxy.h b/src/core/internal_network/socket_proxy.h index 5d6e35693c..078baa7806 100644 --- a/src/core/internal_network/socket_proxy.h +++ b/src/core/internal_network/socket_proxy.h @@ -22,6 +22,8 @@ public: explicit ProxySocket() noexcept; ~ProxySocket() override; + bool AcceptsProxyPacket(const ProxyPacket& packet) override; + void HandleProxyPacket(const ProxyPacket& packet) override; Errno Initialize(Domain domain, Type type, Protocol socket_protocol) override; @@ -82,6 +84,9 @@ public: bool IsOpened() const override; private: + /// Expects packets_mutex to be held + bool MatchesPacket(const ProxyPacket& packet) const; + bool broadcast = false; bool closed = false; u32 send_timeout = 0; diff --git a/src/core/internal_network/sockets.h b/src/core/internal_network/sockets.h index 37cffd4651..29b5b0f18a 100644 --- a/src/core/internal_network/sockets.h +++ b/src/core/internal_network/sockets.h @@ -89,6 +89,9 @@ public: virtual bool IsOpened() const = 0; + /// Whether HandleProxyPacket would queue the packet; checked before decompressing it + virtual bool AcceptsProxyPacket(const ProxyPacket& packet) = 0; + /// Queues a room packet whose data is already decompressed virtual void HandleProxyPacket(const ProxyPacket& packet) = 0; @@ -164,6 +167,8 @@ public: bool IsOpened() const override; + bool AcceptsProxyPacket(const ProxyPacket& packet) override; + void HandleProxyPacket(const ProxyPacket& packet) override; private: -- cgit v1.2.3