From ed29c7313db6551c5785aa132f70d6e03590721d Mon Sep 17 00:00:00 2001 From: Jiangjie Gao Date: Mon, 22 Jan 2024 10:38:19 +0800 Subject: [PATCH 1/2] [core] Fixed thread safety using WSAOVERLAPPED in WSASendTo(#2838). The lpOverlapped parameter must be valid for the duration of the overlapped operation. If multiple I/O operations are simultaneously outstanding, each must reference a separate WSAOVERLAPPED structure. This reverts commit b1c0be20d2ef7d4d1ec9897eb49c194ea7c98993. resolves #973 #2632 #2834 #2838 --- CMakeLists.txt | 4 +++ srtcore/channel.cpp | 65 ++++++++++++++++++++++++++++----------------- srtcore/channel.h | 3 --- 3 files changed, 44 insertions(+), 28 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 82b0e3cff1..e5da8784f3 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -627,6 +627,10 @@ endif() message(STATUS "STDCXX_SYNC: ${ENABLE_STDCXX_SYNC}") message(STATUS "MONOTONIC_CLOCK: ${ENABLE_MONOTONIC_CLOCK}") +if (ENABLE_CXX11) + add_definitions(-DENABLE_CXX11=1) +endif() + if (ENABLE_SOCK_CLOEXEC) add_definitions(-DENABLE_SOCK_CLOEXEC=1) endif() diff --git a/srtcore/channel.cpp b/srtcore/channel.cpp index 0b448d6813..f0b4233309 100644 --- a/srtcore/channel.cpp +++ b/srtcore/channel.cpp @@ -143,14 +143,6 @@ srt::CChannel::CChannel() , m_bBindMasked(true) #endif { -#ifdef _WIN32 - SecureZeroMemory((PVOID)&m_SendOverlapped, sizeof(WSAOVERLAPPED)); - m_SendOverlapped.hEvent = WSACreateEvent(); - if (m_SendOverlapped.hEvent == NULL) { - LOGC(kmlog.Error, log << CONID() << "IPE: WSACreateEvent failed with error: " << NET_ERROR); - throw CUDTException(MJ_SETUP, MN_NORES, NET_ERROR); - } -#endif #ifdef SRT_ENABLE_PKTINFO // Do the check for ancillary data buffer size, kinda assertion static const size_t CMSG_MAX_SPACE = sizeof (CMSGNodeIPv4) + sizeof (CMSGNodeIPv6); @@ -166,12 +158,7 @@ srt::CChannel::CChannel() #endif } -srt::CChannel::~CChannel() -{ -#ifdef _WIN32 - WSACloseEvent(m_SendOverlapped.hEvent); -#endif -} +srt::CChannel::~CChannel() {} void srt::CChannel::createSocket(int family) { @@ -786,34 +773,62 @@ int srt::CChannel::sendto(const sockaddr_any& addr, CPacket& packet, const socka const int res = (int)::sendmsg(m_iSocket, &mh, 0); #else - DWORD size = (DWORD)(CPacket::HDR_SIZE + packet.getLength()); + DWORD size = (DWORD)(packet.m_PacketVector[0].size() + packet.m_PacketVector[1].size()); int addrsize = addr.size(); + class WSAEventRef + { + public: + WSAEventRef() + : e(::WSACreateEvent()) + { + } + ~WSAEventRef() + { + ::WSACloseEvent(e); + e = NULL; + } + void reset() + { + ::WSAResetEvent(e); + } + WSAEVENT Handle() + { + return e; + } - int res = ::WSASendTo(m_iSocket, (LPWSABUF)packet.m_PacketVector, 2, &size, 0, addr.get(), addrsize, &m_SendOverlapped, NULL); - + private: + WSAEVENT e; + }; +#if ENABLE_CXX11 + thread_local WSAEventRef lEvent; +#else + WSAEventRef lEvent; +#endif + WSAOVERLAPPED overlapped; + ::SecureZeroMemory(&overlapped, sizeof(overlapped)); + overlapped.hEvent = lEvent.Handle(); + int res = ::WSASendTo(m_iSocket, (LPWSABUF)packet.m_PacketVector, 2, &size, 0, addr.get(), addrsize, &overlapped, NULL); if (res == SOCKET_ERROR) { if (NET_ERROR == WSA_IO_PENDING) { - res = WSAWaitForMultipleEvents(1, &m_SendOverlapped.hEvent, TRUE, 100 /*ms*/, FALSE); - if (res == WAIT_FAILED) + DWORD flags = 0; + const bool completed = ::WSAGetOverlappedResult(m_iSocket, &overlapped, &size, TRUE, &flags); + if (completed) { - LOGC(kslog.Warn, log << "CChannel::WSAWaitForMultipleEvents: failed with " << NET_ERROR); - res = -1; + res = 0; } else { - DWORD dwFlags = 0; - const bool bCompleted = WSAGetOverlappedResult(m_iSocket, &m_SendOverlapped, &size, false, &dwFlags); - res = bCompleted ? 0 : -1; + LOGC(kslog.Warn, log << "CChannel::sendto call on ::WSAGetOverlappedResult failed with error: " << NET_ERROR); } + lEvent.reset(); } else { LOGC(kmlog.Error, log << CONID() << "WSASendTo failed with error: " << NET_ERROR); } } - WSAResetEvent(m_SendOverlapped.hEvent); res = (0 == res) ? size : -1; #endif diff --git a/srtcore/channel.h b/srtcore/channel.h index 6df7ec0cec..6b764763f0 100644 --- a/srtcore/channel.h +++ b/srtcore/channel.h @@ -176,9 +176,6 @@ class CChannel private: UDPSOCKET m_iSocket; // socket descriptor -#ifdef _WIN32 - mutable WSAOVERLAPPED m_SendOverlapped; -#endif // Mutable because when querying original settings // this comprises the cache for extracted values, From dd49c5b513463b59294f3b862ce323043861a01f Mon Sep 17 00:00:00 2001 From: Maxim Sharabayko Date: Tue, 16 Apr 2024 15:14:20 +0200 Subject: [PATCH 2/2] Apply suggestions from code review --- CMakeLists.txt | 4 ---- srtcore/channel.cpp | 2 +- 2 files changed, 1 insertion(+), 5 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index e5da8784f3..82b0e3cff1 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -627,10 +627,6 @@ endif() message(STATUS "STDCXX_SYNC: ${ENABLE_STDCXX_SYNC}") message(STATUS "MONOTONIC_CLOCK: ${ENABLE_MONOTONIC_CLOCK}") -if (ENABLE_CXX11) - add_definitions(-DENABLE_CXX11=1) -endif() - if (ENABLE_SOCK_CLOEXEC) add_definitions(-DENABLE_SOCK_CLOEXEC=1) endif() diff --git a/srtcore/channel.cpp b/srtcore/channel.cpp index f0b4233309..64be3ebfc4 100644 --- a/srtcore/channel.cpp +++ b/srtcore/channel.cpp @@ -799,7 +799,7 @@ int srt::CChannel::sendto(const sockaddr_any& addr, CPacket& packet, const socka private: WSAEVENT e; }; -#if ENABLE_CXX11 +#ifndef __MINGW32__ thread_local WSAEventRef lEvent; #else WSAEventRef lEvent;