Re: [PATCH weston] RDP compositor: make the seat dynamic and don't destroy it on removal

[Pekka Paalanen]

On Fri, 25 Sep 2015 16:07:12 -0500
Derek Foreman  wrote:

> On 25/09/15 02:46 PM, Hardening wrote:
> > Le 25/09/2015 19:15, Derek Foreman a écrit :
> >> On 25/09/15 08:19 AM, Hardening wrote:
> >>> Le 25/09/2015 11:31, Pekka Paalanen a écrit :
>  On Thu, 24 Sep 2015 23:40:26 +0200
>  David FORT  wrote:
> 
> > This patch makes the seat dynamic and leak it on purpose during seat 
> > removal to
> > prevent the ghost object case.
> > ---
> > 
> > [...]
> > 
> >>>
> >>> Hello Pekka,
> >>> you're right I've not elaborated much.
> >>>
> >>> So the general problem was that wl_seat doesn't have a release request,
> >>> so we can't track the usage of a wl_seat by wayland clients. And as we
> >>> can't track the usage, we can't release it safely or we take the risk
> >>> that clients could address a released object. In the current situation
> >>> we can't safely release a seat object.
> >>
> >> Should we move forward with your patch to add seat release protocol first?

Yeah, that's a very good idea.

> >> Does that change this implementation at all?
> > 
> > Yeah this could be an idea. Once the "release seat patch" is passed, the
> > goal would be to not leak the seat at all, so the present patch would be
> > useless.
> 
> It looked like it needed only trivial changes to land (some version
> bumps...)
> 
> I don't know how to handle old clients (or buggy new ones) that don't
> bother sending the seat release request though...

And unfortunately this problem still won't go away. If you want to keep
everything working, you're going to have to implement both leaking and
non-leaking code paths.

But this holds true for all of Weston. All the wl_seat protocol stuff
is core stuff, so I can't see why everyone would not have this problem
already. It's just that RDP actually does also destroy seats often which
exposes the problem.

So, it might be useful to split the weston_seat_release() function in
Weston core into two: one that leaks, and one that doesn't. Who knows,
maybe we could hide all the leaking nastyness into the core?

I suspect it's getting hairy, so I'd probably prefer a solution in the
core to automatically leak things as necessary. (Up to client
disconnect, they won't be leaked forever.) Then we get to the topic of
inert objects...

Anyone brought a lawnmower for this yak? :-P


Thanks,
pq


pgpCwso8v0ife.pgp
Description: OpenPGP digital signature
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH weston] RDP compositor: make the seat dynamic and don't destroy it on removal

[Derek Foreman]

On 25/09/15 02:46 PM, Hardening wrote:
> Le 25/09/2015 19:15, Derek Foreman a écrit :
>> On 25/09/15 08:19 AM, Hardening wrote:
>>> Le 25/09/2015 11:31, Pekka Paalanen a écrit :
 On Thu, 24 Sep 2015 23:40:26 +0200
 David FORT  wrote:

> This patch makes the seat dynamic and leak it on purpose during seat 
> removal to
> prevent the ghost object case.
> ---
> 
> [...]
> 
>>>
>>> Hello Pekka,
>>> you're right I've not elaborated much.
>>>
>>> So the general problem was that wl_seat doesn't have a release request,
>>> so we can't track the usage of a wl_seat by wayland clients. And as we
>>> can't track the usage, we can't release it safely or we take the risk
>>> that clients could address a released object. In the current situation
>>> we can't safely release a seat object.
>>
>> Should we move forward with your patch to add seat release protocol first?
>>
>> Does that change this implementation at all?
> 
> Yeah this could be an idea. Once the "release seat patch" is passed, the
> goal would be to not leak the seat at all, so the present patch would be
> useless.

It looked like it needed only trivial changes to land (some version
bumps...)

I don't know how to handle old clients (or buggy new ones) that don't
bother sending the seat release request though...

>>
>>> So in my patch I'm mallocating a seat (not having it static with the RDP
>>> context), an I took the parts of weston_seat_release() that only do
>>> things internally to weston (so that I release as much as I can).
>>>
>>>
> 
> [...]
>>>
>>>
>>> Anyway without the patch, valgrind complains during seat releasing (for
>>> example when a RDP peer disconnects).
>>
>> Sigh.  There's a lot of multi-seat related problems in the
>> text-backend/input-method bits.  I've got a bunch of patches I need to
>> sort through and post. :/
>>
>> I don't think you've added this problem, I think it's already there (and
>> I may have fixed it already...)
>>
> 
> I can test with these patches to see if things get better, any reference ?

Ouch, turns out I haven't rebased it lately and it's pretty messed up
right now, I'll try to get it back into shape early next week.

> 
> Best regards.
> 

___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH weston] RDP compositor: make the seat dynamic and don't destroy it on removal

[Hardening]

