Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added ::select to Windows sockets. #510

Merged
merged 1 commit into from
Dec 7, 2018
Merged

Added ::select to Windows sockets. #510

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
134 changes: 75 additions & 59 deletions srtcore/channel.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,10 +162,10 @@ void CChannel::open(const sockaddr* addr)
if (0 != ::getaddrinfo(NULL, "0", &hints, &res))
throw CUDTException(MJ_SETUP, MN_NORES, NET_ERROR);

if (0 != ::bind(m_iSocket, res->ai_addr, res->ai_addrlen))
if (0 != ::bind(m_iSocket, res->ai_addr, (int) res->ai_addrlen))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here socklen_t, too... probably.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The last argument of the ::bind has type int. I think casting to socklen_t is not correct, although it is a typedef of int.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Depends. The version that I found in my manpage has the 3rd argument of ::bind set as socklen_t. Might be that there's some difference on Windows, but still the type of this argument should be the same as the type of addrinfo::ai_addrlen. This cast is then at least nonportable.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ethouris, you are right on this. socklen_t is better.

throw CUDTException(MJ_SETUP, MN_NORES, NET_ERROR);
memcpy(&m_BindAddr, res->ai_addr, res->ai_addrlen);
m_BindAddr.len = res->ai_addrlen;
m_BindAddr.len = (socklen_t) res->ai_addrlen;

::freeaddrinfo(res);
}
Expand Down Expand Up @@ -226,31 +226,31 @@ void CChannel::setUDPSockOpt()
}
#endif


#ifdef UNIX
// Set non-blocking I/O
// UNIX does not support SO_RCVTIMEO
int opts = ::fcntl(m_iSocket, F_GETFL);
if (-1 == ::fcntl(m_iSocket, F_SETFL, opts | O_NONBLOCK))
throw CUDTException(MJ_SETUP, MN_NORES, NET_ERROR);
#elif defined(_WIN32)
u_long nonBlocking = 1;
if (0 != ioctlsocket (m_iSocket, FIONBIO, &nonBlocking))
throw CUDTException (MJ_SETUP, MN_NORES, NET_ERROR);
#else
timeval tv;
tv.tv_sec = 0;
#if defined (BSD) || defined (OSX) || (TARGET_OS_IOS == 1) || (TARGET_OS_TV == 1)
// Known BSD bug as the day I wrote this code.
// A small time out value will cause the socket to block forever.
tv.tv_usec = 10000;
#else
tv.tv_usec = 100;
#endif

#ifdef UNIX
// Set non-blocking I/O
// UNIX does not support SO_RCVTIMEO
int opts = ::fcntl(m_iSocket, F_GETFL);
if (-1 == ::fcntl(m_iSocket, F_SETFL, opts | O_NONBLOCK))
throw CUDTException(MJ_SETUP, MN_NORES, NET_ERROR);
#elif defined(_WIN32)
u_long nonBlocking = 1;
if (0 != ioctlsocket (m_iSocket, FIONBIO, &nonBlocking))
throw CUDTException (MJ_SETUP, MN_NORES, NET_ERROR);
#else
// Set receiving time-out value
if (0 != ::setsockopt(m_iSocket, SOL_SOCKET, SO_RCVTIMEO, (char *)&tv, sizeof(timeval)))
throw CUDTException(MJ_SETUP, MN_NORES, NET_ERROR);
#endif
#if defined (BSD) || defined (OSX) || (TARGET_OS_IOS == 1) || (TARGET_OS_TV == 1)
// Known BSD bug as the day I wrote this code.
// A small time out value will cause the socket to block forever.
tv.tv_usec = 10000;
#else
tv.tv_usec = 100;
#endif
// Set receiving time-out value
if (0 != ::setsockopt(m_iSocket, SOL_SOCKET, SO_RCVTIMEO, (char *)&tv, sizeof(timeval)))
throw CUDTException(MJ_SETUP, MN_NORES, NET_ERROR);
#endif
}

