<div dir="ltr"><div><div><div>Hi, </div><div> </div>at first glance I didn't like returning fd from wl_connection_destroy, but at the other, I did! If you think about the connection as a buffer for the fd (and that is really the case), then it make sense to do something like: </div><div> create conn ------------------ destroy conn </div>fd ----------------------> | connection | ------------------------> fd ------------------ </div><div>The race in threaded environment is real (one thread closes fd, say 6, other thread opens new fd - it will get the first free value which is 6 - and the first thread close the 6 again and we're in trouble..) </div><div>So it's important to close every fd only once. </div><div> </div><div>For me it's OK and: </div>Reviewed-by: Marek Chalupa <<a href="mailto:mchqwerty@gmail.com">mchqwerty@gmail.com</a>> </div><div class="gmail_extra"> <div class="gmail_quote">On 30 September 2014 14:43, Benjamin Herr <<a href="mailto:ben@0x539.de" target="_blank">ben@0x539.de</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Calling close() on the same file descriptor that a previous call to close() already closed is wrong, and racy if another thread received that same file descriptor as a eg. new socket or actual file. There are two situations where wl_connection_destroy() would close its file descriptor and then another function up in the call chain would close the same file descriptor: * When wl_client_create() fails after calling wl_connection_create(), it will call wl_connection_destroy() before returning. However, its caller will always close the file descriptor if wl_client_create() fails. * wl_display_disconnect() unconditionally closes the display file descriptor and also calls wl_connection_destroy(). So these two seem to expect wl_connection_destroy() to leave the file descriptor open. The other caller of wl_connection_destroy(), wl_client_destroy(), does however expect wl_connection_destroy() to close its file descriptor, alas. This patch changes wl_connection_destroy() to indulge this majority of two callers by simply not closing the file descriptor. For the benefit of wl_client_destroy(), wl_connection_destroy() then returns the unclosed file descriptor so that wl_client_destroy() can close it itself. Since wl_connection_destroy() is a private function called from few places, changing its semantics seemed like the more expedient way to address the double-close() problem than shuffling around the logic in wl_client_create() to somehow enable it to always avoid calling wl_connection_destroy(). Signed-off-by: Benjamin Herr <<a href="mailto:ben@0x539.de">ben@0x539.de</a>> --- src/connection.c | 7 +++++-- src/wayland-private.h | 2 +- src/wayland-server.c | 2 +- tests/connection-test.c | 8 ++++++-- 4 files changed, 13 insertions(+), 6 deletions(-) diff --git a/src/connection.c b/src/connection.c index f292853..c5daca6 100644 --- a/src/connection.c +++ b/src/connection.c @@ -189,13 +189,16 @@ close_fds(struct wl_buffer *buffer, int max) buffer->tail += size; } -void +int wl_connection_destroy(struct wl_connection *connection) { + int fd = connection->fd; + close_fds(&connection->fds_out, -1); close_fds(&connection->fds_in, -1); - close(connection->fd); free(connection); + + return fd; } void diff --git a/src/wayland-private.h b/src/wayland-private.h index 67e8783..db76081 100644 --- a/src/wayland-private.h +++ b/src/wayland-private.h @@ -86,7 +86,7 @@ int wl_interface_equal(const struct wl_interface *iface1, const struct wl_interface *iface2); struct wl_connection *wl_connection_create(int fd); -void wl_connection_destroy(struct wl_connection *connection); +int wl_connection_destroy(struct wl_connection *connection); void wl_connection_copy(struct wl_connection *connection, void *data, size_t size); void wl_connection_consume(struct wl_connection *connection, size_t size); diff --git a/src/wayland-server.c b/src/wayland-server.c index 674aeca..7caeb30 100644 --- a/src/wayland-server.c +++ b/src/wayland-server.c @@ -670,7 +670,7 @@ wl_client_destroy(struct wl_client *client) wl_map_for_each(&client->objects, destroy_resource, &serial); wl_map_release(&client->objects); wl_event_source_remove(client->source); - wl_connection_destroy(client->connection); + close(wl_connection_destroy(client->connection)); wl_list_remove(&client->link); free(client); } diff --git a/tests/connection-test.c b/tests/connection-test.c index 659bf68..4497128 100644 --- a/tests/connection-test.c +++ b/tests/connection-test.c @@ -57,6 +57,7 @@ TEST(connection_create) connection = setup(s); wl_connection_destroy(connection); + close(s[0]); close(s[1]); } @@ -74,6 +75,7 @@ TEST(connection_write) assert(memcmp(message, buffer, sizeof message) == 0); wl_connection_destroy(connection); + close(s[0]); close(s[1]); } @@ -92,6 +94,7 @@ TEST(connection_data) wl_connection_consume(connection, sizeof message); wl_connection_destroy(connection); + close(s[0]); close(s[1]); } @@ -117,6 +120,7 @@ TEST(connection_queue) assert(memcmp(message, buffer + sizeof message, sizeof message) == 0); wl_connection_destroy(connection); + close(s[0]); close(s[1]); } @@ -147,8 +151,8 @@ setup_marshal_data(struct marshal_data *data) static void release_marshal_data(struct marshal_data *data) { - wl_connection_destroy(data->read_connection); - wl_connection_destroy(data->write_connection); + close(wl_connection_destroy(data->read_connection)); + close(wl_connection_destroy(data->write_connection)); } static void -- 2.1.1 _______________________________________________ wayland-devel mailing list <a href="mailto:wayland-devel@lists.freedesktop.org">wayland-devel@lists.freedesktop.org</a> <a href="http://lists.freedesktop.org/mailman/listinfo/wayland-devel" target="_blank">http://lists.freedesktop.org/mailman/listinfo/wayland-devel</a> </blockquote></div> </div>