On Wed, Mar 11, 2015 at 10:05 PM, Bryce Harrington <br...@osg.samsung.com>
wrote:
> On Tue, Feb 24, 2015 at 03:08:36PM +0100, Davide Bettio wrote:
> > commit e8b37de8e084d4e50a12bd2911657d54c0ebd9ed
> > Author: Davide Bettio <davide.bet...@ispirata.com>
> > Date: Tue Feb 24 12:40:49 2015 +0100
> >
> > * Extend WAYLAND_DISPLAY and name parameter semantics to support
> > absolute paths.
> > For example WAYLAND_DISPLAY="/my/path/wayland-2" or
> > connect_to_socket("/my/path/wayland-2").
> >
> > * Introduce WAYLAND_SERVER_SOCKET_DIR to change socket directory
> > without changing XDG_RUNTIME_DIR.
> > This might be useful to change socket directory in case
> > wl_display_add_socket_auto is used.
> > For example this will change the socket path for weston without
> > using any command line argument.
> > This envvar is ignored when WAYLAND_SERVER_SOCKET is set to an
> > absolute path.
> >
> > * Introduce WAYLAND_SERVER_SOCKET to change the path where the
> > server will create the socket.
> > It will be possible for a nested compositor to offer a socket
> > located in WAYLAND_SERVER_SOCKET path
> > while connecting to the main compositor socket that is located
> > in WAYLAND_DISPLAY path.
> > That's the reason why a futher WAYLAND_SERVER_SOCKET envvar has
> > been introduced.
> >
> > Wayland clients will not see any new environment variable, while
> > compositors may have to deal
> > with 2 extra environment variables, this is due the fact that a
> > compositor might be a client of
> > another compositor and we must make sure that WAYLAND_DISPLAY is
> > not used for both client
> > and server purposes.
> >
> > Signed-off-by: Davide Bettio <davide.bet...@ispirata.com>
> >
> > diff --git a/doc/man/wl_display_connect.xml
> > b/doc/man/wl_display_connect.xml
> > index 7e6e05c..b78c53f 100644
> > --- a/doc/man/wl_display_connect.xml
> > +++ b/doc/man/wl_display_connect.xml
> > @@ -59,10 +59,10 @@
> > find it. The <varname>name</varname> argument specifies
> > the name of
> > the socket or <constant>NULL</constant> to use the
> > default (which is
> > <constant>"wayland-0"</constant>). The environment variable
> > - <envar>WAYLAND_DISPLAY</envar> replaces the default value. If
> > - <envar>WAYLAND_SOCKET</envar> is set, this function
> > behaves like
> > - <function>wl_display_connect_to_fd</function> with the
> > file-descriptor
> > - number taken from the environment variable.</para>
> > + <envar>WAYLAND_DISPLAY</envar> replaces the default
> > value, and eventually uses
> > + a different path too. If <envar>WAYLAND_SOCKET</envar> is
> > set, this function
> > + behaves like
> > <function>wl_display_connect_to_fd</function> with the
> > + file-descriptor number taken from the environment
> > variable.</para>
> >
> > <para><function>wl_display_connect_to_fd</function> connects to
> > a Wayland
> > socket with an explicit file-descriptor. The
> > file-descriptor is passed
> > diff --git a/src/wayland-client.c b/src/wayland-client.c
> > index 0f1405c..cc01573 100644
> > --- a/src/wayland-client.c
> > +++ b/src/wayland-client.c
> > @@ -768,14 +768,26 @@ connect_to_socket(const char *name)
> >
> > memset(&addr, 0, sizeof addr);
> > addr.sun_family = AF_LOCAL;
> > - name_size =
> > - snprintf(addr.sun_path, sizeof addr.sun_path,
> > - "%s/%s", runtime_dir, name) + 1;
> > + if (name[0] != '/') {
> > + name_size =
> > + snprintf(addr.sun_path, sizeof addr.sun_path,
> > + "%s/%s", runtime_dir, name) + 1;
> > + } else {
> > + /* absolute path */
> > + name_size =
> > + snprintf(addr.sun_path, sizeof addr.sun_path,
> > + "%s", name) + 1;
> > + }
> >
> > assert(name_size > 0);
> > if (name_size > (int)sizeof addr.sun_path) {
> > - wl_log("error: socket path \"%s/%s\" plus null terminator"
> > - " exceeds 108 bytes\n", runtime_dir, name);
> > + if (name[0] != '/') {
> > + wl_log("error: socket path \"%s/%s\" plus null
> terminator"
> > + " exceeds 108 bytes\n", runtime_dir, name);
> > + } else {
> > + wl_log("error: socket path \"%s\" plus null
> terminator"
> > + " exceeds 108 bytes\n", name);
> > + }
>
> Perhaps instead of the 108 magic number in the log message, print the
> actual numerical value of (int)sizeof addr.sun_path ?
>
> > close(fd);
> > /* to prevent programs reporting
> > * "failed to add socket: Success" */
> > diff --git a/src/wayland-server.c b/src/wayland-server.c
> > index 0558634..39ba13f 100644
> > --- a/src/wayland-server.c
> > +++ b/src/wayland-server.c
> > @@ -1093,9 +1093,12 @@ wl_socket_init_for_display_name(struct
> > wl_socket *s, const char *name)
> > int name_size;
> > const char *runtime_dir;
> >
> > - runtime_dir = getenv("XDG_RUNTIME_DIR");
> > + runtime_dir = getenv("WAYLAND_SERVER_SOCKET_DIR");
> > if (!runtime_dir) {
> > - wl_log("error: XDG_RUNTIME_DIR not set in the
> environment\n");
> > + runtime_dir = getenv("XDG_RUNTIME_DIR");
> > + }
> > + if (!runtime_dir) {
> > + wl_log("error: XDG_RUNTIME_DIR or
> WAYLAND_SERVER_SOCKET_DIR not
> > set in the environment\n");
>
> Maybe reword this to avoid any ambiguity:
>
> wl_log("error: Neither XDG_RUNTIME_DIR nor
> WAYLAND_SERVER_SOCKET_DIR have been
> set in the environment\n");
>
> Or, perhaps less passively:
>
> wl_log("error: Either XDG_RUNTIME_DIR or
> WAYLAND_SERVER_SOCKET_DIR must be
> set in the environment\n");
>
> > /* to prevent programs reporting
> > * "failed to add socket: Success" */
> > @@ -1104,15 +1107,28 @@ wl_socket_init_for_display_name(struct
> > wl_socket *s, const char *name)
> > }
> >
> > s->addr.sun_family = AF_LOCAL;
> > - name_size = snprintf(s->addr.sun_path, sizeof s->addr.sun_path,
> > - "%s/%s", runtime_dir, name) + 1;
> > + if (name[0] != '/') {
> > + name_size = snprintf(s->addr.sun_path, sizeof
> s->addr.sun_path,
> > + "%s/%s", runtime_dir, name) + 1;
> > +
> > + s->display_name = (s->addr.sun_path + name_size - 1) -
> strlen(name);
>
> I know this is the original code, and I'm probably missing something,
> but...
> If runtime_dir is of length N, and s->addr.sun_path is let's say 2N, and
> name is length 8N, then name_size = 2N+1, and only the first N-1 chars are
> used from name. I might be off by 1 or 2.
>
> But, now we have
>
> s->display_name = (2N + 2N + 1) - 8N = -4N + 1
>
> Do we have a buffer underflow possibility here?
>
Actually, this will work, but the code is not correct. snprintf returns
number of characters that it would have written if it had enough of space
available (if it returns number < sizeof s->addr.sun_path everything is
ok). So when name is of length 8N, then name_size is 8N + N + 1, and thus
we have 8N + N + 1 - 1 - 8N = N which is the end of xdg_runtime string -
therefore the display_name will point to add.sun_path and it will be valid
pointer. Problem is that the name can be truncated, so we should check for
the result of snprintf in any case.
> In any case, may as well check for error return from snprintf
> (i.e. name_size < 0).
>
> > + } else {
> > + //absolute path
> > + name_size = snprintf(s->addr.sun_path, sizeof
> s->addr.sun_path,
> > + "%s", name) + 1;
> >
> > - s->display_name = (s->addr.sun_path + name_size - 1) -
> strlen(name);
> > + s->display_name = strrchr(s->addr.sun_path, '/') + 1;
> > + }
> >
> > assert(name_size > 0);
> > if (name_size > (int)sizeof s->addr.sun_path) {
> > - wl_log("error: socket path \"%s/%s\" plus null terminator"
> > - " exceeds 108 bytes\n", runtime_dir, name);
> > + if (name[0] != '/') {
> > + wl_log("error: socket path \"%s/%s\" plus null
> terminator"
> > + " exceeds 108 bytes\n", runtime_dir,
> > name);
> > + } else {
> > + wl_log("error: socket path \"%s\" plus null
> terminator"
> > + " exceeds 108 bytes\n", name);
> > + }
>
> 108's here too.
>
> > *s->addr.sun_path = 0;
> > /* to prevent programs reporting
> > * "failed to add socket: Success" */
> > @@ -1203,18 +1219,18 @@ wl_display_add_socket_auto(struct wl_display
> > *display)
> > * This adds a Unix socket to Wayland display which can be used by
> > clients to
> > * connect to Wayland display.
> > *
> > - * If NULL is passed as name, then it would look for
> > WAYLAND_DISPLAY env
> > - * variable for the socket name. If WAYLAND_DISPLAY is not set,
> > then default
> > - * wayland-0 is used.
> > + * If NULL is passed as name, then it would look in order for
> > WAYLAND_SERVER_SOCKET
> > + * and WAYLAND_SERVER env variable for the socket name. If
> > WAYLAND_DISPLAY and
> > + * WAYLAND_SERVER_SOCKET are not set, then default wayland-0 is used.
> > *
> > - * The Unix socket will be created in the directory pointed to by
> > environment
> > - * variable XDG_RUNTIME_DIR. If XDG_RUNTIME_DIR is not set, then
> > this function
> > - * fails and returns -1.
> > + * The Unix socket will be created in the directory pointed to by
> > either environment
> > + * variable WAYLAND_SERVER_SOCKET_DIR or XDG_RUNTIME_DIR. If the
> > none of the is set,
> > + * then this function fails and returns -1.
> > *
> > - * The length of socket path, i.e., the path set in XDG_RUNTIME_DIR
> > and the
> > + * The length of socket path, e.g., the path set in XDG_RUNTIME_DIR
> > and the
> > * socket name, must not exceed the maxium length of a Unix socket
> > path.
> > * The function also fails if the user do not have write permission
> > in the
> > - * XDG_RUNTIME_DIR path or if the socket name is already in use.
> > + * wayland socket path or if the socket name is already in use.
> > *
> > * \memberof wl_display
> > */
> > @@ -1228,6 +1244,8 @@ wl_display_add_socket(struct wl_display
> > *display, const char *name)
> > return -1;
> >
> > if (name == NULL)
> > + name = getenv("WAYLAND_SERVER_SOCKET");
> > + if (name == NULL)
> > name = getenv("WAYLAND_DISPLAY");
> > if (name == NULL)
> > name = "wayland-0";
>
> Aside from that, ditto daniel's review feedback.
>
> >
> > _______________________________________________
> > wayland-devel mailing list
> > wayland-devel@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/wayland-devel
> _______________________________________________
> wayland-devel mailing list
> wayland-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/wayland-devel
>
We're reviewing this series in two threads, we should agree on one to not
confuse others :)
The other is: http://patchwork.freedesktop.org/patch/43801/
_______________________________________________
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/wayland-devel
