Skip to content

Commit 886cf10

Browse files
committed
Ensure guarantees about safe closing of poll watchers
Consider the following scenario: uv_poll_init(loop, poll, fd); uv_poll_start(poll, UV_READABLE, cb); // the cb gets invoked etc. uv_poll_stop(poll); close(fd); fd = allocate_new_socket(); // allocate_new_socket() is assigned the same fd by "bad luck" from the OS // some time later: uv_poll_init(loop, otherpoll, fd); uv_poll_start(otherpoll, UV_READABLE, cb); uv_close(poll); // uv__io_stop: Assertion `loop->watchers[w->fd] == w' failed. According to documentation, "however the fd can be safely closed immediately after a call to uv_poll_stop() or uv_close()." Though, in this scenario, we close()'d our file descriptor, and by bad luck we got the same file descriptor again and register a new handle for it and start polling. Previously that would lead to an assertion failure, if we were to properly free the original handle via uv_close(). This commit fixes that by moving the check whether a only a single poll handle is active to uv_poll_start() instead of the stopping routines.
1 parent 1bcfbfd commit 886cf10

File tree

7 files changed

+113
-3
lines changed

7 files changed

+113
-3
lines changed

CMakeLists.txt

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -363,6 +363,7 @@ if(LIBUV_BUILD_TESTS)
363363
test/test-poll-close-doesnt-corrupt-stack.c
364364
test/test-poll-close.c
365365
test/test-poll-closesocket.c
366+
test/test-poll-multiple-handles.c
366367
test/test-poll-oob.c
367368
test/test-poll.c
368369
test/test-process-priority.c

Makefile.am

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -232,6 +232,7 @@ test_run_tests_SOURCES = test/blackhole-server.c \
232232
test/test-poll-close.c \
233233
test/test-poll-close-doesnt-corrupt-stack.c \
234234
test/test-poll-closesocket.c \
235+
test/test-poll-multiple-handles.c \
235236
test/test-poll-oob.c \
236237
test/test-process-priority.c \
237238
test/test-process-title.c \

