[PATCH 2/6] rdp: don't release the seat until it is safe

[Hardening]

Le 20/05/2016 00:51, Bryce Harrington a écrit :
> On Tue, Apr 26, 2016 at 11:34:04PM +0200, David Fort 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.
>>
>> Signed-off-by: David Fort <contact at hardening-consulting.com>
> 
> I see that this patch disables the weston_seat_release() call (which
> sounds from the changelog works around the issue rather than fixes it).
> 
> I'm not spotting the need for making the seat dynamically allocated, is
> that part of the fix or just refactoring?  Regardless, shouldn't it get
> a corresponding free() 
> 

It's just a workaround. And the need to make it dynamic is that the
RdpPeerContext will be freed by FreeRDP, and we need the seat to live
longer (as it is supposed to live as long as a client is binding it).
For now we're just leaking the seat, but I guess that once seat
releasing will work, we will still need to have the seat dynamically
allocated, as the seat will live longer than the RdpPeerContext.

>> ---
>>  src/compositor-rdp.c | 32 +++++++++++++++++++-------------
>>  1 file changed, 19 insertions(+), 13 deletions(-)
>>
>> diff --git a/src/compositor-rdp.c b/src/compositor-rdp.c
>> index f6778b6..862eedc 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;
>>  };
>> @@ -623,9 +623,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);*/
>>  	}
>>  
>>  	Stream_Free(context->encode_stream, TRUE);
>> @@ -894,9 +894,15 @@ 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) {
>> +		weston_log("unable to create a weston_seat\n");
>> +		return FALSE;
> 
> Does the xkbContext need freed in this error condition?

Indeed, good catch I'm posting a fix for this.

> 
>> +	}
>> +
>> +	weston_seat_init(peersItem->seat, b->compositor, seat_name);
>> +	weston_seat_init_keyboard(peersItem->seat, keymap);
>> +	weston_seat_init_pointer(peersItem->seat);
>>  
>>  	peersItem->flags |= RDP_PEER_ACTIVATED;
>>  
>> @@ -935,7 +941,7 @@ xf_mouseEvent(rdpInput *input, UINT16 flags, UINT16 x, UINT16 y)
>>  	if (flags & PTR_FLAGS_MOVE) {
>>  		output = peerContext->rdpBackend->output;
>>  		if (x < output->base.width && y < output->base.height) {
>> -			notify_motion_absolute(&peerContext->item.seat, weston_compositor_get_time(),
>> +			notify_motion_absolute(peerContext->item.seat, weston_compositor_get_time(),
>>  					x, y);
>>  			need_frame = true;
>>  		}
>> @@ -949,7 +955,7 @@ xf_mouseEvent(rdpInput *input, UINT16 flags, UINT16 x, UINT16 y)
>>  		button = BTN_MIDDLE;
>>  
>>  	if (button) {
>> -		notify_button(&peerContext->item.seat, weston_compositor_get_time(), button,
>> +		notify_button(peerContext->item.seat, weston_compositor_get_time(), button,
>>  			(flags & PTR_FLAGS_DOWN) ? WL_POINTER_BUTTON_STATE_PRESSED : WL_POINTER_BUTTON_STATE_RELEASED
>>  		);
>>  		need_frame = true;
>> @@ -974,13 +980,13 @@ xf_mouseEvent(rdpInput *input, UINT16 flags, UINT16 x, UINT16 y)
>>  		weston_event.discrete = (int)value;
>>  		weston_event.has_discrete = true;
>>  
>> -		notify_axis(&peerContext->item.seat, weston_compositor_get_time(),
>> +		notify_axis(peerContext->item.seat, weston_compositor_get_time(),
>>  			    &weston_event);
>>  		need_frame = true;
>>  	}
>>  
>>  	if (need_frame)
>> -		notify_pointer_frame(&peerContext->item.seat);
>> +		notify_pointer_frame(peerContext->item.seat);
>>  
>>  	FREERDP_CB_RETURN(TRUE);
>>  }
>> @@ -993,7 +999,7 @@ xf_extendedMouseEvent(rdpInput *input, UINT16 flags, UINT16 x, UINT16 y)
>>  
>>  	output = peerContext->rdpBackend->output;
>>  	if (x < output->base.width && y < output->base.height) {
>> -		notify_motion_absolute(&peerContext->item.seat, weston_compositor_get_time(),
>> +		notify_motion_absolute(peerContext->item.seat, weston_compositor_get_time(),
>>  				x, y);
>>  	}
>>  
>> @@ -1056,7 +1062,7 @@ xf_input_keyboard_event(rdpInput *input, UINT16 flags, UINT16 code)
>>  
>>  		/*weston_log("code=%x ext=%d vk_code=%x scan_code=%x\n", code, (flags & KBD_FLAGS_EXTENDED) ? 1 : 0,
>>  				vk_code, scan_code);*/
>> -		notify_key(&peerContext->item.seat, weston_compositor_get_time(),
>> +		notify_key(peerContext->item.seat, weston_compositor_get_time(),
>>  					scan_code - 8, keyState, STATE_UPDATE_AUTOMATIC);
>>  	}
>>  
>> -- 
>> 1.9.1
>>
>> _______________________________________________
>> wayland-devel mailing list
>> wayland-devel at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/wayland-devel
> 
> Bryce
> 


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