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@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