Re: [PATCH] ws2_32: Don't post completion packet if receiving fails with error immediately.

[Bruno Jesus]

On Mon, May 8, 2017 at 4:41 PM, Stefan Dösinger <stefan(a)codeweavers.com> wrote:
> This fixes bug 38980.
>
> This patch is the equivalent to 1bbe92e7 for receiving operations.
> Application in bug 38980 uses libtorrent, which in turn uses boost's
> async IO classes. Boost starts IO operations and waits for their results
> via IOCP. If the operation fails right away it enqueues a completion
> status itself. By failing and enquing a status we broke Boost's
> receiving logic - after the first status was read the internal data was
> reused elsewhere (though the pointer was not freed). The second IOCP
> result caused boost to call libtorrent's callback function with bad
> parameters, ultimately causing a segfault or assertion violation in
> libtorrent.

Awesome work, thanks ;-)
Some unused SetLastError calls and since I'm sending an email why not
complain more?
> Signed-off-by: Stefan Dösinger <stefan(a)codeweavers.com>
> ---
>  dlls/ws2_32/socket.c     |   2 -
>  dlls/ws2_32/tests/sock.c | 127 +++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 127 insertions(+), 2 deletions(-)
>
> diff --git a/dlls/ws2_32/socket.c b/dlls/ws2_32/socket.c
> index 9d27fab..0950754 100644
> --- a/dlls/ws2_32/socket.c
> +++ b/dlls/ws2_32/socket.c
> @@ -7744,9 +7744,7 @@ static int WS2_recv_base( SOCKET s, LPWSABUF lpBuffers, DWORD dwBufferCount,
>
> if (errno != EAGAIN)
>              {
> -                int loc_errno = errno;
>                  err = wsaErrno();
> -                if (cvalue) WS_AddCompletion( s, cvalue, sock_get_ntstatus(loc_errno), 0 );
>                  goto error;
>              }
>          }
> diff --git a/dlls/ws2_32/tests/sock.c b/dlls/ws2_32/tests/sock.c
> index 023eb42..7fefff5 100644
> --- a/dlls/ws2_32/tests/sock.c
> +++ b/dlls/ws2_32/tests/sock.c
> @@ -9299,6 +9299,133 @@ static void test_completion_port(void)
>      if (dest != INVALID_SOCKET)
>          closesocket(dest);
>
> +    /* Test IOCP response on successful immediate read. */
> +    tcp_socketpair(&src, &dest);
> +    if (src == INVALID_SOCKET || dest == INVALID_SOCKET)
> +    {
> +        skip("failed to create sockets\n");
> +        goto end;
> +    }
> +
> +    bufs.len = sizeof(buf);
> +    bufs.buf = buf;
> +    flags = 0;
> +
> +    iret = WSASend(src, &bufs, 1, &num_bytes, 0, &ov, NULL);
> +    ok(!iret, "WSASend failed - %d\n", iret);

Some winsock functions are basically boolean where 0 is true and
SOCKET_ERROR is false, so (I believe) in most other places we use
"WSASend failed with error %d", GetLastError()). This would require a
previous SetLastError.
> +    ok(num_bytes == sizeof(buf), "Managed to send %d\n", num_bytes);

I believe most code in ws2_32 prefer "Expected X, got Y\n".
> +
> +    io_port = CreateIoCompletionPort((HANDLE)dest, previous_port, 125, 0);
> +    ok(io_port != NULL, "failed to create completion port %u\n", GetLastError());
> +
> +    set_blocking(dest, FALSE);
> +
> +    Sleep(100);

Better to remove the extra new empty line as in the other use of Sleep
there is no empty line. Besides it makes it harder for AJ to see you
are adding sleep to tests :-)
> +
> +    num_bytes = 0xdeadbeef;
> +    SetLastError(0xdeadbeef);

SetLastError forever alone, no GetLastError. Maybe reuse the format from above?
> +    flags = 0;
> +
> +    iret = WSARecv(dest, &bufs, 1, &num_bytes, &flags, &ov, NULL);
> +    ok(!iret, "WSARecv failed - %d\n", iret);
> +    ok(num_bytes == sizeof(buf), "Managed to read %d\n", num_bytes);

