[PATCH] ws2_32: hardcode optlen to 4 for setsockopt in the TCP_NODELAY case
According to MSDN [1], `TCP_NODELAY` parameter should be of type `BOOL` which is 4 bytes. Due to a bug [2] in rustc passing a byte instead of an int, any program written in rust that tries to set that option on a socket will fail with a "Invalid parameter supplied" error. Turns out that setsockopt on linux does not want optlen to be less than 4 bytes [3]. Since any value seems to be working on windows (tried with 1, 4, 8, 42, 1337), I hardcoded the length to be 4 on the wine side. This will fix any rust program using the hyper library to do HTTP requests. [1]: https://docs.microsoft.com/en-us/windows/win32/winsock/ipproto-tcp-socket-op... [2]: https://github.com/rust-lang/rust/blob/44593aeb1387b1be355aeaf0040d5927bd80f... [3]: https://github.com/torvalds/linux/blob/d58071a8a76d779eedab38033ae4c821c3029... Signed-off-by: Bastien Orivel <eijebong(a)bananium.fr> --- dlls/ws2_32/socket.c | 2 +- dlls/ws2_32/tests/sock.c | 9 +++++++++ 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/dlls/ws2_32/socket.c b/dlls/ws2_32/socket.c index 2db441bee3c..1ea83844f36 100644 --- a/dlls/ws2_32/socket.c +++ b/dlls/ws2_32/socket.c @@ -2868,7 +2868,7 @@ int WINAPI setsockopt( SOCKET s, int level, int optname, const char *optval, int switch(optname) { case TCP_NODELAY: - return server_setsockopt( s, IOCTL_AFD_WINE_SET_TCP_NODELAY, optval, optlen ); + return server_setsockopt( s, IOCTL_AFD_WINE_SET_TCP_NODELAY, optval, 4 ); default: FIXME("Unknown IPPROTO_TCP optname 0x%08x\n", optname); diff --git a/dlls/ws2_32/tests/sock.c b/dlls/ws2_32/tests/sock.c index 02713a7c625..6b2842383de 100644 --- a/dlls/ws2_32/tests/sock.c +++ b/dlls/ws2_32/tests/sock.c @@ -1259,6 +1259,15 @@ static void test_set_getsockopt(void) ok(err == SOCKET_ERROR && WSAGetLastError() == WSAEFAULT, "got %d with %d (expected SOCKET_ERROR with WSAEFAULT)\n", err, WSAGetLastError()); + /* TCP_NODELAY: optlen doesn't matter on windows, it should work with any value */ + value = 1; + err = setsockopt(s, IPPROTO_TCP, TCP_NODELAY, (char*)&value, 1); + ok (!err, "setsockopt TCP_NODELAY failed with optlen == 1\n"); + err = setsockopt(s, IPPROTO_TCP, TCP_NODELAY, (char*)&value, 4); + ok (!err, "setsockopt TCP_NODELAY failed with optlen == 4\n"); + err = setsockopt(s, IPPROTO_TCP, TCP_NODELAY, (char*)&value, 42); + ok (!err, "setsockopt TCP_NODELAY failed with optlen == 42\n"); + /* Test for erroneously passing a value instead of a pointer as optval */ size = sizeof(char); err = setsockopt(s, SOL_SOCKET, SO_DONTROUTE, (char *)1, size); -- 2.34.0
Hello Bastien, welcome to the Wine project, and thanks for the patch! I have some feedback. On Mon, Nov 29, 2021 at 2:41 PM Bastien Orivel <eijebong(a)bananium.fr> wrote:
@@ -2868,7 +2868,7 @@ int WINAPI setsockopt( SOCKET s, int level, int optname, const char *optval, int switch(optname) { case TCP_NODELAY: - return server_setsockopt( s, IOCTL_AFD_WINE_SET_TCP_NODELAY, optval, optlen ); + return server_setsockopt( s, IOCTL_AFD_WINE_SET_TCP_NODELAY, optval, 4 );
Couldn't it cause a segfault to call setsockopt on a single-byte value when setsockopt expects four bytes to be there? I think we want something like: case TCP_NODELAY: { INT nodelay = optval[1]; return server_setsockopt( s, IOCTL_AFD_WINE_SET_TCP_NODELAY, nodelay, sizeof(nodelay) ); }
@@ -1259,6 +1259,15 @@ static void test_set_getsockopt(void) ok(err == SOCKET_ERROR && WSAGetLastError() == WSAEFAULT, "got %d with %d (expected SOCKET_ERROR with WSAEFAULT)\n", err, WSAGetLastError());
+ /* TCP_NODELAY: optlen doesn't matter on windows, it should work with any value */ + value = 1; + err = setsockopt(s, IPPROTO_TCP, TCP_NODELAY, (char*)&value, 1); + ok (!err, "setsockopt TCP_NODELAY failed with optlen == 1\n"); + err = setsockopt(s, IPPROTO_TCP, TCP_NODELAY, (char*)&value, 4); + ok (!err, "setsockopt TCP_NODELAY failed with optlen == 4\n"); + err = setsockopt(s, IPPROTO_TCP, TCP_NODELAY, (char*)&value, 42); + ok (!err, "setsockopt TCP_NODELAY failed with optlen == 42\n");
What happens if the length is zero or negative? Or if the length is 4 but the value is 0x100 (no bits set in the first byte but a bit is set in the second byte)? I think we need tests for those cases too. -Alex
Hey, thanks for the quick feedback On 11/29/21 11:15 PM, Alex Henrie wrote:
Hello Bastien, welcome to the Wine project, and thanks for the patch! I have some feedback.
On Mon, Nov 29, 2021 at 2:41 PM Bastien Orivel <eijebong(a)bananium.fr> wrote:
@@ -2868,7 +2868,7 @@ int WINAPI setsockopt( SOCKET s, int level, int optname, const char *optval, int switch(optname) { case TCP_NODELAY: - return server_setsockopt( s, IOCTL_AFD_WINE_SET_TCP_NODELAY, optval, optlen ); + return server_setsockopt( s, IOCTL_AFD_WINE_SET_TCP_NODELAY, optval, 4 ); Couldn't it cause a segfault to call setsockopt on a single-byte value when setsockopt expects four bytes to be there? I think we want something like:
Yeah I wondered that too, from my tests it didn't cause any problems but I can see why it might be an issue. I'll change it to copy the value into an int and pass a pointer to that.
case TCP_NODELAY: { INT nodelay = optval[1]; return server_setsockopt( s, IOCTL_AFD_WINE_SET_TCP_NODELAY, nodelay, sizeof(nodelay) ); }
Shouldn't it be `INT nodelay = *optval;` or `optval[0]` ?
@@ -1259,6 +1259,15 @@ static void test_set_getsockopt(void) ok(err == SOCKET_ERROR && WSAGetLastError() == WSAEFAULT, "got %d with %d (expected SOCKET_ERROR with WSAEFAULT)\n", err, WSAGetLastError());
+ /* TCP_NODELAY: optlen doesn't matter on windows, it should work with any value */ + value = 1; + err = setsockopt(s, IPPROTO_TCP, TCP_NODELAY, (char*)&value, 1); + ok (!err, "setsockopt TCP_NODELAY failed with optlen == 1\n"); + err = setsockopt(s, IPPROTO_TCP, TCP_NODELAY, (char*)&value, 4); + ok (!err, "setsockopt TCP_NODELAY failed with optlen == 4\n"); + err = setsockopt(s, IPPROTO_TCP, TCP_NODELAY, (char*)&value, 42); + ok (!err, "setsockopt TCP_NODELAY failed with optlen == 42\n"); What happens if the length is zero or negative? Or if the length is 4 but the value is 0x100 (no bits set in the first byte but a bit is set in the second byte)? I think we need tests for those cases too.
Good question. After some testing I'm getting this: optlen = -1, 1, 4, 8, 64, 1337 with optval = 1, -> tcp_nodelay = 1 optlen = 4, optval = 0x100 -> tcp_nodelay = 0 It's only reading the first byte which makes sense. I'll add tests to confirm that. I will also add `getsockopt` calls to check that the value changed accordingly.
-Alex
On Mon, Nov 29, 2021 at 4:03 PM Bastien Orivel <eijebong(a)bananium.fr> wrote:
case TCP_NODELAY: { INT nodelay = optval[1]; return server_setsockopt( s, IOCTL_AFD_WINE_SET_TCP_NODELAY, nodelay, sizeof(nodelay) ); }
Shouldn't it be `INT nodelay = *optval;` or `optval[0]` ?
Yes, my neurons got crossed. Thanks for catching that.
After some testing I'm getting this:
optlen = -1, 1, 4, 8, 64, 1337 with optval = 1, -> tcp_nodelay = 1
optlen = 4, optval = 0x100 -> tcp_nodelay = 0
It's only reading the first byte which makes sense.
I'll add tests to confirm that. I will also add `getsockopt` calls to check that the value changed accordingly.
Great! -Alex
participants (2)
-
Alex Henrie -
Bastien Orivel