Re: [PATCH] Destroy resources when destroying input and output
After looking at it a bit more I don't think we can really make the automatic zombie idea work. The trouble is that libwayland-server would need to know when the client has destroyed the proxy in order to destroy the zombie resource. However the destroy semantics are interface-dependent so only an implementation of the interface can know when the resource is really destroyed. The wl_output interface doesn't currently have a destructor so in this particular case it's even worse. I guess if a client binds to an output then the resource for it has to leak until the client disconnects. Assuming we later add a release request to wl_output (I think we should) then I think we would still need to make the zombie handling specific to the output implementation in Weston. We can't just set the implementation to NULL because then nothing would destroy the resource when the release request is called. Perhaps the best thing to do would be to make Weston swap out the interface for a simple one that only waits for the release event when an output is unplugged. We would also have to do this for pointers, keyboards and touch devices. This could end up with quite a lot of code because you have to double up all of the implementations. We also have to be aware of these zombie objects if we ever add any more requests that can refer to these objects because they would have to check if the object is a zombie and skip the request. If there's ever a Wayland 2.0 I think it would be good to make all interfaces implicitly have a destructor which is handled outside of the protocol description. I can't think of an interface that shouldn't have a destructor. Even with wl_callback which is defined to be destroyed when it is signalled it might make sense to allow the client to destroy the callback early if it's no longer interested. Regards, - Neil ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH] Destroy resources when destroying input and output
On Wed, 14 May 2014 19:12:20 +0100 Neil Roberts n...@linux.intel.com wrote: After looking at it a bit more I don't think we can really make the automatic zombie idea work. The trouble is that libwayland-server would need to know when the client has destroyed the proxy in order to destroy the zombie resource. However the destroy semantics are interface-dependent so only an implementation of the interface can know when the resource is really destroyed. The wl_output interface doesn't currently have a destructor so in this particular case it's even worse. I guess if a client binds to an output then the resource for it has to leak until the client disconnects. Assuming we later add a release request to wl_output (I think we should) then I think we would still need to make the zombie handling specific to the output implementation in Weston. We can't just set the implementation to NULL because then nothing would destroy the resource when the release request is called. I think you are totally right. Perhaps the best thing to do would be to make Weston swap out the interface for a simple one that only waits for the release event when an output is unplugged. We would also have to do this for pointers, keyboards and touch devices. This could end up with quite a lot of code because you have to double up all of the implementations. We also have to be aware of these zombie objects if we ever add any more requests that can refer to these objects because they would have to check if the object is a zombie and skip the request. Right. If there's ever a Wayland 2.0 I think it would be good to make all interfaces implicitly have a destructor which is handled outside of the protocol description. I can't think of an interface that shouldn't have a destructor. Even with wl_callback which is defined to be destroyed when it is signalled it might make sense to allow the client to destroy the callback early if it's no longer interested. Yeah, I think there are ways to make it work, if we could change it. Another idea would be to annotate destructors and allow both requests and events to be destructors. I think I have suggested this before, but we could start collecting a Wayland protocol extension designer's cook book: rules of thumb (e.g. always define destructor protocol unless it is explicitly not right), design patterns (inert objects, factory interfaces...), where and how to define errors etc. Thanks, pq ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH] Destroy resources when destroying input and output
On Thu, 8 May 2014 19:32:12 -0500 Jason Ekstrand ja...@jlekstrand.net wrote: On Tue, Dec 31, 2013 at 3:37 AM, Pekka Paalanen ppaala...@gmail.com wrote: You cannot just go and destroy wl_resources, because there are clients using them - that is why the wl_resources exist in the first place. Clients use all protocol objects they have, and if the protocol does not say an object is destroyed, then it must be legal to use it, regardless of what happens in the server. If you can guarantee, that clients know the protocol object has gone away without any races, then the clients have already destroyed the protocol objects themselves, which means that you have no wl_resources to destroy in the list. If you cannot guarantee that, then the protocol objects (wl_resources) must continue to exist in such a way, that clients do not get a fatal protocol error when using them. The objects just do nothing. This state is called inert. If this principle is broken, it usually leads to races between the server and the client, which sometimes leads to a fatal protocol error and causes disconnection of the client. This happens, when the server destroys a wl_resource, and then receives a request from the client for that destroyed resource. The client may have sent the request even before the wl_resource was destroyed in the server, at which point it was a legal request from the client's perspective (this is the essence of the race). However, since wl_output interface contains no requests, your patch might be right. A client cannot send a wl_output request after the wl_resource has been destroyed in the server, because the interface has no requests. The wl_output could still be used as an argument to other requests in other interfaces, though. Whether that is fatal, or just silently gets turned into NULL argument in libwayland-server, I do not remember off-hand. Would be best to verify what happens in that case, since it must not be fatal to the client. Yes, I just looked it up. If a client sends a request with the output as an argument after the output gets destroyed, the client will get an INVALID_OBJECT protocol error and disconnect. This is a problem. If this patch leads to a race with possibly a protocol error as the result (think about output hot-unplug), then I think you would need to turn the wl_resource into an inert object instead of destroying it directly. The way to do this would be to set dummy handler and call wl_resource_set_destructor(resource, NULL); This leaves the resource inert and allows us to destroy the underlying object. The resources will get cleaned up when the client quits. Looking at it in general, there is one more fun complication. If the inert object has requests in its interface, that create new objects, the server cannot just ignore those requests. I think the server will need to actually create new objects if requested, but make those inert too. Otherwise there would again be a mismatch between the server and the client on which objects exist. Thanks, pq ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH] Destroy resources when destroying input and output
Perhaps we should consider applying the patch anyway even though it's not ideal. Currently if a client uses a dead output in a request such as xdg_surface.set_output Weston will end up with a weston_output pointer that points to freed memory. This could cause the compositor to crash. That is worse than the alternative provided by this patch which is to make the client abort. The clients know about the output being destroyed via the wl_registry.global_remove event so in practice they would only hit the problem in the unlikely event that they used the output in a request in the short time between the output being unplugged and noticing the removal event. In the longer term I was thinking maybe it would be good to handle the inert resource idea within libwayland-server. We could add a function like wl_resource_zombify() which would mark the resource as a zombie and call the destroy handlers. From the compositor's perspective it can then act as if the resource has been destroyed. We could detect zombie resources being used within the request marshalling code and ignore the request. If the request creates new resource we can internally create new zombie resources too and Weston would never need to know about it. I am planning to experiment with this approach now. Regards, - Neil ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH] Destroy resources when destroying input and output
On Fri, 09 May 2014 14:33:58 +0100 Neil Roberts n...@linux.intel.com wrote: Perhaps we should consider applying the patch anyway even though it's not ideal. Currently if a client uses a dead output in a request such as xdg_surface.set_output Weston will end up with a weston_output pointer that points to freed memory. This could cause the compositor to crash. True, it's very bad now. That is worse than the alternative provided by this patch which is to make the client abort. The clients know about the output being destroyed via the wl_registry.global_remove event so in practice they would only hit the problem in the unlikely event that they used the output in a request in the short time between the output being unplugged and noticing the removal event. That is also true, but if it is fixed improperly now, there is a very good chance we just forget about the problem, and never fix it for real. Especially when it becomes very hard to trigger. At least make sure we have an open bug report about it, please. In the longer term I was thinking maybe it would be good to handle the inert resource idea within libwayland-server. We could add a function like wl_resource_zombify() which would mark the resource as a zombie and call the destroy handlers. From the compositor's perspective it can then act as if the resource has been destroyed. We could detect zombie resources being used within the request marshalling code and ignore the request. If the request creates new resource we can internally create new zombie resources too and Weston would never need to know about it. I am planning to experiment with this approach now. Hmm... will be interesting to see, if that works out. It does feel like quite a lot of magic in libwayland-server, while also making life a lot easier. Thanks, pq ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH] Destroy resources when destroying input and output
On Fri, May 9, 2014 at 1:37 AM, Pekka Paalanen ppaala...@gmail.com wrote: Looking at it in general, there is one more fun complication. If the inert object has requests in its interface, that create new objects, the server cannot just ignore those requests. I think the server will need to actually create new objects if requested, but make those inert too. Otherwise there would again be a mismatch between the server and the client on which objects exist. I was hoping that this wasn't actually going to be a problem, but on wl_seat it is. The server can destroy a seat while a client is calling wl_seat.get_pointer for instance. Then, yes, we do have to create an intert object. On Fri, May 9, 2014 at 9:02 AM, Pekka Paalanen ppaala...@gmail.com wrote: On Fri, 09 May 2014 14:33:58 +0100 Neil Roberts n...@linux.intel.com wrote: Perhaps we should consider applying the patch anyway even though it's not ideal. Currently if a client uses a dead output in a request such as xdg_surface.set_output Weston will end up with a weston_output pointer that points to freed memory. This could cause the compositor to crash. True, it's very bad now. That is worse than the alternative provided by this patch which is to make the client abort. The clients know about the output being destroyed via the wl_registry.global_remove event so in practice they would only hit the problem in the unlikely event that they used the output in a request in the short time between the output being unplugged and noticing the removal event. That is also true, but if it is fixed improperly now, there is a very good chance we just forget about the problem, and never fix it for real. Especially when it becomes very hard to trigger. At least make sure we have an open bug report about it, please. I agree. This is bad and we need to make sure it gets fixed properly. In the longer term I was thinking maybe it would be good to handle the inert resource idea within libwayland-server. We could add a function like wl_resource_zombify() which would mark the resource as a zombie and call the destroy handlers. From the compositor's perspective it can then act as if the resource has been destroyed. We could detect zombie resources being used within the request marshalling code and ignore the request. If the request creates new resource we can internally create new zombie resources too and Weston would never need to know about it. I am planning to experiment with this approach now. Hmm... will be interesting to see, if that works out. It does feel like quite a lot of magic in libwayland-server, while also making life a lot easier. Most of the magic there is in allowing resources with no handler in libwayland-server. The patch would be about 4 lines. Right now, client-side wl_proxy objects are allowed to have a NULL implementation and there's no problem; server-side, this is not currently allowed. If we allowed this ten Neil's wl_resource_zombify would only have to set the destructor, and implementation to NULL. For that matter, we could just do that explicitly in weston and not add API for it. The big chunk of magic is in handling new_id requests. We would need to create zombie wl_resource's for each new_id argument on a call to a zombified wl_resource. Now that Pekka mentioned it, I'm not sure that we're handling those correctly client-side if there's no implementation set. --Jason Ekstrand ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH] Destroy resources when destroying input and output
Jason Ekstrand ja...@jlekstrand.net writes: Most of the magic there is in allowing resources with no handler in libwayland-server. The patch would be about 4 lines. Right now, client-side wl_proxy objects are allowed to have a NULL implementation and there's no problem; server-side, this is not currently allowed. If we allowed this ten Neil's wl_resource_zombify would only have to set the destructor, and implementation to NULL. For that matter, we could just do that explicitly in weston and not add API for it. Don't forget that we also want to ignore requests that have zombie resources as arguments, not just if the resource is directly used as a target of a request. It looks like the client-side already has code to handle zombie objects by putting a marker called WL_ZOMBIE_OBJECT in the ID map. Perhaps we should use the same mechanism on the server side too. If a zombie object is received in an event on the client side it looks like wl_closure_lookup_objects just sets the proxy object to NULL. Is that safe? Wouldn't the event handlers crash if they get a NULL resource in an event? If we can somehow make wl_closure_lookup_objects report when it finds a zombie object we can make the upper layers just consume the event. We could do this on both the client and side and the server side. - Neil ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH] Destroy resources when destroying input and output
On Tue, Dec 31, 2013 at 3:37 AM, Pekka Paalanen ppaala...@gmail.com wrote:
On Wed, 25 Dec 2013 17:02:09 +0100
Mariusz Ceier mceier+wayl...@gmail.com wrote:
Some structures containing resources list are freed before
resources on that list are destroyed, and that triggers invalid
read/write in compositor.c:3103 indirectly called from
text-backend.c:938 when running weston under valgrind.
This patch destroys resources on these lists before such
structures are freed.
That fixes at least x11 and freerds backend.
Signed-off-by: Mariusz Ceier mceier+wayl...@gmail.com
---
src/compositor.c | 4
src/input.c | 15 ---
2 files changed, 16 insertions(+), 3 deletions(-)
diff --git a/src/compositor.c b/src/compositor.c
index ff0f3ab..a4077e8 100644
--- a/src/compositor.c
+++ b/src/compositor.c
@@ -3183,6 +3183,7 @@ weston_compositor_verify_pointers(struct
weston_compositor *ec) WL_EXPORT void
weston_output_destroy(struct weston_output *output)
{
+ struct wl_resource *resource, *next_resource;
output-destroying = 1;
weston_compositor_remove_output(output-compositor,
output); @@ -3192,6 +3193,9 @@ weston_output_destroy(struct
weston_output *output)
wl_signal_emit(output-destroy_signal, output);
+ wl_resource_for_each_safe(resource, next_resource,
output-resource_list)
+ wl_resource_destroy(resource);
+
Hi,
I didn't check the other cases, but this case seems suspicious at
first. There is the following fundamental principle with Wayland
protocol objects as I understand it.
You cannot just go and destroy wl_resources, because there are
clients using them - that is why the wl_resources exist in the
first place. Clients use all protocol objects they have, and if the
protocol does not say an object is destroyed, then it must be legal
to use it, regardless of what happens in the server.
If you can guarantee, that clients know the protocol object has
gone away without any races, then the clients have already
destroyed the protocol objects themselves, which means that you
have no wl_resources to destroy in the list.
If you cannot guarantee that, then the protocol objects
(wl_resources) must continue to exist in such a way, that clients
do not get a fatal protocol error when using them. The objects just
do nothing. This state is called inert.
If this principle is broken, it usually leads to races between the
server and the client, which sometimes leads to a fatal protocol
error and causes disconnection of the client. This happens, when the
server destroys a wl_resource, and then receives a request from the
client for that destroyed resource. The client may have sent the
request even before the wl_resource was destroyed in the server, at
which point it was a legal request from the client's perspective
(this is the essence of the race).
However, since wl_output interface contains no requests, your patch
might be right. A client cannot send a wl_output request after the
wl_resource has been destroyed in the server, because the interface
has no requests.
The wl_output could still be used as an argument to other requests
in other interfaces, though. Whether that is fatal, or just
silently gets turned into NULL argument in libwayland-server, I do
not remember off-hand. Would be best to verify what happens in that
case, since it must not be fatal to the client.
Yes, I just looked it up. If a client sends a request with the output as
an argument after the output gets destroyed, the client will get an
INVALID_OBJECT protocol error and disconnect. This is a problem.
If this patch leads to a race with possibly a protocol error as the
result (think about output hot-unplug), then I think you would need
to turn the wl_resource into an inert object instead of destroying
it directly.
The way to do this would be to set dummy handler and call
wl_resource_set_destructor(resource, NULL); This leaves the resource inert
and allows us to destroy the underlying object. The resources will get
cleaned up when the client quits. The reason for the invalid read/write is
because the resource destructor calls wl_list_remove on the resource's
internal wl_list link. Because we deleted the underlying weston_output
object, this results in an invalid read/write.
The same idea applies to the hunks below, too.
Thanks,
pq
free(output-name);
pixman_region32_fini(output-region);
pixman_region32_fini(output-previous_damage);
diff --git a/src/input.c b/src/input.c
index 07e9d6c..062a2cb 100644
--- a/src/input.c
+++ b/src/input.c
@@ -471,10 +471,13 @@ weston_pointer_create(struct weston_seat
*seat) WL_EXPORT void
weston_pointer_destroy(struct weston_pointer *pointer)
{
+ struct wl_resource *resource, *next_resource;
+
if (pointer-sprite)
pointer_unmap_sprite(pointer);
- /* XXX: What about
[PATCH] Destroy resources when destroying input and output
Some structures containing resources list are freed before resources
on that list are destroyed, and that triggers invalid read/write
in compositor.c:3103 indirectly called from text-backend.c:938
when running weston under valgrind.
This patch destroys resources on these lists before such
structures are freed.
That fixes at least x11 and freerds backend.
Signed-off-by: Mariusz Ceier mceier+wayl...@gmail.com
---
src/compositor.c | 4
src/input.c | 15 ---
2 files changed, 16 insertions(+), 3 deletions(-)
diff --git a/src/compositor.c b/src/compositor.c
index ff0f3ab..a4077e8 100644
--- a/src/compositor.c
+++ b/src/compositor.c
@@ -3183,6 +3183,7 @@ weston_compositor_verify_pointers(struct
weston_compositor *ec)
WL_EXPORT void
weston_output_destroy(struct weston_output *output)
{
+ struct wl_resource *resource, *next_resource;
output-destroying = 1;
weston_compositor_remove_output(output-compositor, output);
@@ -3192,6 +3193,9 @@ weston_output_destroy(struct weston_output *output)
wl_signal_emit(output-destroy_signal, output);
+ wl_resource_for_each_safe(resource, next_resource,
output-resource_list)
+ wl_resource_destroy(resource);
+
free(output-name);
pixman_region32_fini(output-region);
pixman_region32_fini(output-previous_damage);
diff --git a/src/input.c b/src/input.c
index 07e9d6c..062a2cb 100644
--- a/src/input.c
+++ b/src/input.c
@@ -471,10 +471,13 @@ weston_pointer_create(struct weston_seat *seat)
WL_EXPORT void
weston_pointer_destroy(struct weston_pointer *pointer)
{
+ struct wl_resource *resource, *next_resource;
+
if (pointer-sprite)
pointer_unmap_sprite(pointer);
- /* XXX: What about pointer-resource_list? */
+ wl_resource_for_each_safe(resource, next_resource,
pointer-resource_list)
+ wl_resource_destroy(resource);
wl_list_remove(pointer-focus_resource_listener.link);
wl_list_remove(pointer-focus_view_listener.link);
@@ -520,7 +523,7 @@ weston_xkb_info_destroy(struct weston_xkb_info *xkb_info);
WL_EXPORT void
weston_keyboard_destroy(struct weston_keyboard *keyboard)
{
- /* XXX: What about keyboard-resource_list? */
+ struct wl_resource *resource, *next_resource;
#ifdef ENABLE_XKBCOMMON
if (keyboard-seat-compositor-use_xkbcommon) {
@@ -533,6 +536,9 @@ weston_keyboard_destroy(struct weston_keyboard *keyboard)
}
#endif
+ wl_resource_for_each_safe(resource, next_resource,
keyboard-resource_list)
+ wl_resource_destroy(resource);
+
wl_array_release(keyboard-keys);
wl_list_remove(keyboard-focus_resource_listener.link);
free(keyboard);
@@ -570,7 +576,10 @@ weston_touch_create(void)
WL_EXPORT void
weston_touch_destroy(struct weston_touch *touch)
{
- /* XXX: What about touch-resource_list? */
+ struct wl_resource *resource, *next_resource;
+
+ wl_resource_for_each_safe(resource, next_resource,
touch-resource_list)
+ wl_resource_destroy(resource);
wl_list_remove(touch-focus_view_listener.link);
wl_list_remove(touch-focus_resource_listener.link);
--
1.8.5.2
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/wayland-devel
