[PATCH] connection: Leave fd open in wl_connection_destroy

[Pekka Paalanen]

On Mon, 27 Oct 2014 09:55:46 +0100
Marek Chalupa <mchqwerty at gmail.com> wrote:

> 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>

Hi,

yeah, I see.

It is not explicitly documented, but it seems that wl_client_create()
takes the ownership of the fd only if it succeeds. That is how it is
being used. Also, wl_client_create() did not close the fd on some error
paths, but did on the others. This patch fixes wl_client_create() to
never close the fd on error.

We cannot comfortably make wl_connection_create() take the fd ownership
when it succeeds, because wl_client_create() can fail other ways later,
which means it would not be able to untake the fd. Or well, maybe we
could, but it doesn't really matter, since this is all internal. Or we
could use dup() which is perhaps what a good public API should do, but
that's overkill here, and wl_client_create does not dup anyway.

Looks good, pushed!


Thanks,
pq

> > ---
> >  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
> >