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