void CChannel::close() const
Expand Down Expand Up @@ -390,7 +390,7 @@ int CChannel::sendto(const sockaddr* addr, CPacket& packet) const
// convert control information into network order
// XXX USE HtoNLA!
if (packet.isControl())
for (int i = 0, n = packet.getLength() / 4; i < n; ++ i)
for (ptrdiff_t i = 0, n = (ptrdiff_t) packet.getLength() / 4; i < n; ++i)
*((uint32_t *)packet.m_pcData + i) = htonl(*((uint32_t *)packet.m_pcData + i));

// convert packet header into network order
Expand All @@ -415,7 +415,7 @@ int CChannel::sendto(const sockaddr* addr, CPacket& packet) const

int res = ::sendmsg(m_iSocket, &mh, 0);
#else
DWORD size = CPacket::HDR_SIZE + packet.getLength();
DWORD size = (DWORD) (CPacket::HDR_SIZE + packet.getLength());
int addrsize = m_iSockAddrSize;
int res = ::WSASendTo(m_iSocket, (LPWSABUF)packet.m_PacketVector, 2, &size, 0, addr, addrsize, NULL, NULL);
res = (0 == res) ? size : -1;
Expand All @@ -433,7 +433,7 @@ int CChannel::sendto(const sockaddr* addr, CPacket& packet) const

if (packet.isControl())
{
for (int l = 0, n = packet.getLength() / 4; l < n; ++ l)
for (ptrdiff_t l = 0, n = packet.getLength() / 4; l < n; ++ l)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is cast to (ptrdiff_t) not required, while it's required in the loop above?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because in the loop above 'k' is not used to offset a pointer.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The "above loop" I meant this in line 393 with 'i' control variable.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ethouris, do you mean this lines:
n = (ptrdiff_t) packet.getLength() / 4
and
n = packet.getLength() / 4
They should be the same, I missed this. I will remove this casting.

*((uint32_t *)packet.m_pcData + l) = ntohl(*((uint32_t *)packet.m_pcData + l));
}

