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

[Windows] Use fd_set according to Winsock2 specifics #4954

Merged
merged 1 commit into from
May 30, 2024
Merged

[Windows] Use fd_set according to Winsock2 specifics #4954

Show file tree
Hide file tree
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
80 changes: 74 additions & 6 deletions CoreFoundation/RunLoop.subproj/CFSocket.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -213,13 +213,30 @@ CF_INLINE int __CFSocketLastError(void) {
}

CF_INLINE CFIndex __CFSocketFdGetSize(CFDataRef fdSet) {
#if TARGET_OS_WIN32
if (CFDataGetLength(fdSet) == 0) {
return 0;
}
return FD_SETSIZE;
Copy link
Contributor

Choose a reason for hiding this comment

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

My understanding of the structure on Windows is that it has a default size but it's legit to allocate a bigger buffer and treat the array as bigger than FD_SETSIZE. I think a better implementation would calculate the number of entries from CFDataGetLength after removing the size of the count field. This would also avoid the implicit dependency where this code is assuming that the data length is at least that of fd_set.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ahh, I think I will update the patch and exclude this function from Win build at all. It is not used in any of Windows code paths except one place where its result is ignored anyway.

Copy link
Contributor

Choose a reason for hiding this comment

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

Resolved per convo about large FD_SETSIZE override

#else
return NBBY * CFDataGetLength(fdSet);
#endif
}

