Skip to content
This repository has been archived by the owner on May 4, 2018. It is now read-only.

unix: fix reopened fd bug #971

Closed
wants to merge 6 commits into from
Closed

unix: fix reopened fd bug #971

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
59d3350
unix: fix reopened fd bug
indutny Oct 25, 2013
00ace90
test
indutny Nov 2, 2013
ee3810c
fix test
indutny Nov 2, 2013
bb3d280
description of test
indutny Nov 2, 2013
89e241d
fixes
indutny Nov 3, 2013
6b9a7bb
fixes
indutny Nov 3, 2013
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions build.mk
View file
Open in desktop
Expand Up @@ -114,6 +114,7 @@ TESTS= \
test/test-tcp-bind6-error.o \
test/test-tcp-bind-error.o \
test/test-tcp-close.o \
test/test-tcp-close-accept.o \
test/test-tcp-close-while-connecting.o \
test/test-tcp-connect6-error.o \
test/test-tcp-connect-error-after-write.o \
Expand Down
1 change: 1 addition & 0 deletions checksparse.sh
View file
Open in desktop
Expand Up @@ -133,6 +133,7 @@ test/test-stdio-over-pipes.c
test/test-tcp-bind-error.c
test/test-tcp-bind6-error.c
test/test-tcp-close-while-connecting.c
test/test-tcp-close-accept.c
test/test-tcp-close.c
test/test-tcp-connect-error-after-write.c
test/test-tcp-connect-error.c
Expand Down
20 changes: 18 additions & 2 deletions src/unix/core.c
View file
Open in desktop
Expand Up @@ -595,20 +595,33 @@ static unsigned int next_power_of_two(unsigned int val) {

static void maybe_resize(uv_loop_t* loop, unsigned int len) {
uv__io_t** watchers;
void* fake_watcher_list;
void* fake_watcher_count;
unsigned int nwatchers;
unsigned int i;

if (len <= loop->nwatchers)
return;

nwatchers = next_power_of_two(len);
watchers = realloc(loop->watchers, nwatchers * sizeof(loop->watchers[0]));
nwatchers = next_power_of_two(len + 2) - 2;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This logic is unsound. Say that len == 14. In that case, next_power_of_two(14 + 2) - 2 is still 14.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, and that's ok. Since I'm allocating nwatchers + 2 below.

watchers = realloc(loop->watchers,
(nwatchers + 2) * sizeof(loop->watchers[0]));

if (watchers == NULL)
abort();

/* Copy watchers, preserving fake one in the end */
if (loop->watchers == NULL) {
fake_watcher_list = watchers[loop->nwatchers];
fake_watcher_count = watchers[loop->nwatchers + 1];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using loop->watchers here would be a little clearer.

} else {
fake_watcher_list = NULL;
fake_watcher_count = NULL;
}
for (i = loop->nwatchers; i < nwatchers; i++)
watchers[i] = NULL;
watchers[nwatchers] = fake_watcher_list;
watchers[nwatchers + 1] = fake_watcher_count;

loop->watchers = watchers;
loop->nwatchers = nwatchers;
Expand Down Expand Up @@ -700,6 +713,9 @@ void uv__io_stop(uv_loop_t* loop, uv__io_t* w, unsigned int events) {
void uv__io_close(uv_loop_t* loop, uv__io_t* w) {
uv__io_stop(loop, w, UV__POLLIN | UV__POLLOUT);
ngx_queue_remove(&w->pending_queue);

/* Remove stale events for this file descriptor */
uv__platform_invalidate_fd(loop, w->fd);
}


Expand Down
19 changes: 19 additions & 0 deletions src/unix/darwin.c
View file
Open in desktop
Expand Up @@ -102,6 +102,25 @@ void uv__platform_loop_delete(uv_loop_t* loop) {
}


void uv__platform_invalidate_fd(uv_loop_t* loop, int fd) {
struct kevent* events;
uintptr_t i;
uintptr_t nfds;

assert(loop->watchers != NULL);

events = (struct kevent*) loop->watchers[loop->nwatchers];
nfds = (uintptr_t) loop->watchers[loop->nwatchers + 1];
if (events == NULL)
return;

/* Invalidate events with same file descriptor */
for (i = 0; i < nfds; i++)
if ((int) events[i].ident == fd)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd cast fd to uintptr_t here. Casts from unsigned to signed types of equal or smaller width are implementation defined.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

erm... we are doing the same thing in kqueue.c.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

erm... we are doing the same thing in kqueue.c.

That's something we should fix then.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In another PR, right?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes.

events[i].ident = -1;
}


static void uv__cf_loop_runner(void* arg) {
uv_loop_t* loop;

Expand Down
1 change: 1 addition & 0 deletions src/unix/internal.h
View file
Open in desktop
Expand Up @@ -183,6 +183,7 @@ uint64_t uv__hrtime(void);
int uv__kqueue_init(uv_loop_t* loop);
int uv__platform_loop_init(uv_loop_t* loop, int default_loop);
void uv__platform_loop_delete(uv_loop_t* loop);
void uv__platform_invalidate_fd(uv_loop_t* loop, int fd);

/* various */
void uv__async_close(uv_async_t* handle);
Expand Down
9 changes: 9 additions & 0 deletions src/unix/kqueue.c
View file
Open in desktop
Expand Up @@ -162,11 +162,18 @@ void uv__io_poll(uv_loop_t* loop, int timeout) {

nevents = 0;

assert(loop->watchers != NULL);
loop->watchers[loop->nwatchers] = (void*) events;
loop->watchers[loop->nwatchers + 1] = (void*) ((uintptr_t) nfds);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Superfluous parentheses.

for (i = 0; i < nfds; i++) {
ev = events + i;
fd = ev->ident;
w = loop->watchers[fd];

/* Skip invalidated events */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe point to uv__platform_invalidate_fd() here, else future readers will wonder how the fd became -1 all of a sudden.

if (fd == -1)
continue;

if (w == NULL) {
/* File descriptor that we've stopped watching, disarm it. */
/* TODO batch up */
Expand Down Expand Up @@ -227,6 +234,8 @@ void uv__io_poll(uv_loop_t* loop, int timeout) {
w->cb(loop, w, revents);
nevents++;
}
loop->watchers[loop->nwatchers] = NULL;
loop->watchers[loop->nwatchers + 1] = NULL;

if (nevents != 0) {
if (nfds == ARRAY_SIZE(events) && --count != 0) {
Expand Down
28 changes: 28 additions & 0 deletions src/unix/linux-core.c
View file
Open in desktop
Expand Up @@ -97,6 +97,25 @@ void uv__platform_loop_delete(uv_loop_t* loop) {
}


void uv__platform_invalidate_fd(uv_loop_t* loop, int fd) {
struct uv__epoll_event* events;
uintptr_t i;
uintptr_t nfds;

assert(loop->watchers != NULL);

events = (struct uv__epoll_event*) loop->watchers[loop->nwatchers];
nfds = (uintptr_t) loop->watchers[loop->nwatchers + 1];
if (events == NULL)
return;

/* Invalidate events with same file descriptor */
for (i = 0; i < nfds; i++)
if ((int) events[i].data == fd)
events[i].data = -1;
}


void uv__io_poll(uv_loop_t* loop, int timeout) {
struct uv__epoll_event events[1024];
struct uv__epoll_event* pe;
Expand Down Expand Up @@ -190,10 +209,17 @@ void uv__io_poll(uv_loop_t* loop, int timeout) {

nevents = 0;

assert(loop->watchers != NULL);
loop->watchers[loop->nwatchers] = (void*) events;
loop->watchers[loop->nwatchers + 1] = (void*) ((intptr_t) nfds);
for (i = 0; i < nfds; i++) {
pe = events + i;
fd = pe->data;

/* Skip invalidated events */
if (fd == -1)
continue;

assert(fd >= 0);
assert((unsigned) fd < loop->nwatchers);

Expand All @@ -220,6 +246,8 @@ void uv__io_poll(uv_loop_t* loop, int timeout) {
w->cb(loop, w, masked_events);
nevents++;
}
loop->watchers[loop->nwatchers] = NULL;
loop->watchers[loop->nwatchers + 1] = NULL;

if (nevents != 0) {
if (nfds == ARRAY_SIZE(events) && --count != 0) {
Expand Down
28 changes: 28 additions & 0 deletions src/unix/sunos.c
View file
Open in desktop
Expand Up @@ -87,6 +87,25 @@ void uv__platform_loop_delete(uv_loop_t* loop) {
}


void uv__platform_invalidate_fd(uv_loop_t* loop, int fd) {
struct port_event* events;
uintptr_t i;
uintptr_t nfds;

assert(loop->watchers != NULL);

events = (struct port_event*) loop->watchers[loop->nwatchers];
nfds = (uintptr_t) loop->watchers[loop->nwatchers + 1];
if (events == NULL)
return;

/* Invalidate events with same file descriptor */
for (i = 0; i < nfds; i++)
if ((int) events[i].portev_object == fd)
events[i].portev_object = -1;
}


void uv__io_poll(uv_loop_t* loop, int timeout) {
struct port_event events[1024];
struct port_event* pe;
Expand Down Expand Up @@ -173,10 +192,17 @@ void uv__io_poll(uv_loop_t* loop, int timeout) {

nevents = 0;

assert(loop->watchers != NULL);
loop->watchers[loop->nwatchers] = (void*) events;
loop->watchers[loop->nwatchers + 1] = (void*) ((intptr_t) nfds);
for (i = 0; i < nfds; i++) {
pe = events + i;
fd = pe->portev_object;

/* Skip invalidated events */
if (fd == -1)
continue;

assert(fd >= 0);
assert((unsigned) fd < loop->nwatchers);

Expand All @@ -193,6 +219,8 @@ void uv__io_poll(uv_loop_t* loop, int timeout) {
if (w->pevents != 0 && ngx_queue_empty(&w->watcher_queue))
ngx_queue_insert_tail(&loop->watcher_queue, &w->watcher_queue);
}
loop->watchers[loop->nwatchers] = NULL;
loop->watchers[loop->nwatchers + 1] = NULL;

if (nevents != 0) {
if (nfds == ARRAY_SIZE(events) && --count != 0) {
Expand Down
2 changes: 2 additions & 0 deletions test/test-list.h
View file
Open in desktop
Expand Up @@ -64,6 +64,7 @@ TEST_DECLARE (tcp_connect_error_fault)
TEST_DECLARE (tcp_connect_timeout)
TEST_DECLARE (tcp_close_while_connecting)
TEST_DECLARE (tcp_close)
TEST_DECLARE (tcp_close_accept)
TEST_DECLARE (tcp_flags)
TEST_DECLARE (tcp_write_to_half_open_connection)
TEST_DECLARE (tcp_unexpected_read)
Expand Down Expand Up @@ -298,6 +299,7 @@ TASK_LIST_START
TEST_ENTRY (tcp_connect_timeout)
TEST_ENTRY (tcp_close_while_connecting)
TEST_ENTRY (tcp_close)
TEST_ENTRY (tcp_close_accept)
TEST_ENTRY (tcp_flags)
TEST_ENTRY (tcp_write_to_half_open_connection)
TEST_ENTRY (tcp_unexpected_read)
Expand Down