[PATCH] connection: Leave fd open in wl_connection_destroy

[Marek Chalupa]

Hi,

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:

      create conn      ------------------     destroy conn
fd ----------------------> | connection | ------------------------> fd
                             ------------------

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..)
So it's important to close every fd only once.

For me it's OK and:

Reviewed-by: Marek Chalupa <mchqwerty at gmail.com>

On 30 September 2014 14:43, Benjamin Herr <ben at 0x539.de> wrote:

> 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 at 0x539.de>
> ---
>  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
> wayland-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/wayland-devel
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/wayland-devel/attachments/20141027/7fad9884/attachment.html>