<div dir="ltr"> <div class="gmail_extra"> <div class="gmail_quote">On Wed, Mar 11, 2015 at 10:05 PM, Bryce Harrington <<a href="mailto:bryce@osg.samsung.com" target="_blank">bryce@osg.samsung.com</a>> wrote: <blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div class=""><div class="h5">On Tue, Feb 24, 2015 at 03:08:36PM +0100, Davide Bettio wrote: > commit e8b37de8e084d4e50a12bd2911657d54c0ebd9ed > Author: Davide Bettio <<a href="mailto:davide.bettio@ispirata.com">davide.bettio@ispirata.com</a>> > 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 <<a href="mailto:davide.bettio@ispirata.com">davide.bettio@ispirata.com</a>> > > 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); > + } </div></div>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? </blockquote><div> </div>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. <div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> 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. <div><div class="h5"> > *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"; </div></div>Aside from that, ditto daniel's review feedback. <div class=""><div class="h5"> > > _______________________________________________ > wayland-devel mailing list > <a href="mailto:wayland-devel@lists.freedesktop.org">wayland-devel@lists.freedesktop.org</a> > <a href="http://lists.freedesktop.org/mailman/listinfo/wayland-devel" target="_blank">http://lists.freedesktop.org/mailman/listinfo/wayland-devel</a> _______________________________________________ wayland-devel mailing list <a href="mailto:wayland-devel@lists.freedesktop.org">wayland-devel@lists.freedesktop.org</a> <a href="http://lists.freedesktop.org/mailman/listinfo/wayland-devel" target="_blank">http://lists.freedesktop.org/mailman/listinfo/wayland-devel</a> </div></div></blockquote></div> </div><div class="gmail_extra">We're reviewing this series in two threads, we should agree on one to not confuse others :) </div><div class="gmail_extra">The other is: <a href="http://patchwork.freedesktop.org/patch/43801/">http://patchwork.freedesktop.org/patch/43801/</a> </div></div>