Reuse expected/got message format?
> +    SetLastError(0xdeadbeef);
> +    key = 0xdeadbeef;
> +    num_bytes = 0xdeadbeef;
> +    olp = (WSAOVERLAPPED *)0xdeadbeef;
> +
> +    bret = GetQueuedCompletionStatus( io_port, &num_bytes, &key, &olp, 200 );
> +    ok(bret == TRUE, "failed to get completion status %u\n", bret);
> +    ok(GetLastError() == 0xdeadbeef, "Last error was %d\n", GetLastError());
> +    ok(key == 125, "Key is %lu\n", key);
> +    ok(num_bytes == sizeof(buf), "Number of bytes transferred is %u\n", num_bytes);
> +    ok(olp == &ov, "Overlapped structure is at %p\n", olp);
> +
> +    /* Test IOCP response on graceful shutdown. */
> +    closesocket(src);
> +    Sleep(100);
> +
> +    num_bytes = 0xdeadbeef;
> +    SetLastError(0xdeadbeef);

No GetLastError.
> +    flags = 0;
> +    memset(&ov, 0, sizeof(ov));
> +
> +    iret = WSARecv(dest, &bufs, 1, &num_bytes, &flags, &ov, NULL);
> +    ok(!iret, "WSARecv failed - %d\n", iret);
> +    ok(!num_bytes, "Managed to read %d\n", num_bytes);
> +
> +    SetLastError(0xdeadbeef);
> +    key = 0xdeadbeef;
> +    num_bytes = 0xdeadbeef;
> +    olp = (WSAOVERLAPPED *)0xdeadbeef;
> +
> +    bret = GetQueuedCompletionStatus( io_port, &num_bytes, &key, &olp, 200 );
> +    ok(bret == TRUE, "failed to get completion status %u\n", bret);
> +    ok(GetLastError() == 0xdeadbeef, "Last error was %d\n", GetLastError());
> +    ok(key == 125, "Key is %lu\n", key);
> +    ok(!num_bytes, "Number of bytes transferred is %u\n", num_bytes);
> +    ok(olp == &ov, "Overlapped structure is at %p\n", olp);
> +
> +    closesocket(src);
> +    src = INVALID_SOCKET;
> +    closesocket(dest);
> +    dest = INVALID_SOCKET;
> +
> +    /* Test IOCP response on hard shutdown. */
> +    tcp_socketpair(&src, &dest);
> +    if (src == INVALID_SOCKET || dest == INVALID_SOCKET)
> +    {
> +        skip("failed to create sockets\n");
> +        goto end;
> +    }
> +
> +    bufs.len = sizeof(buf);
> +    bufs.buf = buf;
> +    flags = 0;
> +    memset(&ov, 0, sizeof(ov));
> +
> +    ling.l_onoff = 1;
> +    ling.l_linger = 0;
> +    iret = setsockopt (src, SOL_SOCKET, SO_LINGER, (char *) &ling, sizeof(ling));
> +    ok(!iret, "Failed to set linger %d\n", GetLastError());
> +
> +    io_port = CreateIoCompletionPort((HANDLE)dest, previous_port, 125, 0);
> +    ok(io_port != NULL, "failed to create completion port %u\n", GetLastError());
> +
> +    set_blocking(dest, FALSE);
> +
> +    closesocket(src);
> +    src = INVALID_SOCKET;
> +
> +    Sleep(100);
> +
> +    num_bytes = 0xdeadbeef;
> +    SetLastError(0xdeadbeef);
> +
> +    /* Somehow a hard shutdown doesn't work on my Linux box. It seems SO_LINGER is ignored. */

AFAIR this is not your fault, it is a bug. WSARecv has a particular
behavior of not returning a "0 bytes read", instead it returns error
with WSAECONNRESET. Wine simply returns success with 0 bytes, which
stands for connection reset in the old recv() way. This explains the 3
todo_wine below.
> +    iret = WSARecv(dest, &bufs, 1, &num_bytes, &flags, &ov, NULL);
> +    todo_wine ok(iret == SOCKET_ERROR, "WSARecv failed - %d\n", iret);
> +    todo_wine ok(GetLastError() == WSAECONNRESET, "Last error was %d\n", GetLastError());
> +    todo_wine ok(num_bytes == 0xdeadbeef, "Managed to send %d\n", num_bytes);

