[PATCH v4 wayland] client: Allow absolute paths in WAYLAND_DISPLAY

Matt Hoosier matt.hoosier at gmail.com
Wed Dec 13 14:19:46 UTC 2017


On Mon, Dec 11, 2017 at 2:03 AM, Pekka Paalanen <ppaalanen at gmail.com> wrote:
> On Mon, 27 Nov 2017 08:54:54 -0600
> Matt Hoosier <matt.hoosier at gmail.com> wrote:
>
>> From: Matt Hoosier <matt.hoosier at garmin.com>
>>
>> In order to support system compositor instances, it is necessary to
>> allow clients' wl_display_connect() to find the compositor's listening
>> socket somewhere outside of XDG_RUNTIME_DIR. For a full account, see
>> the discussion beginning here:
>>
>> https://lists.freedesktop.org/archives/wayland-devel/2017-November/035664.html
>>
>> This change adjusts the client-side connection logic so that, if
>> WAYLAND_DISPLAY is formatted as an absolute pathname, the socket
>> connection attempt is made to just $WAYLAND_DISPLAY rather than
>> usual user-private location $XDG_RUNTIME_DIR/$WAYLAND_DISPLAY.
>>
>> This change is based on Davide Bettio's submission of the same concept
>> at:
>>
>> https://lists.freedesktop.org/archives/wayland-devel/2015-August/023838.html.
>>
>> v4 changes:
>>
>> * Improved internal comments and some boundary-condition
>>   error checks in test case.
>> * Refer to compositor as "Wayland server" rather than "Wayland
>>   display" in wl_display_connect() doxygen comments.
>> * Remove redundant descriptions of parameter-interpretation
>>   mechanics from wl_display_connect() manpage. Reworked things
>>   to make it clear that 'name' and $WAYLAND_DISLAY are each
>>   capable of encoding absolute server socket paths.
>> * Remove callout to reference implementation behavior in protocol
>>   documented. In its place there is now a simple statement that
>>   implementations can optionally support absolute socket paths.
>>
>> v3 changes:
>>
>> * Added test case.
>> * Clarified documentation to note that 'name' parameter to wl_display_connect()
>>   can also be an absolute path.
>>
>> v2 changes:
>>
>> * Added backward incompatibility note to wl_display_connect() manpage.
>> * Rephased wl_display_connect() manpage changes to precisely match actual
>>   changed behavior.
>> * Added mention of new absolute path behavior in wl_display_connect()
>>   doxygen comments.
>> * Mentioned new absolute path interpretation of WAYLAND_DISPLAY in
>>   protocol documentation.
>>
>> Signed-off-by: Matt Hoosier <matt.hoosier at gmail.com>
>> Acked-by: Pekka Paalanen <pekka.paalanen at collabora.co.uk>
>> Acked-by: Jonas Ã…dahl <jadahl at gmail.com>
>> ---
>>  doc/man/wl_display_connect.xml    |  32 +++++++++--
>>  doc/publican/sources/Protocol.xml |   5 +-
>>  src/wayland-client.c              |  47 ++++++++++++----
>>  tests/socket-test.c               | 109 ++++++++++++++++++++++++++++++++++++++
>>  4 files changed, 177 insertions(+), 16 deletions(-)
>
> Hi Matt,
>
> this patch is:
>
> Reviewed-by: Pekka Paalanen <pekka.paalanen at collabora.co.uk>
>
> The wording in the man page sounds little like WAYLAND_DISPLAY
> accepting an absolute path is a side-effect rather than an intentional
> feature, but it doesn't matter.
>
> Everyone,
>
> if there are no objections, I will push this patch on Wednesday, that
> is in two days. If you want your R-b or Acks recorded that are not
> already in the above, please send them explicitly.
>
>
> Thanks,
> pq

Hi Pekka,

Did you ever hear any objections to this one?

-Matt

