connection: Leave fd open in wl_connection_destroy

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 <ben@0x539.de>
Reviewed-by: Marek Chalupa <mchqwerty@gmail.com>
Reviewed-by: Pekka Paalanen <pekka.paalanen@collabora.co.uk>

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