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

[Pekka Paalanen]

On Thu, 21 Aug 2014 09:05:48 -0700
Jason Ekstrand <jason at jlekstrand.net> wrote:

> 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

Let's see if there is any fallout during the alpha period, and think
about the stable branch then.


Thanks,
pq