[Windows] Use `fd_set` according to Winsock2 specifics #4954

lxbndr · 2024-05-12T20:16:25Z

On Windows, socket handles in a fd_set are not represented as bit flags as in Berkeley sockets. While we have no fd_set dynamic growth in this implementation, the FD_SETSIZE defined as 1024 in CoreFoundation_Prefix.h should be enough for majority of tasks.

This also includes a test for SocketPort which uses CFSocket internally. This test fails (hangs or crashes with memory corruption error) without the fix.

lxbndr · 2024-05-12T20:22:24Z

@hjyamauchi this patch has minor differences from that version you checked. Just to be sure, could you please validate original issue with this implementation?

parkera · 2024-05-13T17:32:21Z

@swift-ci test

Fixes apple/swift#73532. On Windows, socket handles in a `fd_set` are not represented as bit flags as in Berkeley sockets. While we have no `fd_set` dynamic growth in this implementation, the `FD_SETSIZE` defined as 1024 in `CoreFoundation_Prefix.h` should be enough for majority of tasks.

lxbndr · 2024-05-13T22:45:47Z

Force-pushed NFC (removed unused variable x in the for-loop in the test code).

compnerd · 2024-05-13T23:11:16Z

@swift-ci please test

hjyamauchi · 2024-05-14T03:09:49Z

@hjyamauchi this patch has minor differences from that version you checked. Just to be sure, could you please validate original issue with this implementation?

Validated. It worked fine.

hjyamauchi · 2024-05-14T03:09:59Z

CC @tristanlabelle

tristanlabelle · 2024-05-14T15:00:46Z

CoreFoundation/RunLoop.subproj/CFSocket.c

+    if (CFDataGetLength(fdSet) == 0) {
+        return 0;
+    }
+    return FD_SETSIZE;


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.

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.

Resolved per convo about large FD_SETSIZE override

tristanlabelle · 2024-05-14T15:03:44Z

CoreFoundation/RunLoop.subproj/CFSocket.c

 }

 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) {


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.

Resolved per convo about large FD_SETSIZE override.

tristanlabelle · 2024-05-14T15:05:05Z

CoreFoundation/RunLoop.subproj/CFSocket.c

            retval = true;
-            FD_SET(sock, (fd_set *)fds_bits);
+            FD_SET(sock, fds);


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.

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

tristanlabelle · 2024-05-14T15:06:32Z

CoreFoundation/RunLoop.subproj/CFSocket.c

+        }
+
+        fd_set *fds = (fd_set *)CFDataGetMutableBytePtr(d);
+        fd_set invalidFds;


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

Resolving per convo about large FD_SETSIZE override.

tristanlabelle

This change is very necessary, thanks! I think there is still a problem with the Windows implementation. The code is treating fd_set as fixed-size, which will cause silent failures if we exceed the 64 array size in the structure (not a problem on macOS with bitset resizing). Reading the docs, the fd_set structure is designed to have a trailing array that can be bigger than the default 64. I think the logic should support resizing the array past 64 as needed, so that we don't have this additional silent failure condition on Windows.

lxbndr · 2024-05-14T22:39:34Z

@tristanlabelle so the major concern is the static non-growing structure, right? I agree that it is not comfortable to live with this restriction, and my initial intention was to make it dynamic.

Not sure if it is really ok to manipulate inner structure array, but I think it worth to try. I am suspicious about changing it on the fly, because FD_SETSIZE is a define, and it must be defined before including winsock2 header. So it could be baked into some macros like FD_SET or FD_ZERO.

I initially planned not to grow the array, but create additional fd_sets as needed, so we would have growing array of fd_sets instead.

Also, some good news. The default cap is 64, which is pretty low. But look what we got here. We already have FD_SETSIZE defined as 1024. That's a plenty of space.

I could try dynamic sizing, but probably not early than this weekend 😔 If that not blocking anything critical for you, we could keep this for now. Or we could land it and I'll do a follow up.

And... Do you think it makes sense to fight 1024 sockets limit, while corelibs are gradually moving away from CoreFoundation as backing implementation? WDYT?

tristanlabelle · 2024-05-16T15:27:45Z

@lxbndr Ah given the 1024 socket override I don't have much of a concern. I'll resolve my comments and file a follow-up issue.

You're right that we wouldn't be able to use FD_SET or FD_ZERO on Windows with dynamic sizing, they would need their own implementation.

tristanlabelle · 2024-05-16T15:30:49Z

Actually FD_SET is the only problematic one for dynamic sizing:

#define FD_CLR(fd, set) do { \
    u_int __i; \
    for (__i = 0; __i < ((fd_set FAR *)(set))->fd_count ; __i++) { \
        if (((fd_set FAR *)(set))->fd_array[__i] == fd) { \
            while (__i < ((fd_set FAR *)(set))->fd_count-1) { \
                ((fd_set FAR *)(set))->fd_array[__i] = \
                    ((fd_set FAR *)(set))->fd_array[__i+1]; \
                __i++; \
            } \
            ((fd_set FAR *)(set))->fd_count--; \
            break; \
        } \
    } \
} while(0)

#define FD_SET(fd, set) do { \
    if (((fd_set FAR *)(set))->fd_count < FD_SETSIZE) \
        ((fd_set FAR *)(set))->fd_array[((fd_set FAR *)(set))->fd_count++]=(fd);\
} while(0)

#define FD_ZERO(set) (((fd_set FAR *)(set))->fd_count=0)

#define FD_ISSET(fd, set) __WSAFDIsSet((SOCKET)(fd), (fd_set FAR *)(set))

