[PATCH wayland v4] server: Add a socket with an existing fd

[Pekka Paalanen]

On Thu, 17 Dec 2015 17:08:19 -0800
Bryce Harrington <bryce at osg.samsung.com> wrote:

> This adds functionality to allow system-level control over handing out
> file descriptors for sockets, to allow tighter security when running a
> Wayland compositor under a Wayland session server.

Hi Bryce,

this is looking good. Commit message could also mention, that this new
API also allows writing socket activated Wayland servers.

> Signed-off-by: Bryce Harrington <bryce at osg.samsung.com>
> Cc: Sung-Jin Park <sj76.park at samsung.com>
> Cc: Sangjin Lee <lsj119 at samsung.com>
> 
> ---
> v2:
>  + Drop tab corrections
>  + Add patch to move if statement into assert
> 
> v3:
>  + Removed wl_os_socket_check_cloexec
>  + Removed wl_display_add_socket_fd_auto
>  + Replaced _wl_display_add_socket
>  + Rewrote wl_display_add_socket_fd
> 
> v4:
>  + Rewrote wl_display_add_socket_fd
>  + Remove everything except just storing the fd
>  + Document that we're assuming the caller sets up the fd properly
> 
>  src/wayland-server-core.h |  3 +++
>  src/wayland-server.c      | 43 +++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 46 insertions(+)
> 
> diff --git a/src/wayland-server-core.h b/src/wayland-server-core.h
> index 85b4b9e..1700cd3 100644
> --- a/src/wayland-server-core.h
> +++ b/src/wayland-server-core.h
> @@ -131,6 +131,9 @@ wl_display_add_socket(struct wl_display *display, const char *name);
>  const char *
>  wl_display_add_socket_auto(struct wl_display *display);
>  
> +int
> +wl_display_add_socket_fd(struct wl_display *display, int sock_fd);
> +
>  void
>  wl_display_terminate(struct wl_display *display);
>  
> diff --git a/src/wayland-server.c b/src/wayland-server.c
> index 1364d5d..a4fcc96 100644
> --- a/src/wayland-server.c
> +++ b/src/wayland-server.c
> @@ -1198,6 +1198,49 @@ wl_display_add_socket_auto(struct wl_display *display)
>  	return NULL;
>  }
>  
> +/**  Add a socket with an existing fd to Wayland display for the clients to connect.
> + *
> + * \param display Wayland display to which the socket should be added.
> + * \param name Name of the Unix socket.

There is no 'name' anymore, but sock_fd is.

> + * \return 0 if success. -1 if failed.
> + *
> + * The existing socket fd must already be created, opened, and locked.
> + * The fd must be properly set to CLOEXEC and bound to a socket file
> + * with both bind() and listen() already called.

Yeah, I think that is the complete list of requirements. Very good.

> + *
> + * \memberof wl_display
> + */
> +WL_EXPORT int
> +wl_display_add_socket_fd(struct wl_display *display, int sock_fd)
> +{
> +	struct wl_socket *s;
> +	struct stat buf;
> +
> +	/* Require a valid fd or fail */
> +	if (sock_fd < 0 || fstat(sock_fd, &buf) < 0 || !S_ISSOCK(buf.st_mode)) {
> +		return -1;
> +	}
> +
> +	s = wl_socket_alloc();
> +	if (s == NULL)
> +		return -1;
> +
> +	/* Reuse the existing fd */
> +	s->fd = sock_fd;
> +
> +	s->source = wl_event_loop_add_fd(display->loop, s->fd,
> +					 WL_EVENT_READABLE,
> +					 socket_data, display);
> +	if (s->source == NULL) {
> +		wl_log("failed to establish event source\n");
> +		return -1;
> +	}
> +
> +	wl_list_insert(display->socket_list.prev, &s->link);
> +
> +	return 0;
> +}

The implementation looks good. A good decision to polish this first so
you don't have to change the tests twice more if there were anything
wrong here.

> +
>  /** Add a socket to Wayland display for the clients to connect.
>   *
>   * \param display Wayland display to which the socket should be added.

If you fix the doc comment, and regardless if you add to the commit
message or not:

Reviewed-by: Pekka Paalanen <pekka.paalanen at collabora.co.uk>

There was some good material in your earlier series, I hope to see
those going forward too. :-)


Thanks,
pq
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 811 bytes
Desc: OpenPGP digital signature
URL: <http://lists.freedesktop.org/archives/wayland-devel/attachments/20151218/0eeda669/attachment.sig>