Le 25/09/2015 19:15, Derek Foreman a écrit :
> On 25/09/15 08:19 AM, Hardening wrote:
>> Le 25/09/2015 11:31, Pekka Paalanen a écrit :
>>> On Thu, 24 Sep 2015 23:40:26 +0200
>>> David FORT  wrote:
>>>
 This patch makes the seat dynamic and leak it on purpose during seat 
 removal to
 prevent the ghost object case.
 ---

[...]

>>
>> Hello Pekka,
>> you're right I've not elaborated much.
>>
>> So the general problem was that wl_seat doesn't have a release request,
>> so we can't track the usage of a wl_seat by wayland clients. And as we
>> can't track the usage, we can't release it safely or we take the risk
>> that clients could address a released object. In the current situation
>> we can't safely release a seat object.
> 
> Should we move forward with your patch to add seat release protocol first?
> 
> Does that change this implementation at all?

Yeah this could be an idea. Once the "release seat patch" is passed, the
goal would be to not leak the seat at all, so the present patch would be
useless.

> 
>> So in my patch I'm mallocating a seat (not having it static with the RDP
>> context), an I took the parts of weston_seat_release() that only do
>> things internally to weston (so that I release as much as I can).
>>
>>

[...]
>>
>>
>> Anyway without the patch, valgrind complains during seat releasing (for
>> example when a RDP peer disconnects).
> 
> Sigh.  There's a lot of multi-seat related problems in the
> text-backend/input-method bits.  I've got a bunch of patches I need to
> sort through and post. :/
> 
> I don't think you've added this problem, I think it's already there (and
> I may have fixed it already...)
> 

I can test with these patches to see if things get better, any reference ?


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

___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH weston] RDP compositor: make the seat dynamic and don't destroy it on removal

[Hardening]

Le 25/09/2015 11:31, Pekka Paalanen a écrit :
> On Thu, 24 Sep 2015 23:40:26 +0200
> David FORT  wrote:
> 
>> This patch makes the seat dynamic and leak it on purpose during seat removal 
>> to
>> prevent the ghost object case.
>> ---
>>  src/compositor-rdp.c | 39 +++
>>  1 file changed, 27 insertions(+), 12 deletions(-)
> 
> Hi David,
> 
> this patch is still missing the whole explanation of what is going on in
> here. My questions from
> http://lists.freedesktop.org/archives/wayland-devel/2015-May/022055.html
> are still unanswered, and today I understand even less than then. :-)
> 
> Or is there already a comment in the code explaining why rdp-backend
> does funny stuff with seats? I couldn't find it on a quick look.
> 
> Why leak it?
> What is the ghost object problem?
> Why you must use only part of weston_seat_release()?
> 
> I have some very vague memories of wl_seat missing destructor protocol
> or something, is this related?


Hello Pekka,
you're right I've not elaborated much.

So the general problem was that wl_seat doesn't have a release request,
so we can't track the usage of a wl_seat by wayland clients. And as we
can't track the usage, we can't release it safely or we take the risk
that clients could address a released object. In the current situation
we can't safely release a seat object.

So in my patch I'm mallocating a seat (not having it static with the RDP
context), an I took the parts of weston_seat_release() that only do
things internally to weston (so that I release as much as I can).


Perhaps I'm not cleaning up correctly because with my patch applied, if I:
* open a terminal;
* connect with 2 RDP clients (so 2 seats);
* take the focus in the terminal;
* stop weston with a Ctrl-C

I get the following valgrind traceback during weston's shutdown, related
to the input method:
^C[14:35:47.687] caught signal 2
==21909== Invalid read of size 8
==21909==at 0x4173B9: unbind_input_method (text-backend.c:834)
==21909==by 0x4E3D37B: destroy_resource (wayland-server.c:537)
==21909==by 0x4E42255: for_each_helper.isra.0 (wayland-util.c:359)
==21909==by 0x4E4279E: wl_map_for_each (wayland-util.c:365)
==21909==by 0x4E3DF57: wl_client_destroy (wayland-server.c:675)
==21909==by 0x418401: text_backend_destroy (text-backend.c:1046)
==21909==by 0x93542F2: shell_destroy (shell.c:6473)
==21909==by 0x4109C6: weston_compositor_destroy
(wayland-server-core.h:264)
==21909==by 0x40897D: main (main.c:829)
==21909==  Address 0x9b9e6f0 is 112 bytes inside a block of size 120 free'd
==21909==at 0x4C2BDEC: free (in
/usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==21909==by 0x6A468AE: rdp_peer_context_free (wayland-server-core.h:264)
==21909==by 0x6D520DA: freerdp_peer_context_free (peer.c:738)
==21909==by 0x6A469B7: rdp_client_activity (compositor-rdp.c:674)
==21909==by 0x4E3FE51: wl_event_loop_dispatch (event-loop.c:422)
==21909==by 0x4E3E6B4: wl_display_run (wayland-server.c:1004)
==21909==by 0x408D35: main (main.c:818)
==21909==


Anyway without the patch, valgrind complains during seat releasing (for
example when a RDP peer disconnects).

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

___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH weston] RDP compositor: make the seat dynamic and don't destroy it on removal