src/unix/core.c

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -912,13 +912,12 @@ void uv__io_stop(uv_loop_t* loop, uv__io_t* w, unsigned int events) {
912912
if (w->pevents == 0) {
913913
QUEUE_REMOVE(&w->watcher_queue);
914914
QUEUE_INIT(&w->watcher_queue);
915+
w->events = 0;
915916

916-
if (loop->watchers[w->fd] != NULL) {
917-
assert(loop->watchers[w->fd] == w);
917+
if (w == loop->watchers[w->fd]) {
918918
assert(loop->nfds > 0);
919919
loop->watchers[w->fd] = NULL;
920920
loop->nfds--;
921-
w->events = 0;
922921
}
923922
}
924923
else if (QUEUE_EMPTY(&w->watcher_queue))

src/unix/poll.c

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -116,12 +116,21 @@ int uv_poll_stop(uv_poll_t* handle) {
116116

117117

118118
int uv_poll_start(uv_poll_t* handle, int pevents, uv_poll_cb poll_cb) {
119+
uv__io_t** watchers;
120+
uv__io_t* w;
119121
int events;
120122

121123
assert((pevents & ~(UV_READABLE | UV_WRITABLE | UV_DISCONNECT |
122124
UV_PRIORITIZED)) == 0);
123125
assert(!uv__is_closing(handle));
124126

127+
watchers = handle->loop->watchers;
128+
w = &handle->io_watcher;
129+
130+
if (uv__fd_exists(handle->loop, w->fd))
131+
if (watchers[w->fd] != w)
132+
return UV_EEXIST;
133+
125134
uv__poll_stop(handle);
126135

127136
if (pevents == 0)

test/test-list.h

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -442,6 +442,7 @@ TEST_DECLARE (poll_nested_epoll)
442442
#ifdef UV_HAVE_KQUEUE
443443
TEST_DECLARE (poll_nested_kqueue)
444444
#endif
445+
TEST_DECLARE (poll_multiple_handles)
445446

446447
TEST_DECLARE (ip4_addr)
447448
TEST_DECLARE (ip6_addr_link_local)
@@ -874,6 +875,7 @@ TASK_LIST_START
874875
#ifdef UV_HAVE_KQUEUE
875876
TEST_ENTRY (poll_nested_kqueue)
876877
#endif
878+
TEST_ENTRY (poll_multiple_handles)
877879

878880
TEST_ENTRY (socket_buffer_size)
879881

test/test-poll-multiple-handles.c

Lines changed: 97 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,97 @@
1+
/* Copyright libuv project contributors. All rights reserved.
2+
*
3+
* Permission is hereby granted, free of charge, to any person obtaining a copy
4+
* of this software and associated documentation files (the "Software"), to
5+
* deal in the Software without restriction, including without limitation the
6+
* rights to use, copy, modify, merge, publish, distribute, sublicense, and/or
7+
* sell copies of the Software, and to permit persons to whom the Software is
8+
* furnished to do so, subject to the following conditions:
9+
*
10+
* The above copyright notice and this permission notice shall be included in
11+
* all copies or substantial portions of the Software.
12+
*
13+
* THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
14+
* IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
15+
* FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
16+
* AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
17+
* LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
18+
* FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
19+
* IN THE SOFTWARE.
20+
*/
21+
22+
#include <errno.h>
23+
24+
#ifndef _WIN32
25+
# include <fcntl.h>
26+
# include <sys/socket.h>
27+
# include <unistd.h>
28+
#endif
29+
30+
#include "uv.h"
31+
#include "task.h"
32+
33+
34+
static int close_cb_called = 0;
35+
36+
37+
static void close_cb(uv_handle_t* handle) {
38+
close_cb_called++;
39+
}
40+
41+
static void poll_cb(uv_poll_t* handle, int status, int events) {
42+
/* Not a bound socket, linux immediately reports UV_READABLE, other OS do not */
43+
ASSERT(events == UV_READABLE);
44+
}
45+
46+
TEST_IMPL(poll_multiple_handles) {
47+
uv_os_sock_t sock;
48+
uv_poll_t first_poll_handle, second_poll_handle;
49+
50+
#ifdef _WIN32
51+
{
52+
struct WSAData wsa_data;
53+
int r = WSAStartup(MAKEWORD(2, 2), &wsa_data);
54+
ASSERT(r == 0);
55+
}
56+
#endif
57+
58+
sock = socket(AF_INET, SOCK_STREAM, 0);
59+
#ifdef _WIN32
60+
ASSERT(sock != INVALID_SOCKET);
61+
#else
62+
ASSERT(sock != -1);
63+
#endif
64+
ASSERT(0 == uv_poll_init_socket(uv_default_loop(), &first_poll_handle, sock));
65+
ASSERT(0 == uv_poll_init_socket(uv_default_loop(), &second_poll_handle, sock));
66+
67+
ASSERT(0 == uv_poll_start(&first_poll_handle, UV_READABLE, poll_cb));
68+
69+
/* We may not start polling while another polling handle is active
70+
* on that fd.
71+
*/
72+
#ifndef _WIN32
73+
/* We do not track handles in an O(1) lookupable way on Windows,
74+
* so not checking that here.
75+
*/
76+
ASSERT(uv_poll_start(&second_poll_handle, UV_READABLE, poll_cb) == UV_EEXIST);
77+
#endif
78+
79+
/* After stopping the other polling handle, we now should be able to poll */
80+
ASSERT(0 == uv_poll_stop(&first_poll_handle));
81+
ASSERT(0 == uv_poll_start(&second_poll_handle, UV_READABLE, poll_cb));
82+
83+
/* Closing an already stopped polling handle is safe in any case */
84+
uv_close((uv_handle_t*) &first_poll_handle, close_cb);
85+
86+
ASSERT(1 == uv_run(uv_default_loop(), UV_RUN_ONCE));
87+
ASSERT(close_cb_called == 1);
88+
89+
ASSERT(uv_is_active((uv_handle_t*) &second_poll_handle));
90+
uv_close((uv_handle_t*) &second_poll_handle, close_cb);
91+
92+
ASSERT(0 == uv_run(uv_default_loop(), UV_RUN_DEFAULT));
93+
ASSERT(close_cb_called == 2);
94+
95+
MAKE_VALGRIND_HAPPY();
96+
return 0;
97+
}

test/test.gyp

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -86,6 +86,7 @@
8686
'test-poll-close.c',
8787
'test-poll-close-doesnt-corrupt-stack.c',
8888
'test-poll-closesocket.c',
89+
'test-poll-multiple-handles.c',
8990
'test-poll-oob.c',
9091
'test-process-priority.c',
9192
'test-process-title.c',

0 commit comments

Comments
 (0)