>
>>
>> diff --git a/doc/man/wl_display_connect.xml b/doc/man/wl_display_connect.xml
>> index 7e6e05c..dab4ddb 100644
>> --- a/doc/man/wl_display_connect.xml
>> +++ b/doc/man/wl_display_connect.xml
>> @@ -55,15 +55,39 @@
>>      <title>Description</title>
>>      <para><function>wl_display_connect</function> connects to a Wayland socket
>>            that was previously opened by a Wayland server. The server socket must
>> -          be placed in <envar>XDG_RUNTIME_DIR</envar> for this function to
>> -          find it. The <varname>name</varname> argument specifies the name of
>> +          be placed in <envar>XDG_RUNTIME_DIR</envar> when <envar>WAYLAND_DISPLAY</envar>
>> +       (or <varname>name</varname>, see below) is a simple name, for this
>> +       function to find it. The server socket is also allowed to exist at an
>> +       arbitrary path; usage details follow. See below for compatibility issue
>> +       details.</para>
>> +
>> +    <para>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
>> +          <envar>WAYLAND_DISPLAY</envar> replaces the default value.
>> +
>> +       If <varname>name</varname> is an absolute path, then that path is used
>> +       as the Wayland socket to which the connection is attempted. Note that
>> +       in combination with the default-value behavior described above, this
>> +       implies that setting <envar>WAYLAND_DISPLAY</envar> to an absolute
>> +       path will implicitly cause <varname>name</varname> to take on that
>> +       absolute path if <varname>name</varname> is <constant>NULL</constant>.
>> +
>> +          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>Support for interpreting <envar>WAYLAND_DISPLAY</envar> as an
>> +          absolute path is a change in behavior compared to
>> +          <function>wl_display_connect</function>'s behavior in versions
>> +          1.14 and older of Wayland. It is no longer guaranteed in versions
>> +          1.15 and higher that the Wayland socket chosen is equivalent to
>> +          manually constructing a socket pathname by concatenating
>> +          <envar>XDG_RUNTIME_DIR</envar> and <envar>WAYLAND_DISPLAY</envar>.
>> +          Manual construction of the socket path must account for the
>> +          possibility that <envar>WAYLAND_DISPLAY</envar> contains an absolute
>> +          path.</para>
>> +
>>      <para><function>wl_display_connect_to_fd</function> connects to a Wayland
>>            socket with an explicit file-descriptor. The file-descriptor is passed
>>            as argument <varname>fd</varname>.</para>
>> diff --git a/doc/publican/sources/Protocol.xml b/doc/publican/sources/Protocol.xml
>> index ba6b5f1..9fdee9a 100644
>> --- a/doc/publican/sources/Protocol.xml
>> +++ b/doc/publican/sources/Protocol.xml
>> @@ -94,7 +94,10 @@
>>        The protocol is sent over a UNIX domain stream socket, where the endpoint
>>        usually is named <systemitem class="service">wayland-0</systemitem>
>>        (although it can be changed via <emphasis>WAYLAND_DISPLAY</emphasis>
>> -      in the environment).
>> +      in the environment). Beginning in Wayland 1.15, implementations can
>> +      optionally support server socket endpoints located at arbitrary
>> +      locations in the filesystem by setting <emphasis>WAYLAND_DISPLAY</emphasis>
>> +      to the absolute path at which the server endpoint listens.
>>      </para>
>>      <para>
>>        Every message is structured as 32-bit words; values are represented in the
>> diff --git a/src/wayland-client.c b/src/wayland-client.c
>> index 3d7361e..ecb8840 100644
>> --- a/src/wayland-client.c
>> +++ b/src/wayland-client.c
>> @@ -857,9 +857,17 @@ connect_to_socket(const char *name)
>>       socklen_t size;
>>       const char *runtime_dir;
>>       int name_size, fd;
>> +     bool path_is_absolute;
>> +
>> +     if (name == NULL)
>> +             name = getenv("WAYLAND_DISPLAY");
>> +     if (name == NULL)
>> +             name = "wayland-0";
>> +
>> +     path_is_absolute = name[0] == '/';
>>
>>       runtime_dir = getenv("XDG_RUNTIME_DIR");
>> -     if (!runtime_dir) {
>> +     if (!runtime_dir && !path_is_absolute) {
>>               wl_log("error: XDG_RUNTIME_DIR not set in the environment.\n");
>>               /* to prevent programs reporting
>>                * "failed to create display: Success" */
>> @@ -867,25 +875,32 @@ connect_to_socket(const char *name)
>>               return -1;
>>       }
>>
>> -     if (name == NULL)
>> -             name = getenv("WAYLAND_DISPLAY");
>> -     if (name == NULL)
>> -             name = "wayland-0";
>> -
>>       fd = wl_os_socket_cloexec(PF_LOCAL, SOCK_STREAM, 0);
>>       if (fd < 0)
>>               return -1;
>>
>>       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 (!path_is_absolute) {
>> +             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 (!path_is_absolute) {
>> +                     wl_log("error: socket path \"%s/%s\" plus null terminator"
>> +                            " exceeds %i bytes\n", runtime_dir, name, (int) sizeof(addr.sun_path));
>> +             } else {
>> +                     wl_log("error: socket path \"%s\" plus null terminator"
>> +                            " exceeds %i bytes\n", name, (int) sizeof(addr.sun_path));
>> +             }
>>               close(fd);
>>               /* to prevent programs reporting
>>                * "failed to add socket: Success" */
>> @@ -994,6 +1009,16 @@ wl_display_connect_to_fd(int fd)
>>   * its value will be replaced with the WAYLAND_DISPLAY environment
>>   * variable if it is set, otherwise display "wayland-0" will be used.
>>   *
>> + * If \c name is an absolute path, then that path is used as-is for
>> + * the location of the socket at which the Wayland server is listening;
>> + * no qualification inside XDG_RUNTIME_DIR is attempted.
>> + *
>> + * If \c name is \c NULL and the WAYLAND_DISPLAY environment variable
>> + * is set to an absolute pathname, then that pathname is used as-is
>> + * for the socket in the same manner as if \c name held an absolute
>> + * path. Support for absolute paths in \c name and WAYLAND_DISPLAY
>> + * is present since Wayland version 1.15.
>> + *
>>   * \memberof wl_display
>>   */
>>  WL_EXPORT struct wl_display *
>> diff --git a/tests/socket-test.c b/tests/socket-test.c
>> index bb034f4..e970562 100644
>> --- a/tests/socket-test.c
>> +++ b/tests/socket-test.c
>> @@ -26,10 +26,14 @@
>>  #include <errno.h>
>>  #include <string.h>
>>  #include <stdio.h>
>> +#include <sys/socket.h>
>> +#include <sys/types.h>
>> +#include <sys/wait.h>
>>  #include <sys/un.h>
>>  #include <unistd.h>
>>
>>  #include "wayland-client.h"
>> +#include "wayland-os.h"
>>  #include "wayland-server.h"
>>  #include "test-runner.h"
>>
>> @@ -173,3 +177,108 @@ TEST(add_socket_auto)
>>
>>       wl_display_destroy(d);
>>  }
>> +
>> +struct client_create_listener {
>> +     struct wl_listener listener;
>> +     struct wl_display *display;
>> +};
>> +
>> +struct client_destroy_listener {
>> +     struct wl_listener listener;
>> +     struct wl_display *display;
>> +};
>> +
>> +static void
>> +client_destroy_notify(struct wl_listener *l, void *data)
>> +{
>> +     struct client_destroy_listener *listener =
>> +             wl_container_of(l, listener, listener);
>> +     wl_display_terminate(listener->display);
>> +     free(listener);
>> +}
>> +
>> +static void
>> +client_create_notify(struct wl_listener *l, void *data)
>> +{
>> +     struct wl_client *client = data;
>> +     struct client_create_listener *listener =
>> +             wl_container_of(l, listener, listener);
>> +     struct client_destroy_listener *destroy_listener = (struct client_destroy_listener *)malloc(sizeof *destroy_listener);
>> +     assert(destroy_listener != NULL);
>> +     destroy_listener->display = listener->display;
>> +     destroy_listener->listener.notify = client_destroy_notify;
>> +     wl_client_add_destroy_listener(client, &destroy_listener->listener);
>> +}
>> +
>> +TEST(absolute_socket_path)
>> +{
>> +     struct wl_display *display;
>> +     struct client_create_listener client_create_listener;
>> +     struct sockaddr_un addr;
>> +     int fd;
>> +     socklen_t size;
>> +     const char *xdg_runtime_dir;
>> +     size_t len;
>> +     int ret;
>> +     pid_t pid;
>> +
>> +     /* It's a little weird that this test about absolute socket paths
>> +      * uses XDG_RUNTIME_DIR, but that's the only location guaranteed
>> +      * by test-runner to be both writable and unique. This isn't
>> +      * really a problem; we'll just take care that the leaf-level
>> +      * filename used for the socket isn't anything that would
>> +      * accidentally be generated by a default usage of wl_display_connect(). */
>> +     xdg_runtime_dir = require_xdg_runtime_dir();
>> +     memset(&addr, 0, sizeof addr);
>> +     len = snprintf(addr.sun_path, sizeof addr.sun_path,
>> +                    "%s/%s", xdg_runtime_dir, "wayland-absolute-0");
>> +     assert(len < sizeof addr.sun_path
>> +            && "Bug in test. Path too long");
>> +
>> +     /* The path must not exist prior to binding. */
>> +     assert(access(addr.sun_path, F_OK) == -1);
>> +
>> +     size = offsetof (struct sockaddr_un, sun_path) + strlen(addr.sun_path);
>> +     addr.sun_family = AF_LOCAL;
>> +     fd = wl_os_socket_cloexec(PF_LOCAL, SOCK_STREAM, 0);
>> +     assert(fd >= 0 );
>> +     ret = bind(fd, (struct sockaddr *) &addr, size);
>> +     assert(ret >= 0);
>> +     ret = listen(fd, 128);
>> +     assert(ret >= 0);
>> +
>> +     /* Start server display. Be careful (by avoiding wl_display_add_socket_auto()
>> +      * to offer only the absolutely qualified socket made above. */
>> +     display = wl_display_create();
>> +     assert(display != NULL);
>> +     client_create_listener.listener.notify = client_create_notify;
>> +     client_create_listener.display = display;
>> +     wl_display_add_client_created_listener(display, &client_create_listener.listener);
>> +     ret = wl_display_add_socket_fd(display, fd);
>> +     assert(ret == 0);
>> +
>> +     /* Execute client that connects to the absolutely qualified server socket path. */
>> +     pid = fork();
>> +     assert(pid != -1);
>> +
>> +     if (pid == 0) {
>> +             ret = setenv("WAYLAND_DISPLAY", addr.sun_path, 1);
>> +             assert(ret == 0);
>> +             struct wl_display *client_display = wl_display_connect(NULL);
>> +             assert(client_display != NULL);
>> +             ret = wl_display_roundtrip(client_display);
>> +             assert(ret != -1);
>> +             wl_display_disconnect(client_display);
>> +             exit(0);
>> +             assert(false);
>> +     }
>> +
>> +     wl_display_run(display);
>> +     ret = waitpid(pid, NULL, 0);
>> +     assert(ret == pid);
>> +
>> +     wl_display_destroy(display);
>> +
>> +     ret = unlink(addr.sun_path);
>> +     assert(ret == 0);
>> +}
>


More information about the wayland-devel mailing list