[PATCH] rdp: don't release the seat until it is safe v2

[Pekka Paalanen]

On Fri, 20 May 2016 15:02:02 +0200
Hardening <rdp.effort at gmail.com> wrote:

> Le 20/05/2016 11:58, Sam Spilsbury a écrit :
> > On Fri, May 20, 2016 at 5:33 PM, David Fort <rdp.effort at gmail.com> wrote:  
> >> Releasing a seat is not safe, so let's just announce it without keyboard
> >> and mouse until this is fixed. Without this patch we just can't reconnect on
> >> the RDP compositor as it crashes.
> >>
> >> v2: fixed the leak of the xkb_keymap
> >>
> >> Signed-off-by: David Fort <contact at hardening-consulting.com>
> >> ---
> >>  src/compositor-rdp.c | 33 ++++++++++++++++++++-------------
> >>  1 file changed, 20 insertions(+), 13 deletions(-)
> >>
> >> diff --git a/src/compositor-rdp.c b/src/compositor-rdp.c
> >> index 4fc7c74..52cf426 100644
> >> --- a/src/compositor-rdp.c
> >> +++ b/src/compositor-rdp.c
> >> @@ -109,7 +109,7 @@ enum peer_item_flags {
> >>  struct rdp_peers_item {
> >>         int flags;
> >>         freerdp_peer *peer;
> >> -       struct weston_seat seat;
> >> +       struct weston_seat *seat;
> >>
> >>         struct wl_list link;
> >>  };
> >> @@ -640,9 +640,9 @@ rdp_peer_context_free(freerdp_peer* client, RdpPeerContext* context)
> >>         }
> >>
> >>         if (context->item.flags & RDP_PEER_ACTIVATED) {
> >> -               weston_seat_release_keyboard(&context->item.seat);
> >> -               weston_seat_release_pointer(&context->item.seat);
> >> -               weston_seat_release(&context->item.seat);
> >> +               weston_seat_release_keyboard(context->item.seat);
> >> +               weston_seat_release_pointer(context->item.seat);
> >> +               /*weston_seat_release(context->item.seat);*/
> >>         }  
> > 
> > I think instead of just having commented out code, put the reasons why
> > the seat cannot be released safely at the moment. Just having it
> > commented out will confuse future maintainers.
> >   
> 
> Well, future maintainers actually means me. The explanation is quite
> long, I don't think a comment in the code is the right place for this.
> And BTW it's not something that is RDP compositor specific, all the
> weston compositors have it.

That is even more reason to add a comment there. Please do so, gather
review tags, and re-send. The future you is not the same as today's you.

You don't have to make comment exhaustive, even though that would be
good. As long as there is /* XXX: should weston_seat_release() here,
but it will crash on reconnect */ or something like that would be
enough.

> >>
> >>         Stream_Free(context->encode_stream, TRUE);
> >> @@ -911,9 +911,16 @@ xf_peer_activate(freerdp_peer* client)
> >>         else
> >>                 snprintf(seat_name, sizeof(seat_name), "RDP peer @%s", settings->ClientAddress);
> >>
> >> -       weston_seat_init(&peersItem->seat, b->compositor, seat_name);
> >> -       weston_seat_init_keyboard(&peersItem->seat, keymap);
> >> -       weston_seat_init_pointer(&peersItem->seat);
> >> +       peersItem->seat = zalloc(sizeof(*peersItem->seat));
> >> +       if (!peersItem->seat) {
> >> +               xkb_keymap_unref(keymap);
> >> +               weston_log("unable to create a weston_seat\n");
> >> +               return FALSE;
> >> +       }
> >> +
> >> +       weston_seat_init(peersItem->seat, b->compositor, seat_name);
> >> +       weston_seat_init_keyboard(peersItem->seat, keymap);
> >> +       weston_seat_init_pointer(peersItem->seat);  
> > 
> > Any reason to make this dynamically allocated memory? It seems to me
> > like it is adding an additional point of failure for little added
> > benefit. If it needs to be dynamically allocated, I think such a
> > change should come in a separate patch, since it seems unrelated to
> > this one.
> >   
> 
> I have already answered this in a previous mail to Bryce. To do it
> short, it's because the seat has to live longer than the RdpPeerContext.
> So when the RDP peer disconnects, the RdpPeerContext will be destroyed
> immediately, while the seat is supposed to live until all wayland client
> have released it.
> 
> 
> > Reviewed-by: Sam Spilsbury <smspillaz at gmail.com>
> >   

Yeah, it indeed seems no other backend dynamically destroys and
re-creates whole seats, they only do it with the specific device
capabilities, so the idea of this patch is good to me.

However, I think you really should store the weston_seat pointer
somewhere where it does not get lost, so you can re-use it, and destroy
it on compositor shutdown.

Anyway, leaking seems better than crashing, so:
Acked-by: Pekka Paalanen <pekka.paalanen at collabora.co.uk>


Thanks,
pq
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 811 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/wayland-devel/attachments/20160527/94e59655/attachment.sig>