Expand All @@ -443,30 +443,42 @@ int CChannel::sendto(const sockaddr* addr, CPacket& packet) const
EReadStatus CChannel::recvfrom(sockaddr* addr, CPacket& packet) const
{
EReadStatus status = RST_OK;
int msg_flags = 0;
int recv_size = -1;

#ifndef _WIN32
msghdr mh;
mh.msg_name = addr;
mh.msg_namelen = m_iSockAddrSize;
mh.msg_iov = packet.m_PacketVector;
mh.msg_iovlen = 2;
mh.msg_control = NULL;
mh.msg_controllen = 0;
mh.msg_flags = 0;

#ifdef UNIX
#if defined(UNIX) || defined(_WIN32)
fd_set set;
timeval tv;
FD_ZERO(&set);
FD_SET(m_iSocket, &set);
tv.tv_sec = 0;
tv.tv_sec = 0;
tv.tv_usec = 10000;
::select(m_iSocket+1, &set, NULL, &set, &tv);
const int select_ret = ::select((int) m_iSocket + 1, &set, NULL, &set, &tv);
#else
const int select_ret = 1; // the socket is expected to be in the blocking mode itself
#endif

int res = ::recvmsg(m_iSocket, &mh, 0);
int msg_flags = mh.msg_flags;
if (select_ret == 0) // timeout
{
packet.setLength(-1);
return RST_AGAIN;
}

#ifndef _WIN32
if (select_ret > 0)
{
msghdr mh;
mh.msg_name = addr;
mh.msg_namelen = m_iSockAddrSize;
mh.msg_iov = packet.m_PacketVector;
mh.msg_iovlen = 2;
mh.msg_control = NULL;
mh.msg_controllen = 0;
mh.msg_flags = 0;

recv_size = ::recvmsg(m_iSocket, &mh, 0);
msg_flags = mh.msg_flags;
}

// Note that there are exactly four groups of possible errors
// reported by recvmsg():
Expand All @@ -489,9 +501,10 @@ EReadStatus CChannel::recvfrom(sockaddr* addr, CPacket& packet) const
// Return: RST_ERROR. This will simply make the worker thread exit, which is
// expected to happen after CChannel::close() is called by another thread.

if (res == -1)
// We do not handle <= SOCKET_ERROR as they are handled further by checking the recv_size
if (select_ret == -1 || recv_size == -1)
{
int err = NET_ERROR;
const int err = NET_ERROR;
if (err == EAGAIN || err == EINTR || err == ECONNREFUSED) // For EAGAIN, this isn't an error, just a useless call.
{
status = RST_AGAIN;
Expand Down Expand Up @@ -522,20 +535,23 @@ EReadStatus CChannel::recvfrom(sockaddr* addr, CPacket& packet) const
// value one Windows than 0, unless this procedure below is rewritten
// to use WSARecvMsg().

DWORD size = CPacket::HDR_SIZE + packet.getLength();
int recv_ret = SOCKET_ERROR;
DWORD flag = 0;
int addrsize = m_iSockAddrSize;

int msg_flags = 0;
int sockerror = ::WSARecvFrom(m_iSocket, (LPWSABUF)packet.m_PacketVector, 2, &size, &flag, addr, &addrsize, NULL, NULL);
int res;
if (sockerror == 0)
if (select_ret > 0) // the total number of socket handles that are ready
{
res = size;
DWORD size = (DWORD) (CPacket::HDR_SIZE + packet.getLength());
int addrsize = m_iSockAddrSize;

recv_ret = ::WSARecvFrom(m_iSocket, (LPWSABUF)packet.m_PacketVector, 2, &size, &flag, addr, &addrsize, NULL, NULL);
if (recv_ret == 0)
recv_size = size;
}
else // == SOCKET_ERROR

// We do not handle <= SOCKET_ERROR as they are handled further by checking the recv_size
if (select_ret == SOCKET_ERROR || recv_ret == SOCKET_ERROR) // == SOCKET_ERROR
{
res = -1;
recv_size = -1;
// On Windows this is a little bit more complicated, so simply treat every error
// as an "again" situation. This should still be probably fixed, but it needs more
// thorough research. For example, the problem usually reported from here is
Expand All @@ -551,7 +567,7 @@ EReadStatus CChannel::recvfrom(sockaddr* addr, CPacket& packet) const
WSA_OPERATION_ABORTED
};
static const int* fatals_end = fatals + Size(fatals);
int err = NET_ERROR;
const int err = NET_ERROR;
if (std::find(fatals, fatals_end, err) != fatals_end)
{
HLOGC(mglog.Debug, log << CONID() << "(sys)WSARecvFrom: " << SysStrError(err) << " [" << err << "]");
Expand All @@ -572,10 +588,10 @@ EReadStatus CChannel::recvfrom(sockaddr* addr, CPacket& packet) const


// Sanity check for a case when it didn't fill in even the header
if ( size_t(res) < CPacket::HDR_SIZE )
if (size_t(recv_size) < CPacket::HDR_SIZE)
{
status = RST_AGAIN;
HLOGC(mglog.Debug, log << CONID() << "POSSIBLE ATTACK: received too short packet with " << res << " bytes");
HLOGC(mglog.Debug, log << CONID() << "POSSIBLE ATTACK: received too short packet with " << recv_size << " bytes");
goto Return_error;
}

Expand All @@ -598,13 +614,13 @@ EReadStatus CChannel::recvfrom(sockaddr* addr, CPacket& packet) const
// packet was received, so the packet will be then retransmitted.
if ( msg_flags != 0 )
{
HLOGC(mglog.Debug, log << CONID() << "NET ERROR: packet size=" << res
HLOGC(mglog.Debug, log << CONID() << "NET ERROR: packet size=" << recv_size
<< " msg_flags=0x" << hex << msg_flags << ", possibly MSG_TRUNC (0x" << hex << int(MSG_TRUNC) << ")");
status = RST_AGAIN;
goto Return_error;
}

packet.setLength(res - CPacket::HDR_SIZE);
packet.setLength(recv_size - CPacket::HDR_SIZE);

// convert back into local host order
// XXX use NtoHLA().
Expand Down