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

[Hardening]

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.

>>
>>         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>
> 


-- 
David FORT
website: http://www.hardening-consulting.com/