[PATCH] ws2_32: hardcode optlen to 4 for setsockopt in the TCP_NODELAY case

[Bastien Orivel]

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