tristanlabelle · 2024-05-16T15:36:02Z

Filed #4958 as follow-up

compnerd · 2024-05-16T20:07:48Z

@swift-ci please test Windows platform

compnerd · 2024-05-17T17:45:06Z

@swift-ci please test macOS platform

hjyamauchi · 2024-05-20T00:38:08Z

@swift-ci please test macOS platform

hjyamauchi · 2024-05-20T22:32:18Z

Does it looks like the macos CI was broken on May 11 before this PR was created? The latest run 466 looks broken in the same way as the May 11 run 461.

May 20 (the latest):
https://ci.swift.org/job/swift-corelibs-foundation-PR-macOS/466/

May 11:
https://ci.swift.org/job/swift-corelibs-foundation-PR-macOS/461/

Failed Tests (24):
  SwiftXCTestFunctionalTests :: ArgumentParser/main.swift
  SwiftXCTestFunctionalTests :: Asynchronous/Expectations/main.swift
  SwiftXCTestFunctionalTests :: Asynchronous/Handler/main.swift
  SwiftXCTestFunctionalTests :: Asynchronous/Misuse/main.swift
  SwiftXCTestFunctionalTests :: Asynchronous/Notifications/Expectations/main.swift
  SwiftXCTestFunctionalTests :: Asynchronous/Notifications/Handler/main.swift
  SwiftXCTestFunctionalTests :: Asynchronous/Predicates/Expectations/main.swift
  SwiftXCTestFunctionalTests :: Asynchronous/Predicates/Handler/main.swift
  SwiftXCTestFunctionalTests :: Asynchronous/Use/main.swift
  SwiftXCTestFunctionalTests :: EqualityWithAccuracy/main.swift
  SwiftXCTestFunctionalTests :: ErrorHandling/main.swift
  SwiftXCTestFunctionalTests :: FailingTestSuite/main.swift
  SwiftXCTestFunctionalTests :: FailureMessagesTestCase/main.swift
  SwiftXCTestFunctionalTests :: ListTests/main.swift
  SwiftXCTestFunctionalTests :: Observation/All/main.swift
  SwiftXCTestFunctionalTests :: Observation/MultipleObservers/main.swift
  SwiftXCTestFunctionalTests :: Observation/Selected/main.swift
  SwiftXCTestFunctionalTests :: Performance/Misuse/main.swift
  SwiftXCTestFunctionalTests :: Performance/main.swift
  SwiftXCTestFunctionalTests :: SelectedTest/main.swift
  SwiftXCTestFunctionalTests :: SingleFailingTestCase/main.swift
  SwiftXCTestFunctionalTests :: SkippingTestCase/main.swift
  SwiftXCTestFunctionalTests :: TestCaseLifecycle/Misuse/main.swift
  SwiftXCTestFunctionalTests :: TestCaseLifecycle/main.swift

compnerd · 2024-05-21T20:07:45Z

This is really windows specific, the tests haven't regressed, @parkera okay to merge ignoring the macOS issue?

parkera requested a review from compnerd May 13, 2024 17:32

lxbndr force-pushed the win-fd-set branch from a44167a to d3871a5 Compare May 13, 2024 22:43

tristanlabelle reviewed May 14, 2024

View reviewed changes

tristanlabelle suggested changes May 14, 2024

View reviewed changes

tristanlabelle approved these changes May 16, 2024

View reviewed changes

hjyamauchi mentioned this pull request May 17, 2024

Workaround for https://github.com/apple/swift/issues/73532 #4951

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Windows] Use `fd_set` according to Winsock2 specifics #4954

[Windows] Use `fd_set` according to Winsock2 specifics #4954

lxbndr commented May 12, 2024

lxbndr commented May 12, 2024

parkera commented May 13, 2024

lxbndr commented May 13, 2024

compnerd commented May 13, 2024

hjyamauchi commented May 14, 2024

hjyamauchi commented May 14, 2024

tristanlabelle May 14, 2024

lxbndr May 14, 2024

tristanlabelle May 16, 2024

tristanlabelle May 14, 2024

tristanlabelle May 16, 2024

tristanlabelle May 14, 2024

tristanlabelle May 16, 2024

tristanlabelle May 14, 2024

tristanlabelle May 16, 2024

tristanlabelle left a comment

lxbndr commented May 14, 2024

tristanlabelle commented May 16, 2024

tristanlabelle commented May 16, 2024

tristanlabelle commented May 16, 2024

compnerd commented May 16, 2024

compnerd commented May 17, 2024

hjyamauchi commented May 20, 2024

hjyamauchi commented May 20, 2024

compnerd commented May 21, 2024

[Windows] Use fd_set according to Winsock2 specifics #4954

Are you sure you want to change the base?

[Windows] Use fd_set according to Winsock2 specifics #4954

Conversation

lxbndr commented May 12, 2024

lxbndr commented May 12, 2024

parkera commented May 13, 2024

lxbndr commented May 13, 2024

compnerd commented May 13, 2024

hjyamauchi commented May 14, 2024

hjyamauchi commented May 14, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tristanlabelle left a comment

Choose a reason for hiding this comment

lxbndr commented May 14, 2024

tristanlabelle commented May 16, 2024

tristanlabelle commented May 16, 2024

tristanlabelle commented May 16, 2024

compnerd commented May 16, 2024

compnerd commented May 17, 2024

hjyamauchi commented May 20, 2024

hjyamauchi commented May 20, 2024

compnerd commented May 21, 2024

[Windows] Use `fd_set` according to Winsock2 specifics #4954

[Windows] Use `fd_set` according to Winsock2 specifics #4954