[PATCH wayland] server: Don't expose wl_display as a global

[Jason Ekstrand]

On Thu, Aug 21, 2014 at 4:00 AM, Pekka Paalanen <ppaalanen at gmail.com> wrote:

> On Thu,  7 Aug 2014 09:55:49 -0400
> "Jasper St. Pierre" <jstpierre at mecheye.net> wrote:
>
> > The idea here was that once upon a time, clients could rebind wl_display
> > to a higher version, so we offered the ability to rebind it
> > here. However, this is particularly broken. The existing bind
> > implementation actually still hardcodes version numbers, and it leaks
> > previous resources, overwriting the existing one.
> >
> > The newly bound resource *also* won't have any listeners attached by the
> > client, meaning that the error and delete_id events won't get delivered
> > correctly. Unless the client poked into libwayland internals, it also
> > can't possibly set up these handlers correctly either, so the client
> > will sustain errors and leak all deleted globals.
> >
> > Since this never worked correctly in the first place, we can feel safe
> > removing it.
> > ---
> >  src/wayland-server.c | 28 ++++++++--------------------
> >  1 file changed, 8 insertions(+), 20 deletions(-)
> >
> > diff --git a/src/wayland-server.c b/src/wayland-server.c
> > index 3c162d4..dc3f502 100644
> > --- a/src/wayland-server.c
> > +++ b/src/wayland-server.c
> > @@ -380,9 +380,8 @@ wl_client_get_display(struct wl_client *client)
> >       return client->display;
> >  }
> >
> > -static void
> > -bind_display(struct wl_client *client,
> > -          void *data, uint32_t version, uint32_t id);
> > +static int
> > +bind_display(struct wl_client *client, struct wl_display *display);
> >
> >  /** Create a client for the given file descriptor
> >   *
> > @@ -440,9 +439,7 @@ wl_client_create(struct wl_display *display, int fd)
> >               goto err_map;
> >
> >       wl_signal_init(&client->destroy_signal);
> > -     bind_display(client, display, 1, 1);
> > -
> > -     if (!client->display_resource)
> > +     if (bind_display(client, display) < 0)
> >               goto err_map;
> >
> >       wl_list_insert(display->client_list.prev, &client->link);
> > @@ -772,22 +769,20 @@ destroy_client_display_resource(struct wl_resource
> *resource)
> >       resource->client->display_resource = NULL;
> >  }
> >
> > -static void
> > -bind_display(struct wl_client *client,
> > -          void *data, uint32_t version, uint32_t id)
> > +static int
> > +bind_display(struct wl_client *client, struct wl_display *display)
> >  {
> > -     struct wl_display *display = data;
> > -
> >       client->display_resource =
> > -             wl_resource_create(client, &wl_display_interface, 1, id);
> > +             wl_resource_create(client, &wl_display_interface, 1, 1);
> >       if (client->display_resource == NULL) {
> >               wl_client_post_no_memory(client);
> > -             return;
> > +             return -1;
> >       }
> >
> >       wl_resource_set_implementation(client->display_resource,
> >                                      &display_interface, display,
> >                                      destroy_client_display_resource);
> > +     return 0;
> >  }
> >
> >  /** Create Wayland display object.
> > @@ -831,13 +826,6 @@ wl_display_create(void)
> >
> >       wl_array_init(&display->additional_shm_formats);
> >
> > -     if (!wl_global_create(display, &wl_display_interface, 1,
> > -                           display, bind_display)) {
> > -             wl_event_loop_destroy(display->loop);
> > -             free(display);
> > -             return NULL;
> > -     }
> > -
> >       return display;
> >  }
> >
>
> I asked krh about this in irc, and he eventually agreed that exposing
> wl_display as a global is not so useful.
>
> I reviewed the patch, and the alleged problems the current code has,
> and I agree they are real.
>
> If we ever end up needing wl_display as a global, we can easily add it
> back. Since the implementation was indeed broken, no client should ever
> try to bind to wl_display global with version 1.
>
> Therefore, pushed with ack from Jason and R-b from me.
>
> In fact, I think this warrants this patch as a condidate for the stable
> branch.
>

Yeah, it probably counts as a bug-fix.  It does change behaviour though, so
I'm not sure I really want to call it a stable thing.  Unless, of course,
re-binding wl_display could cause a crash; in which case, push it to 1.5
--Jason


>
>
> Thanks,
> pq
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/wayland-devel/attachments/20140821/5c2c34e7/attachment.html>