From 5f2016a425664831488a0169fece030635031b5e Mon Sep 17 00:00:00 2001 From: Bert Belder Date: Tue, 9 Dec 2014 22:03:05 +0100 Subject: [PATCH 1/4] test: test that closing a poll handle doesn't corrupt the stack MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This is a regression test for an issue that was originally reported in https://github.com/libuv/libuv/pull/36, and fixed in cd89452. PR-URL: https://github.com/libuv/libuv/pull/48 Reviewed-By: Saúl Ibarra Corretgé --- test/test-list.h | 2 + test/test-poll-close-doesnt-corrupt-stack.c | 113 ++++++++++++++++++++ uv.gyp | 1 + 3 files changed, 116 insertions(+) create mode 100644 test/test-poll-close-doesnt-corrupt-stack.c diff --git a/test/test-list.h b/test/test-list.h index 47444258..fb4b438f 100644 --- a/test/test-list.h +++ b/test/test-list.h @@ -215,6 +215,7 @@ TEST_DECLARE (poll_duplex) TEST_DECLARE (poll_unidirectional) TEST_DECLARE (poll_close) #ifdef _WIN32 +TEST_DECLARE (poll_close_doesnt_corrupt_stack) TEST_DECLARE (poll_closesocket) TEST_DECLARE (spawn_detect_pipe_name_collisions_on_windows) TEST_DECLARE (argument_escaping) @@ -451,6 +452,7 @@ TASK_LIST_START TEST_ENTRY (kill) #ifdef _WIN32 + TEST_ENTRY (poll_close_doesnt_corrupt_stack) TEST_ENTRY (poll_closesocket) TEST_ENTRY (spawn_detect_pipe_name_collisions_on_windows) TEST_ENTRY (argument_escaping) diff --git a/test/test-poll-close-doesnt-corrupt-stack.c b/test/test-poll-close-doesnt-corrupt-stack.c new file mode 100644 index 00000000..5c244b46 --- /dev/null +++ b/test/test-poll-close-doesnt-corrupt-stack.c @@ -0,0 +1,113 @@ +/* Copyright Bert Belder, and other libuv contributors. All rights reserved. + * + * Permission is hereby granted, free of charge, to any person obtaining a copy + * of this software and associated documentation files (the "Software"), to + * deal in the Software without restriction, including without limitation the + * rights to use, copy, modify, merge, publish, distribute, sublicense, and/or + * sell copies of the Software, and to permit persons to whom the Software is + * furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in + * all copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS + * IN THE SOFTWARE. + */ + +#ifdef _WIN32 + +#include +#include + +#include "uv.h" +#include "task.h" + +#ifdef _MSC_VER /* msvc */ +# define NO_INLINE __declspec(noinline) +#else /* gcc */ +# define NO_INLINE __attribute__ ((noinline)) +#endif + + +uv_os_sock_t sock; +uv_poll_t handle; + +static int close_cb_called = 0; + + +static void close_cb(uv_handle_t* h) { + close_cb_called++; +} + + +static void poll_cb(uv_poll_t* h, int status, int events) { + ASSERT(0 && "should never get here"); +} + + +static void NO_INLINE close_socket_and_verify_stack() { + const uint32_t MARKER = 0xDEADBEEF; + const int VERIFY_AFTER = 10; /* ms */ + + volatile uint32_t data[65536]; + size_t i; + + for (i = 0; i < ARRAY_SIZE(data); i++) + data[i] = MARKER; + + int r = closesocket(sock); + ASSERT(r == 0); + + uv_sleep(VERIFY_AFTER); + + for (i = 0; i < ARRAY_SIZE(data); i++) + ASSERT(data[i] == MARKER); +} + + +TEST_IMPL(poll_close_doesnt_corrupt_stack) { + struct WSAData wsa_data; + int r; + unsigned long on; + struct sockaddr_in addr; + + r = WSAStartup(MAKEWORD(2, 2), &wsa_data); + ASSERT(r == 0); + + sock = socket(AF_INET, SOCK_STREAM, 0); + ASSERT(sock != INVALID_SOCKET); + on = 1; + r = ioctlsocket(sock, FIONBIO, &on); + ASSERT(r == 0); + + addr = uv_ip4_addr("127.0.0.1", TEST_PORT); + ASSERT(r == 0); + + r = connect(sock, (const struct sockaddr*) &addr, sizeof addr); + ASSERT(r != 0); + ASSERT(WSAGetLastError() == WSAEWOULDBLOCK); + + r = uv_poll_init_socket(uv_default_loop(), &handle, sock); + ASSERT(r == 0); + r = uv_poll_start(&handle, UV_READABLE | UV_WRITABLE, poll_cb); + ASSERT(r == 0); + + uv_close((uv_handle_t*) &handle, close_cb); + + close_socket_and_verify_stack(); + + r = uv_run(uv_default_loop(), UV_RUN_DEFAULT); + ASSERT(r == 0); + + ASSERT(close_cb_called == 1); + + MAKE_VALGRIND_HAPPY(); + return 0; +} + +#endif /* _WIN32 */ diff --git a/uv.gyp b/uv.gyp index 5af99bd5..d64aa25f 100644 --- a/uv.gyp +++ b/uv.gyp @@ -323,6 +323,7 @@ 'test/test-platform-output.c', 'test/test-poll.c', 'test/test-poll-close.c', + 'test/test-poll-close-doesnt-corrupt-stack.c', 'test/test-poll-closesocket.c', 'test/test-process-title.c', 'test/test-ref.c', From 2e30a19641189fd345766978a796adb0e530623b Mon Sep 17 00:00:00 2001 From: Marc Schlaich Date: Wed, 10 Dec 2014 08:48:52 +0100 Subject: [PATCH 2/4] win: fix compilation of tests PR-URL: https://github.com/libuv/libuv/pull/51 Reviewed-By: Ben Noordhuis --- test/test-poll-close-doesnt-corrupt-stack.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/test/test-poll-close-doesnt-corrupt-stack.c b/test/test-poll-close-doesnt-corrupt-stack.c index 5c244b46..eb10507a 100644 --- a/test/test-poll-close-doesnt-corrupt-stack.c +++ b/test/test-poll-close-doesnt-corrupt-stack.c @@ -53,6 +53,7 @@ static void poll_cb(uv_poll_t* h, int status, int events) { static void NO_INLINE close_socket_and_verify_stack() { const uint32_t MARKER = 0xDEADBEEF; const int VERIFY_AFTER = 10; /* ms */ + int r; volatile uint32_t data[65536]; size_t i; @@ -60,7 +61,7 @@ static void NO_INLINE close_socket_and_verify_stack() { for (i = 0; i < ARRAY_SIZE(data); i++) data[i] = MARKER; - int r = closesocket(sock); + r = closesocket(sock); ASSERT(r == 0); uv_sleep(VERIFY_AFTER); From 152c35d54dfe9a9a7bf0e63dd00b606b0dfe61ac Mon Sep 17 00:00:00 2001 From: Bert Belder Date: Wed, 10 Dec 2014 16:33:38 +0100 Subject: [PATCH 3/4] Revert "win: keep a reference to AFD_POLL_INFO in cancel poll" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The offending patch doesn't completely fix the issue, it just trades stack corruption for heap corruption which is less likely. In addition there is a much simpler solution for this problem. This reverts commit cd894521dd9e1e648fde803950f2dcb3a8529a3b. PR-URL: https://github.com/libuv/libuv/pull/49 Reviewed-By: Ben Noordhuis Reviewed-By: Marc Schlaich Reviewed-By: Saúl Ibarra Corretgé --- include/uv-private/uv-win.h | 5 +---- src/win/poll.c | 31 ++++++++++--------------------- 2 files changed, 11 insertions(+), 25 deletions(-) diff --git a/include/uv-private/uv-win.h b/include/uv-private/uv-win.h index e5f2f3b4..c9ec38ef 100644 --- a/include/uv-private/uv-win.h +++ b/include/uv-private/uv-win.h @@ -474,10 +474,7 @@ RB_HEAD(uv_timer_tree_s, uv_timer_s); /* Used in fast mode */ \ SOCKET peer_socket; \ AFD_POLL_INFO afd_poll_info_1; \ - union { \ - AFD_POLL_INFO* afd_poll_info_ptr; \ - AFD_POLL_INFO afd_poll_info; \ - } afd_poll_info_2; \ + AFD_POLL_INFO afd_poll_info_2; \ /* Used in fast and slow mode. */ \ uv_req_t poll_req_1; \ uv_req_t poll_req_2; \ diff --git a/src/win/poll.c b/src/win/poll.c index 5c5c7d80..76427b01 100644 --- a/src/win/poll.c +++ b/src/win/poll.c @@ -79,7 +79,7 @@ static void uv__fast_poll_submit_poll_req(uv_loop_t* loop, uv_poll_t* handle) { handle->mask_events_2 = handle->events; } else if (handle->submitted_events_2 == 0) { req = &handle->poll_req_2; - afd_poll_info = &handle->afd_poll_info_2.afd_poll_info_ptr[0]; + afd_poll_info = &handle->afd_poll_info_2; handle->submitted_events_2 = handle->events; handle->mask_events_1 = handle->events; handle->mask_events_2 = 0; @@ -118,19 +118,18 @@ static void uv__fast_poll_submit_poll_req(uv_loop_t* loop, uv_poll_t* handle) { static int uv__fast_poll_cancel_poll_req(uv_loop_t* loop, uv_poll_t* handle) { - AFD_POLL_INFO* afd_poll_info; + AFD_POLL_INFO afd_poll_info; DWORD result; - afd_poll_info = &handle->afd_poll_info_2.afd_poll_info_ptr[1]; - afd_poll_info->Exclusive = TRUE; - afd_poll_info->NumberOfHandles = 1; - afd_poll_info->Timeout.QuadPart = INT64_MAX; - afd_poll_info->Handles[0].Handle = (HANDLE) handle->socket; - afd_poll_info->Handles[0].Status = 0; - afd_poll_info->Handles[0].Events = AFD_POLL_ALL; + afd_poll_info.Exclusive = TRUE; + afd_poll_info.NumberOfHandles = 1; + afd_poll_info.Timeout.QuadPart = INT64_MAX; + afd_poll_info.Handles[0].Handle = (HANDLE) handle->socket; + afd_poll_info.Handles[0].Status = 0; + afd_poll_info.Handles[0].Events = AFD_POLL_ALL; result = uv_msafd_poll(handle->socket, - afd_poll_info, + &afd_poll_info, uv__get_overlapped_dummy()); if (result == SOCKET_ERROR) { @@ -155,7 +154,7 @@ static void uv__fast_poll_process_poll_req(uv_loop_t* loop, uv_poll_t* handle, handle->submitted_events_1 = 0; mask_events = handle->mask_events_1; } else if (req == &handle->poll_req_2) { - afd_poll_info = &handle->afd_poll_info_2.afd_poll_info_ptr[0]; + afd_poll_info = &handle->afd_poll_info_2; handle->submitted_events_2 = 0; mask_events = handle->mask_events_2; } else { @@ -552,12 +551,6 @@ int uv_poll_init_socket(uv_loop_t* loop, uv_poll_t* handle, handle->poll_req_2.type = UV_POLL_REQ; handle->poll_req_2.data = handle; - handle->afd_poll_info_2.afd_poll_info_ptr = malloc(sizeof(*handle->afd_poll_info_2.afd_poll_info_ptr) * 2); - if (handle->afd_poll_info_2.afd_poll_info_ptr == NULL) { - uv__set_artificial_error(loop, UV_ENOMEM); - return -1; - } - return 0; } @@ -611,9 +604,5 @@ void uv_poll_endgame(uv_loop_t* loop, uv_poll_t* handle) { assert(handle->submitted_events_1 == 0); assert(handle->submitted_events_2 == 0); - if (handle->afd_poll_info_2.afd_poll_info_ptr) { - free(handle->afd_poll_info_2.afd_poll_info_ptr); - handle->afd_poll_info_2.afd_poll_info_ptr = NULL; - } uv__handle_close(handle); } From 48d39345bc0f19e6ba39f40950018e1edd0cccb2 Mon Sep 17 00:00:00 2001 From: Bert Belder Date: Tue, 9 Dec 2014 22:53:05 +0100 Subject: [PATCH 4/4] win: avoid stack corruption when closing a poll handle MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When the user closes an uv_poll_t handle, there may still be an outstanding AFD_POLL request. Libuv can't cancel this AFD_POLL request directly, so it has to submit another which makes the the previous poll request return. Libuv completely forgets about the "canceling" AFD_POLL request, but at some point in time it may complete and store its result somewhere in memory. Without this patch the result of the canceling poll request is written somewhere on the stack, which sometimes causes stack corruption. This patch ensures that the result is written to some static scratch space where it can do no harm. PR-URL: https://github.com/libuv/libuv/pull/49 Reviewed-By: Ben Noordhuis Reviewed-By: Marc Schlaich Reviewed-By: Saúl Ibarra Corretgé --- src/win/internal.h | 4 ++-- src/win/poll.c | 9 +++++++++ src/win/winsock.c | 12 ++++++------ 3 files changed, 17 insertions(+), 8 deletions(-) diff --git a/src/win/internal.h b/src/win/internal.h index 9920f704..f32d24a7 100644 --- a/src/win/internal.h +++ b/src/win/internal.h @@ -332,8 +332,8 @@ int WSAAPI uv_wsarecvfrom_workaround(SOCKET socket, WSABUF* buffers, int* addr_len, WSAOVERLAPPED *overlapped, LPWSAOVERLAPPED_COMPLETION_ROUTINE completion_routine); -int WSAAPI uv_msafd_poll(SOCKET socket, AFD_POLL_INFO* info, - OVERLAPPED* overlapped); +int WSAAPI uv_msafd_poll(SOCKET socket, AFD_POLL_INFO* info_in, + AFD_POLL_INFO* info_out, OVERLAPPED* overlapped); /* Whether there are any non-IFS LSPs stacked on TCP */ extern int uv_tcp_non_ifs_lsp_ipv4; diff --git a/src/win/poll.c b/src/win/poll.c index 76427b01..37242792 100644 --- a/src/win/poll.c +++ b/src/win/poll.c @@ -46,6 +46,8 @@ typedef struct uv_single_fd_set_s { static OVERLAPPED overlapped_dummy_; static uv_once_t overlapped_dummy_init_guard_ = UV_ONCE_INIT; +static AFD_POLL_INFO afd_poll_info_dummy_; + static void uv__init_overlapped_dummy(void) { HANDLE event; @@ -65,6 +67,11 @@ static OVERLAPPED* uv__get_overlapped_dummy() { } +static AFD_POLL_INFO* uv__get_afd_poll_info_dummy() { + return &afd_poll_info_dummy_; +} + + static void uv__fast_poll_submit_poll_req(uv_loop_t* loop, uv_poll_t* handle) { uv_req_t* req; AFD_POLL_INFO* afd_poll_info; @@ -107,6 +114,7 @@ static void uv__fast_poll_submit_poll_req(uv_loop_t* loop, uv_poll_t* handle) { memset(&req->overlapped, 0, sizeof req->overlapped); result = uv_msafd_poll((SOCKET) handle->peer_socket, + afd_poll_info, afd_poll_info, &req->overlapped); if (result != 0 && WSAGetLastError() != WSA_IO_PENDING) { @@ -130,6 +138,7 @@ static int uv__fast_poll_cancel_poll_req(uv_loop_t* loop, uv_poll_t* handle) { result = uv_msafd_poll(handle->socket, &afd_poll_info, + uv__get_afd_poll_info_dummy(), uv__get_overlapped_dummy()); if (result == SOCKET_ERROR) { diff --git a/src/win/winsock.c b/src/win/winsock.c index cf6d0318..b91a2373 100644 --- a/src/win/winsock.c +++ b/src/win/winsock.c @@ -467,8 +467,8 @@ int WSAAPI uv_wsarecvfrom_workaround(SOCKET socket, WSABUF* buffers, } -int WSAAPI uv_msafd_poll(SOCKET socket, AFD_POLL_INFO* info, - OVERLAPPED* overlapped) { +int WSAAPI uv_msafd_poll(SOCKET socket, AFD_POLL_INFO* info_in, + AFD_POLL_INFO* info_out, OVERLAPPED* overlapped) { IO_STATUS_BLOCK iosb; IO_STATUS_BLOCK* iosb_ptr; HANDLE event = NULL; @@ -506,10 +506,10 @@ int WSAAPI uv_msafd_poll(SOCKET socket, AFD_POLL_INFO* info, apc_context, iosb_ptr, IOCTL_AFD_POLL, - info, - sizeof *info, - info, - sizeof *info); + info_in, + sizeof *info_in, + info_out, + sizeof *info_out); if (overlapped == NULL) { /* If this is a blocking operation, wait for the event to become */