Re: [PATCH] connection: Leave fd open in wl_connection_destroy

2014-11-04 Thread Pekka Paalanen 
On Mon, 27 Oct 2014 09:55:46 +0100
Marek Chalupa mchqwe...@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 mchqwe...@gmail.com
 
 On 30 September 2014 14:43, Benjamin Herr b...@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 b...@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,

Re: [PATCH] connection: Leave fd open in wl_connection_destroy

2014-10-27 Thread 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 mchqwe...@gmail.com

On 30 September 2014 14:43, Benjamin Herr b...@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 b...@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]);