[Pekka Paalanen]

On Thu, 24 Sep 2015 23:40:26 +0200
David FORT  wrote:

> This patch makes the seat dynamic and leak it on purpose during seat removal 
> to
> prevent the ghost object case.
> ---
>  src/compositor-rdp.c | 39 +++
>  1 file changed, 27 insertions(+), 12 deletions(-)

Hi David,

this patch is still missing the whole explanation of what is going on in
here. My questions from
http://lists.freedesktop.org/archives/wayland-devel/2015-May/022055.html
are still unanswered, and today I understand even less than then. :-)

Or is there already a comment in the code explaining why rdp-backend
does funny stuff with seats? I couldn't find it on a quick look.

Why leak it?
What is the ghost object problem?
Why you must use only part of weston_seat_release()?

I have some very vague memories of wl_seat missing destructor protocol
or something, is this related?


Thanks,
pq

> diff --git a/src/compositor-rdp.c b/src/compositor-rdp.c
> index c221eb9..7abffee 100644
> --- a/src/compositor-rdp.c
> +++ b/src/compositor-rdp.c
> @@ -110,7 +110,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;
>  };
> @@ -628,6 +628,7 @@ static void
>  rdp_peer_context_free(freerdp_peer* client, RdpPeerContext* context)
>  {
>   int i;
> + struct weston_seat *seat;
>   if (!context)
>   return;
>  
> @@ -638,9 +639,17 @@ rdp_peer_context_free(freerdp_peer* client, 
> RdpPeerContext* context)
>   }
>  
>   if (context->item.flags & RDP_PEER_ACTIVATED) {
> - weston_seat_release_keyboard(>item.seat);
> - weston_seat_release_pointer(>item.seat);
> - weston_seat_release(>item.seat);
> + seat = context->item.seat;
> + weston_seat_release_keyboard(seat);
> + weston_seat_release_pointer(seat);
> +
> + /* picked from weston_seat_release(context->item.seat); */
> + wl_list_remove(>link);
> + if (seat->saved_kbd_focus)
> + wl_list_remove(>saved_kbd_focus_listener.link);
> + wl_global_destroy(seat->global);
> + wl_signal_emit(>destroy_signal, seat);
> +
>   }
>  
>   Stream_Free(context->encode_stream, TRUE);
> @@ -910,9 +919,15 @@ xf_peer_activate(freerdp_peer* client)
>   else
>   snprintf(seat_name, sizeof(seat_name), "RDP peer @%s", 
> settings->ClientAddress);
>  
> - weston_seat_init(>seat, b->compositor, seat_name);
> - weston_seat_init_keyboard(>seat, keymap);
> - weston_seat_init_pointer(>seat);
> + peersItem->seat = zalloc(sizeof(*peersItem->seat));
> + if (!peersItem->seat) {
> + weston_log("unable to allocate the seat for %s", seat_name);
> + return FALSE;
> + }
> +
> + weston_seat_init(peersItem->seat, >compositor, seat_name);
> + weston_seat_init_keyboard(peersItem->seat, keymap);
> + weston_seat_init_pointer(peersItem->seat);
>  
>   peersItem->flags |= RDP_PEER_ACTIVATED;
>  
> @@ -953,7 +968,7 @@ xf_mouseEvent(rdpInput *input, UINT16 flags, UINT16 x, 
> UINT16 y)
>   if (x < output->base.width && y < output->base.height) {
>   wl_x = wl_fixed_from_int((int)x);
>   wl_y = wl_fixed_from_int((int)y);
> - notify_motion_absolute(>item.seat, 
> weston_compositor_get_time(),
> + notify_motion_absolute(peerContext->item.seat, 
> weston_compositor_get_time(),
>   wl_x, wl_y);
>   }
>   }
> @@ -966,7 +981,7 @@ xf_mouseEvent(rdpInput *input, UINT16 flags, UINT16 x, 
> UINT16 y)
>   button = BTN_MIDDLE;
>  
>   if (button) {
> - notify_button(>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
>   );
>   }
> @@ -982,7 +997,7 @@ xf_mouseEvent(rdpInput *input, UINT16 flags, UINT16 x, 
> UINT16 y)
>   if (flags & PTR_FLAGS_WHEEL_NEGATIVE)
>   axis = -axis;
>  
> - notify_axis(>item.seat, 
> weston_compositor_get_time(),
> + notify_axis(peerContext->item.seat, 
> weston_compositor_get_time(),
>   WL_POINTER_AXIS_VERTICAL_SCROLL,
>   axis);
>   }
> @@ -1001,7 +1016,7 @@ xf_extendedMouseEvent(rdpInput *input, UINT16 flags, 
> UINT16 x, UINT16 y)
>   if (x < output->base.width && y < output->base.height) {
>   wl_x = wl_fixed_from_int((int)x);
>   wl_y = wl_fixed_from_int((int)y);
> -