CF_INLINE Boolean __CFSocketFdSet(CFSocketNativeHandle sock, CFMutableDataRef fdSet) {
/* returns true if a change occurred, false otherwise */
Boolean retval = false;
if (INVALID_SOCKET != sock && 0 <= sock) {
fd_set *fds;
#if TARGET_OS_WIN32
if (CFDataGetLength(fdSet) == 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this logic should support resizing the buffer past sizeof(fd_set) since fd_set's trailing array appears meant to be used as elastic-sized, and we have nothing to detect the error condition if we use a fixed size and can't add a new socket.

Copy link
Contributor

Choose a reason for hiding this comment

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

Resolved per convo about large FD_SETSIZE override.

CFDataIncreaseLength(fdSet, sizeof(fd_set));
fds = (fd_set *)CFDataGetMutableBytePtr(fdSet);
FD_ZERO(fds);
} else {
fds = (fd_set *)CFDataGetMutableBytePtr(fdSet);
}
#else
CFIndex numFds = NBBY * CFDataGetLength(fdSet);
fd_mask *fds_bits;
if (sock >= numFds) {
Expand All @@ -230,9 +247,11 @@ CF_INLINE Boolean __CFSocketFdSet(CFSocketNativeHandle sock, CFMutableDataRef fd
} else {
fds_bits = (fd_mask *)CFDataGetMutableBytePtr(fdSet);
}
if (!FD_ISSET(sock, (fd_set *)fds_bits)) {
fds = (fd_set *)fds_bits;
#endif
if (!FD_ISSET(sock, fds)) {
retval = true;
FD_SET(sock, (fd_set *)fds_bits);
FD_SET(sock, fds);
Copy link
Contributor

Choose a reason for hiding this comment

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

The macOS code made sure to resize so knows that this will succeed. However the Windows code is capping at 64 sockets and does not handle failure.

I think we should avoid using FD_SET on Windows because it has to assume an array size of 64, but fd_set is designed such that the allocation can make the trailing array bigger than that if it needs to.

Copy link
Contributor

Choose a reason for hiding this comment

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

Resolved per convo about large FD_SETSIZE override, although we could add an assert.

}
}
return retval;
Expand Down Expand Up @@ -416,6 +435,15 @@ CF_INLINE Boolean __CFSocketFdClr(CFSocketNativeHandle sock, CFMutableDataRef fd
/* returns true if a change occurred, false otherwise */
Boolean retval = false;
if (INVALID_SOCKET != sock && 0 <= sock) {
#if TARGET_OS_WIN32
if (CFDataGetLength(fdSet) > 0) {
fd_set *fds = (fd_set *)CFDataGetMutableBytePtr(fdSet);
if (FD_ISSET(sock, fds)) {
retval = true;
FD_CLR(sock, fds);
}
}
#else
CFIndex numFds = NBBY * CFDataGetLength(fdSet);
fd_mask *fds_bits;
if (sock < numFds) {
Expand All @@ -425,6 +453,7 @@ CF_INLINE Boolean __CFSocketFdClr(CFSocketNativeHandle sock, CFMutableDataRef fd
FD_CLR(sock, (fd_set *)fds_bits);
}
}
#endif
}
return retval;
}
Expand Down Expand Up @@ -1188,6 +1217,27 @@ static void
clearInvalidFileDescriptors(CFMutableDataRef d)
{
if (d) {
#if TARGET_OS_WIN32
if (CFDataGetLength(d) == 0) {
return;
}

fd_set *fds = (fd_set *)CFDataGetMutableBytePtr(d);
fd_set invalidFds;
Copy link
Contributor

Choose a reason for hiding this comment

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

There could be more than 64 invalid sockets if fds->fd_count is bigger than 64.

Copy link
Contributor

Choose a reason for hiding this comment

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

Resolving per convo about large FD_SETSIZE override.

FD_ZERO(&invalidFds);
// Gather all invalid sockets into invalidFds set
for (u_int idx = 0; idx < fds->fd_count; idx++) {
SOCKET socket = fds->fd_array[idx];
if (! __CFNativeSocketIsValid(socket)) {
FD_SET(socket, &invalidFds);
}
}
// Remove invalid sockets from source set
for (u_int idx = 0; idx < invalidFds.fd_count; idx++) {
SOCKET socket = invalidFds.fd_array[idx];
FD_CLR(socket, fds);
}
#else
SInt32 count = __CFSocketFdGetSize(d);
fd_set* s = (fd_set*) CFDataGetMutableBytePtr(d);
for (SInt32 idx = 0; idx < count; idx++) {
Expand All @@ -1196,14 +1246,13 @@ clearInvalidFileDescriptors(CFMutableDataRef d)
FD_CLR(idx, s);
}
}
#endif
}
}

static void
manageSelectError()
manageSelectError(SInt32 selectError)
{
SInt32 selectError = __CFSocketLastError();

__CFSOCKETLOG("socket manager received error %ld from select", (long)selectError);

if (EBADF == selectError) {
Expand Down Expand Up @@ -1263,8 +1312,15 @@ static void *__CFSocketManager(void * arg)
SInt32 nrfds, maxnrfds, fdentries = 1;
SInt32 rfds, wfds;
fd_set *exceptfds = NULL;
#if TARGET_OS_WIN32
fd_set *writefds = (fd_set *)CFAllocatorAllocate(kCFAllocatorSystemDefault, sizeof(fd_set), 0);
fd_set *readfds = (fd_set *)CFAllocatorAllocate(kCFAllocatorSystemDefault, sizeof(fd_set), 0);
FD_ZERO(writefds);
FD_ZERO(readfds);
#else
fd_set *writefds = (fd_set *)CFAllocatorAllocate(kCFAllocatorSystemDefault, fdentries * sizeof(fd_mask), 0);
fd_set *readfds = (fd_set *)CFAllocatorAllocate(kCFAllocatorSystemDefault, fdentries * sizeof(fd_mask), 0);
#endif
fd_set *tempfds;
SInt32 idx, cnt;
uint8_t buffer[256];
Expand All @@ -1290,6 +1346,11 @@ static void *__CFSocketManager(void * arg)
free(readBuffer);
free(writeBuffer);
#endif

#if TARGET_OS_WIN32
// This parameter is ignored by `select` from Winsock2 API
maxnrfds = INT_MAX;
#else
rfds = __CFSocketFdGetSize(__CFReadSocketsFds);
wfds = __CFSocketFdGetSize(__CFWriteSocketsFds);
maxnrfds = __CFMax(rfds, wfds);
Expand All @@ -1300,6 +1361,7 @@ static void *__CFSocketManager(void * arg)
}
memset(writefds, 0, fdentries * sizeof(fd_mask));
memset(readfds, 0, fdentries * sizeof(fd_mask));
#endif
CFDataGetBytes(__CFWriteSocketsFds, CFRangeMake(0, CFDataGetLength(__CFWriteSocketsFds)), (UInt8 *)writefds);
CFDataGetBytes(__CFReadSocketsFds, CFRangeMake(0, CFDataGetLength(__CFReadSocketsFds)), (UInt8 *)readfds);

Expand Down Expand Up @@ -1345,7 +1407,13 @@ static void *__CFSocketManager(void * arg)
}
#endif

SInt32 error = 0;
nrfds = select(maxnrfds, readfds, writefds, exceptfds, pTimeout);
if (nrfds < 0) {
// Store error as early as possible, as the code below could
// reset it and make late check unreliable.
error = __CFSocketLastError();
}

#if defined(LOG_CFSOCKET) && defined(DEBUG_POLLING_SELECT)
__CFSOCKETLOG("socket manager woke from select, ret=%ld", (long)nrfds);
Expand Down Expand Up @@ -1434,7 +1502,7 @@ static void *__CFSocketManager(void * arg)
}

if (0 > nrfds) {
manageSelectError();
manageSelectError(error);
continue;
}
if (FD_ISSET(__CFWakeupSocketPair[1], readfds)) {
Expand Down
44 changes: 44 additions & 0 deletions Tests/Foundation/Tests/TestSocketPort.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,11 +106,55 @@ class TestSocketPort : XCTestCase {
}
}

func testSendingMultipleMessagesRemoteToLocal() throws {
var localPorts = [SocketPort]()
var remotePorts = [SocketPort]()
var delegates = [TestPortDelegateWithBlock]()

let data = Data("I cannot weave".utf8)

for _ in 0..<128 {
let local = try XCTUnwrap(SocketPort(tcpPort: 0))
let tcpPort = try UInt16(XCTUnwrap(tcpOrUdpPort(of: local)))
let remote = try XCTUnwrap(SocketPort(remoteWithTCPPort: tcpPort, host: "localhost"))

let received = expectation(description: "Message received")

let localDelegate = TestPortDelegateWithBlock { message in
XCTAssertEqual(message.components as? [AnyHashable], [data as NSData])
received.fulfill()
}

localPorts.append(local)
remotePorts.append(remote)
delegates.append(localDelegate)

local.setDelegate(localDelegate)
local.schedule(in: .main, forMode: .default)
remote.schedule(in: .main, forMode: .default)
}

withExtendedLifetime(delegates) {
for remote in remotePorts {
let sent = remote.send(before: Date(timeIntervalSinceNow: 5), components: NSMutableArray(array: [data]), from: nil, reserved: 0)
XCTAssertTrue(sent)
}
waitForExpectations(timeout: 5.0)
}

for port in localPorts + remotePorts {
port.setDelegate(nil)
port.remove(from: .main, forMode: .default)
port.invalidate()
}
}

static var allTests: [(String, (TestSocketPort) -> () throws -> Void)] {
return [
("testRemoteSocketPortsAreUniqued", testRemoteSocketPortsAreUniqued),
("testInitPicksATCPPort", testInitPicksATCPPort),
("testSendingOneMessageRemoteToLocal", testSendingOneMessageRemoteToLocal),
("testSendingMultipleMessagesRemoteToLocal", testSendingMultipleMessagesRemoteToLocal),
]
}
}