Expected/got format? If not the sentence is wrong: send/recv.
> +
> +    SetLastError(0xdeadbeef);
> +    key = 0xdeadbeef;
> +    num_bytes = 0xdeadbeef;
> +    olp = (WSAOVERLAPPED *)0xdeadbeef;
> +
> +    bret = GetQueuedCompletionStatus( io_port, &num_bytes, &key, &olp, 200 );
> +    todo_wine ok(bret == FALSE, "GetQueuedCompletionStatus returned %u\n", bret );
> +    todo_wine ok(GetLastError() == WAIT_TIMEOUT, "Last error was %d\n", GetLastError());
> +    todo_wine ok(key == 0xdeadbeef, "Key is %lu\n", key);
> +    todo_wine ok(num_bytes == 0xdeadbeef, "Number of bytes transferred is %u\n", num_bytes);
> +    todo_wine ok(!olp, "Overlapped structure is at %p\n", olp);
> +
> +    num_bytes = 0xdeadbeef;
> +    SetLastError(0xdeadbeef);
> +
> +    closesocket(dest);
> +
>      dest = socket(AF_INET, SOCK_STREAM, 0);
>      if (dest == INVALID_SOCKET)
>      {
> --
> 2.10.2

[PATCH] ws2_32: Don't post completion packet if receiving fails with error immediately.

[Stefan Dösinger]

> Am 09.05.2017 um 01:43 schrieb Bruno Jesus <00cpxxx(a)gmail.com>:
>
> Awesome work, thanks ;-)
> Some unused SetLastError calls and since I'm sending an email why not
> complain more?
> I largely followed the style of the surrounding code. It is very defensive with seemingly redundant calls, yes.

I spent 2 hours trying to figure out why Windows gave me a weird 1024 byte read on the second read (the graceful close one, which should return success with 0 bytes), only to figure out that it happened because I didn’t memset the overlapped structure back to zero. At that point I decided to keep the defensive style of the rest of the test.

I can certainly take another shot at removing things, just don’t blame me that it might affect the behavior of the existing tests in this function :-)
> Some winsock functions are basically boolean where 0 is true and
> SOCKET_ERROR is false, so (I believe) in most other places we use
> "WSASend failed with error %d", GetLastError()). This would require a
> previous SetLastError.
> Following existing style here.
> > +    ok(num_bytes == sizeof(buf), "Managed to send %d\n", num_bytes);
>
> I believe most code in ws2_32 prefer "Expected X, got Y\n“.
> Following existing style, and in d3d we generally avoid ok(result == ABC "expected ABC, but hell froze over"); lines because it specifies the expected results twice and is begging for inconsistency. To understand what is going wrong you need to look at the test code anyway.
> Better to remove the extra new empty line as in the other use of Sleep
> there is no empty line. Besides it makes it harder for AJ to see you
> are adding sleep to tests :-)
> I haven’t checked if the newlines follow existing style, I’ll double check. The existing code already has the sleeps. I am not sure if they are necessary, but if removing them introduces random failures we may or may not catch it immediately.
> AFAIR this is not your fault, it is a bug. WSARecv has a particular
> behavior of not returning a "0 bytes read", instead it returns error
> with WSAECONNRESET. Wine simply returns success with 0 bytes, which
> stands for connection reset in the old recv() way. This explains the 3
> todo_wine below.
> No, Windows does return 0 bytes on a graceful close (see the 2nd of my 3 tests). On Wine we do get WSAECONNRESET in some situations. These WSAECONNRESET returns are what triggers the crash in libtorrent. On Windows I can trigger WSAECONNRESET on the receiving end with the SO_LINGER option + closesocket on the sending end. That doesn’t work on my Linux box, although various internet pages suggest it should work.

As far as I understand WSAECONNRESET (or EPIPE in POSIX) 00is returned when the remote end rebooted, had power cut or got physically disconnected - which are valid situations, but a bit difficult to trigger from a test. But because libtorrent connects to loads of computers across the internet, having one peer go out of wifi range or run out of battery isn’t unexpected.
> > +    iret = WSARecv(dest, &bufs, 1, &num_bytes, &flags, &ov, NULL);
> > +    todo_wine ok(iret == SOCKET_ERROR, "WSARecv failed - %d\n", iret);
> > +    todo_wine ok(GetLastError() == WSAECONNRESET, "Last error was %d\n", GetLastError());
> > +    todo_wine ok(num_bytes == 0xdeadbeef, "Managed to send %d\n", num_bytes);
>
> Expected/got format? If not the sentence is wrong: send/recv.
> Whoops on the send/recv :-) .

[PATCH] ws2_32: Don't post completion packet if receiving fails with error immediately.

[Jacek Caban]

On 09.05.2017 12:06, Stefan Dösinger wrote:
> The existing code already has the sleeps. I am not sure if they are necessary, but if removing them introduces random failures we may or may not catch it immediately.

In this case you may use select() to make sure that data is available
before calling WSARecv.


Jacek