[PATCH v2 1/6] ws2_32/tests: Add tests for setting and getting socket options with shorter length.

[Zebediah Figura]

On 3/3/22 15:16, Paul Gofman wrote:
> Signed-off-by: Paul Gofman <pgofman at codeweavers.com>
> ---
>       v2:
>            - also check for nonzero optval where appropriate throughout the patchset.
> 
>       The aim of the patchset is to fix setting and getting 32bit sized socket options
>       when the beaviour is different between Windows and Linux setsockopt() and
>       getsockopt(). That behaviour partially regressed on the move from ws2_32 to ntdll:
>       setsockopt() had the code which would adjust the size for option with the provided
>       length less than 4 bytes (which helped, e. g., SO_REUSEADDR and SO_KEEPALIVE and
>       Killing Floor 2 depended on that). ALthough, as my tests show, that was not universally
>       correct across all the options and the details of getsockopt() were also different.
>       I have another 16 patches on top of this series addressing the other options.
> 
>       There are also a few options (e. g., SO_KEEPALIVE) which should use the first byte only
>       (like that is currently done for TCP_NODELAY minus accessing optval before NULL check).
>       Also a few options behave like boolean values (return value of 1 after setting 0x100.
>       But since fixing up those are orthogonal to what these patches are doing (i. e., the
>       present patches do not change the behaviour for currently supported 4 byte optlen)
>       I am fixing up those in the yet further patches.
> 
>   dlls/ws2_32/tests/sock.c | 86 +++++++++++++++++++++++++++++++++++++++-
>   1 file changed, 85 insertions(+), 1 deletion(-)
> 
> diff --git a/dlls/ws2_32/tests/sock.c b/dlls/ws2_32/tests/sock.c
> index 6b6267dc153..0eece82bfb1 100644
> --- a/dlls/ws2_32/tests/sock.c
> +++ b/dlls/ws2_32/tests/sock.c
> @@ -1155,8 +1155,27 @@ static const LINGER linger_testvals[] = {
>   
>   static void test_set_getsockopt(void)
>   {
> +    static struct
> +    {
> +        int af;
> +        int type;
> +        int level;
> +        int optname;
> +        BOOL accepts_short_len;
> +        unsigned int sizes[3];
> +        DWORD values[3];
> +    }
> +    test_optsize[] =
> +    {
> +        {AF_INET, SOCK_STREAM, SOL_SOCKET, SO_RCVTIMEO, FALSE, {1, 2, 4}},
> +        {AF_INET, SOCK_STREAM, SOL_SOCKET, SO_SNDTIMEO, FALSE, {1, 2, 4}},
> +        {AF_INET, SOCK_DGRAM, IPPROTO_IP, IP_MULTICAST_LOOP, TRUE, {1, 1, 4}},
> +        {AF_INET, SOCK_DGRAM, IPPROTO_IP, IP_MULTICAST_TTL, TRUE, {1, 1, 4}},
> +        {AF_INET, SOCK_DGRAM, IPPROTO_IP, IP_TOS, TRUE, {1, 1, 4}},
> +        {AF_INET, SOCK_DGRAM, IPPROTO_IP, IP_TTL, TRUE, {1, 1, 4}},
> +    };
>       SOCKET s, s2;
> -    int i, err, lasterr;
> +    int i, j, err, lasterr;
>       int timeout;
>       LINGER lingval;
>       int size;
> @@ -1375,6 +1394,71 @@ static void test_set_getsockopt(void)
>           err, WSAGetLastError());
>   
>       closesocket(s);
> +
> +    /* Test option short length. */
> +    for (i = 0; i < ARRAY_SIZE(test_optsize); ++i)
> +    {
> +        s2 = socket( test_optsize[i].af, test_optsize[i].type, 0 );
> +        ok(s2 != INVALID_SOCKET, "socket() failed error %d\n", WSAGetLastError());
> +        for (j = 0; j < 3; ++j)

ARRAY_SIZE(test_optsize[i].sizes)?

The way this test is structured does not make it very easy to read, but 
I'm struggling to find a better option. The individual sockopts seem 
very idiosyncratic...

Finding a way to include TCP_NODELAY in this table, as well as finding a 
way to test lengths less than 1 and greater than 4, would probably be 
helpful. I don't know if you have those in your pending patches.

> +        {
> +            DWORD expected_last_error, expected_value, expected_size, save_value;
> +            int expected_err;
> +
> +            winetest_push_context("i %u, level %d, optname %d, len %u",
> +                    i, test_optsize[i].level, test_optsize[i].optname, 1 << j);
> +
> +            size = sizeof(save_value);
> +            err = getsockopt(s2, test_optsize[i].level, test_optsize[i].optname, (char*)&save_value, &size);
> +            ok(!err, "Unexpected getsockopt result %d.\n", err);
> +
> +            if (test_optsize[i].accepts_short_len || j == 2)

"j == 2" is not great, maybe "size = 1 << j;" earlier, and then "size == 
4" here, would be clearer.

> +            {
> +                expected_err = 0;
> +                expected_last_error = ERROR_SUCCESS;
> +                expected_size = test_optsize[i].sizes[j] ? test_optsize[i].sizes[j] : 4;
> +                if (test_optsize[i].values[j])
> +                    expected_value = test_optsize[i].values[j];
> +                else if (expected_size == 4)
> +                    expected_value = 1;
> +                else
> +                    expected_value = (0xdeadbeef & ~((1 << expected_size * 8) - 1)) | 1;

This is not exactly easy to read. Maybe something like the following 
would be clearer?

     value = 1;
     expected_value = 0xdeadbeef;
     memcpy(&expected_value, &value, expected_size);

> +            }
> +            else
> +            {
> +                expected_err = -1;
> +                expected_last_error = WSAEFAULT;
> +                expected_size = test_optsize[i].sizes[j];
> +                if (test_optsize[i].values[j])
> +                    expected_value = test_optsize[i].values[j];
> +                else
> +                    expected_value = 0xdeadbeef;
> +            }
> +
> +            value = 1;
> +            SetLastError(0xdeadbeef);
> +            err = setsockopt(s2, test_optsize[i].level, test_optsize[i].optname, (char*)&value, 1 << j);
> +            ok(err == expected_err, "Unexpected setsockopt result %d.\n", err);
> +            ok(WSAGetLastError() == expected_last_error, "Unexpected WSAGetLastError() %u.\n", WSAGetLastError());
> +
> +            value = 0xdeadbeef;
> +            SetLastError(0xdeadbeef);
> +            size = 1 << j;
> +            err = getsockopt(s2, test_optsize[i].level, test_optsize[i].optname, (char*)&value, &size);
> +            ok(err == expected_err, "Unexpected getsockopt result %d.\n", err);
> +            ok(WSAGetLastError() == expected_last_error, "Unexpected WSAGetLastError() %u.\n", WSAGetLastError());
> +            ok(value == expected_value, "Got unexpected value %#x, expected %#x.\n", value, expected_value);
> +            ok(size == expected_size, "Got unexpected size %u, expected %u.\n", size, expected_size);
> +
> +            err = setsockopt(s2, test_optsize[i].level, test_optsize[i].optname,
> +                    (char*)&save_value, sizeof(save_value));
> +            ok(!err, "Unexpected getsockopt result %d.\n", err);
> +
> +            winetest_pop_context();
> +        }
> +        closesocket(s2);
> +    }
> +
>       /* Test with the closed socket */
>       SetLastError(0xdeadbeef);
>       size = sizeof(i);