Re: Why do I require a `wl_event_loop_add_idle` in creating a fork-exec wayland client

[Markus Ongyerth]

On 2018/7月/11 09:42, Sichem Zhou wrote:
> Hi All,
> 
> I have a question related to the wl_client creation in the compositor. I
> followed these step like weston did:
> 
> 1. create socketpair.
> 2. fork a process and close `socket[0]` in child, close `socket[1]` in in
> parent process.
> 3. set the `WAYLAND_SOCKET` to `socket[1]`.
> 4. exec the program.

You seem to be missing a few steps here. They probably exist in your code, but 
it suggests to me that you might cause this somewhere else.
I have code [1] that doesn't rely on any idle dispatching and worked fine so 
far.

This includes a single step in the compositor:
3. Set up client with wl_display_create_client


Just lookking at my code, I see and remember that I'm using AF_UNIX and 
SOCK_STREAM arguments for the socketpair, which ones do you pass to yours?


[1] 
https://github.com/waymonad/waymonad/blob/master/src/Waymonad/Actions/Spawn.hs#L181

ongy

> 
> I found out if I do this directly both compositor and client will stuck in
> the eventloop. It seems the compositor is stuck in the
> `wl_display_flush_clients` and client is stuck in `wl_event_queue_dispatch`
> and internally stuck in the epoll.  So I think both compositor and client
> were waiting each other for some update.
> 
> Then I found out that  I need to pack the code in a
> `wl_event_loop_idle_func_t` and add it to the event_loop. But I am couldn't
> figure out why did the compositor stuck in the first place and why did the
> wl_event_loop_add_idle helped.
> 
> Thanks very much.
> SZ

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



signature.asc
Description: PGP signature
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH] server: add wl_signal_emit_safe

[Markus Ongyerth]

On 2018/July/13 05:40, Simon Ser wrote:
> This new function allows listeners to remove themselves or any
> other listener when called. This version only works if listeners
> are properly cleaned up when the wl_signal is free'd.
This is about free()ing the wl_listener in the actual handler, not the 
wl_signal, I think the wording here (and in the code-comment in the patch) is 
off.

Also I'd say that the commit message needs to be a bit longer. While "we" 
(anyone who participated in that earlier thread) know what this is about and 
why we need it, someone who looks at this patch and at wl_signal_emit which 
uses wl_list_for_each*_safe* will be confused bout what's going on.
Ah, I just saw it's in the comment of the function, I'd still move that to the 
doxygen comment, or commit message and out of the pure code comment
> 
> Signed-off-by: Simon Ser 
> ---
> This is a [1] follow-up. Since we noticed the previous version is
> not a drop-in replacement for wl_signal_emit, this patch just adds
> a new function instead.
> 
> [1]: https://patchwork.freedesktop.org/patch/204641/
> 
>  src/wayland-server-core.h |  3 ++
>  src/wayland-server.c  | 47 +
>  tests/signal-test.c   | 86 +++
>  3 files changed, 136 insertions(+)
> 
> diff --git a/src/wayland-server-core.h b/src/wayland-server-core.h
> index 2e725d9..4a2948a 100644
> --- a/src/wayland-server-core.h
> +++ b/src/wayland-server-core.h
> @@ -468,6 +468,9 @@ wl_signal_emit(struct wl_signal *signal, void *data)
>   l->notify(l, data);
>  }
>  
> +void
> +wl_signal_emit_safe(struct wl_signal *signal, void *data);
> +
>  typedef void (*wl_resource_destroy_func_t)(struct wl_resource *resource);
>  
>  /*
> diff --git a/src/wayland-server.c b/src/wayland-server.c
> index eae8d2e..a5f3735 100644
> --- a/src/wayland-server.c
> +++ b/src/wayland-server.c
> @@ -1932,6 +1932,53 @@ wl_client_for_each_resource(struct wl_client *client,
>   wl_map_for_each(>objects, resource_iterator_helper, );
>  }
>  
> +static void
> +handle_noop(struct wl_listener *listener, void *data) {
> + /* Do nothing */
> +}
> +
> +/** Emits this signal, safe against removal of any listener.
> + *
> + * \note This function can only be used if listeners are properly removed 
> when
> + *   the wl_signal is free'd.
> + *
> + * \param signal The signal object that will emit the signal
> + * \param data The data that will be emitted with the signal
> + *
> + * \sa wl_signal_emit()
> + *
> + * \memberof wl_signal
> + */
> +WL_EXPORT void
> +wl_signal_emit_safe(struct wl_signal *signal, void *data) {
> + struct wl_listener cursor;
> + struct wl_listener end;
Semi-serious: We could also add something like:
if (signal.listener_list.prev == signal.listener_list.next) {
wl_signal_emit(signal);
return;
}

And get rid of that wl_priv_signal, since that supported exactly this little 
corner-case (as I showed with the additional test a while ago) and not the 
general asumption that it's safe not to remove a destroy listener in general.
> +
> + /* Add two special markers: one cursor and one end marker. This way, we 
> know
> +  * that we've already called listeners on the left of the cursor and 
> that we
> +  * don't want to call listeners on the right of the end marker. The 'it'
> +  * function can remove any element it wants from the list without 
> troubles.
> +  * wl_list_for_each_safe tries to be safe but it fails: it works fine
> +  * if the current item is removed, but not if the next one is. */
> + wl_list_insert(>listener_list, );
> + cursor.notify = handle_noop;
> + wl_list_insert(signal->listener_list.prev, );
> + end.notify = handle_noop;
> +
> + while (cursor.link.next != ) {
> + struct wl_list *pos = cursor.link.next;
> + struct wl_listener *l = wl_container_of(pos, l, link);
> +
> + wl_list_remove();
> + wl_list_insert(pos, );
> +
> + l->notify(l, data);
> + }
> +
> + wl_list_remove();
> + wl_list_remove();
> +}
> +
>  /** \cond INTERNAL */
>  
>  /** Initialize a wl_priv_signal object
> diff --git a/tests/signal-test.c b/tests/signal-test.c
> index 7bbaa9f..dc762a4 100644
> --- a/tests/signal-test.c
> +++ b/tests/signal-test.c
> @@ -115,3 +115,89 @@ TEST(signal_emit_to_more_listeners)
>  
>   assert(3 * counter == count);
>  }
> +
> +struct signal
> +{
> + struct wl_signal signal;
> + struct wl_listener l1, l2, l3;
> + int count;
> + struct wl_listener *current;
> +};
> +
> +static void notify_remove(struct wl_listener *l, void *data)
> +{
> + struct signal *sig = data;
> + wl_list_remove(>current->link);
> + wl_list_init(>current->link);
> + sig->count++;
> +}
> +
> +#define INIT \
> + wl_signal_init(); \
> + wl_list_init(); \
> + wl_list_init(); \
> + wl_list_init();
> +#define 

Re: [RFC wayland-protocols] unstable: add protocol to give focus to a foreign surface

[Markus Ongyerth]

On 2018/7月/05 04:26, David Edmundson wrote:
> This protocol is to address the following use case.
> 
> A user clicks on a URL link in an IRC chat and a web browser opens. We want
> an existing browser window to raise or if it's a newly spawned application
> to claim focus on startup.
> 
> Naturally we also don't want any arbitrary client to be able to raise
> themselves arbitrarily.
Since this steals focus, I have to agree. But what we do want, is a way for 
clients to gain attention.
Probably via some kind of urgency flag on their toplevel.
I think raising a window (or for virtual-desktops, changing the desktop) is 
the extreme case of this, so I think it would fit a general urgency/attention 
grabbing protocol.
As I explain further down (I wrote this mail bottom up) I think we should be 
able to provide a way to allow the focus stealing, so we could have this in a 
protocl with a request family (or enum on one request?) for:
Attention: e.g. console bell, or some password query for a background task
Urgent: Something went wrong in e.g. gparted long-running process
Raise/Show: e.g. browser got opened a new tab for another application

> 
> The protocol is a combination of xdg-foreign and some very simplified
> aspects of X's startup-notification.
> 
> In the example above, the chat client gets a token from the compositor and
> passes it to the browser using:
>   - an environment variable for newly spawned apps
>   - platofrm-hints in the org.freedesktop.Application interface
>   - some hint in the relevant xdg-portal interface
>   - any custom mechanism
There was a discussion about this very recently in the suggestion for session 
recovery, it's not quite this easy.
And I think a quite common way to start applications (if we don't want to 
limit this to http[s]) is xdg-open, which I'm not sure if it provides for this 
use case.

As Drew pointed out in his answer already, the out  of band mechanism will not 
be the easiest thing to define. And when you read the previous discussion, 
you'll notice that the consense on which IPC to use isn't all that strong =.=

> 
> The browser then requests a raise of a top-level using this token.
> 
> (Exact common mechanisms should probably be defined, but maybe not in this
> document)
> 
> The exporter is virtually identical to xdg-foreign's exporter, but we can't
> re-use directly it as it's important that clients know what the exported
> handle could be used for.
> 
> Extending xdg-foreign would be a viable solution, but the lifespan of
> handles needs to be different..
> 
> Honouring the activation request is entirely up to the compositor which can
> have whatever advanced focus rules it wants.
> 
> Draft proposal below:
I feel like this is a bit too specific and complicated for what's supposed to 
be achieved here.

IMO we'd have a better solution (and more flexible to build on in the future) 
if this was split into two protocols, one for raising a window, and the other 
for passing serials.

The former protocol would simply have a request to be raised, that takes a 
valid serial. This could also be used for applications with multiple windows 
if they want to have another of their windows focused (e.g. fancy editor when 
you try to open a file again).

Then provide a secondary protocol that "hooks" into this one, by sharing 
serials.
This would be a simpler protocol without a direct use, but could be used as 
base to share (implicit) permissions gated on user interaction between clients 
that intend to outsource some task.
This protocol could largely look like xdg-foreign (maybe have the exporter one 
be destroyed implicit immediatly after the event) since serials have their own 
lifetime.
> 
> ---
> 
> 
> 
> 
>   
> 
>   
> 
>   
>   This protocol allows a top level to give up focus and pass it to a
> specific other client.
> 
>   An example use case would be a chat client opening a link in a
> browser, that should raise an existing window
>   or start the newly opened window with focus.
> 
>   The application releasing focus (focus_exported) fetches a handle
> from the compositor and then passes it to another client using a third
> party mechanism.
> 
>   The receiving application can use this handle to request focus from
> the compositor.
>   
> 
>   
> 
>   A global interface used for exporting surfaces that can later be
> imported
>   using xdg_importer.
I think you meant to:
s/xdg_importer/xdg_focus_activator/
> 
> 
> 
>   
> Notify the compositor that the xdg_exporter object will no longer be
> used.
>   
> 
> 
> 
>   
>   It is an error to export a focus that is not mapped.
>   
>   summary="the new xdg_exported object"/>
>   summary="the surface to export"/>
> 
>   
> 
>   
> 
> 
> 
> 
>   
>   Inform the compositor that this focus_exported object is no
> longer used.
>   The handle remains valid 

Re: [PATCH] libwayland: Add WAYLAND_DEBUG_INTERFACES

[Markus Ongyerth]

Urgh, of course I forget to mention something when I write mails.

This is current RFC quality, not actual patch. Should this be something people 
are interested in, I think it should live mostly in connection.c and be 
supported by both -server and -client libraries.

I'm also not sure whether the supplied whitelist approach is the best way to 
go, or this should be a blacklist in practice.

On 2018/6月/29 10:43, w...@ongy.net wrote:
> From: Markus Ongyerth 
> 
> Add environment variable WAYLAND_DEBUG_INTERFACES for filtering the
> output of WAYLAND_DEBUG logs.
> While WAYLAND_DEBUG is a pretty powerful and useful debug tool, printing
> everything has a few downsides.
> 
> 1) It's a full keylogger (getting debug-logs from users)
> 2) It can be overly spammy with wl_buffer/wl_surface actions (e.g. when
>playing a video))
> 
> With this addition it's possible to supply another environment
> variable, to filter on the interfaces one is interested in.
> E.g. when interested in the behaviour of xdg-shell popups the filter could be
> WAYLAND_DEBUG_INTERFACES=xdg_positioner,xdg_surface,xdg_popup
> greatly improving SNR on the output and hiding potentially sensitive
> information such as keystrokes.
> ---
>  src/wayland-client.c | 85 +---
>  1 file changed, 81 insertions(+), 4 deletions(-)
> 
> diff --git a/src/wayland-client.c b/src/wayland-client.c
> index efeb745..9de8e4e 100644
> --- a/src/wayland-client.c
> +++ b/src/wayland-client.c
> @@ -118,6 +118,31 @@ struct wl_display {
>  /** \endcond */
>  
>  static int debug_client = 0;
> +static size_t debug_client_count = 0;
> +static char **debug_client_interfaces = NULL;
> +
> +static void
> +wl_client_debug_print(struct wl_closure *closure,
> +  struct wl_object *target, int send)
> +{
> + if (debug_client_interfaces) {
> + bool found = false;
> + size_t i;
> + for (i = 0; i < debug_client_count; ++i) {
> + if (!strcmp(target->interface->name,
> + debug_client_interfaces[i])) {
> + found = true;
> + break;
> + }
> + }
> +
> + if (!found) {
> + return;
> + }
> + }
> +
> + wl_closure_print(closure, target, send);
> +}
>  
>  /**
>   * This helper function wakes up all threads that are
> @@ -748,7 +773,7 @@ wl_proxy_marshal_array_constructor_versioned(struct 
> wl_proxy *proxy,
>   wl_abort("Error marshalling request: %s\n", strerror(errno));
>  
>   if (debug_client)
> - wl_closure_print(closure, >object, true);
> + wl_client_debug_print(closure, >object, true);
>  
>   if (wl_closure_send(closure, proxy->display->connection))
>   wl_abort("Error sending request: %s\n", strerror(errno));
> @@ -1016,6 +1041,56 @@ connect_to_socket(const char *name)
>   return fd;
>  }
>  
> +/* Set up the filter list for WAYLAND_DEBUG output.
> + * This reads WAYLAND_DEBUG_INTERFACEs and splits the provided string on 
> every
> + * ','.
> + * The resulting list of strings is used as whitelist for interfaces printed 
> by
> + * the WAYLAND_DEBUG mechanism.
> + * Sadly this leaks the memory required to strdup() the environment variable,
> + * but since this is a debug feature, and it should be constant over the
> + * lifetime of a single process, I consider it a minor problem
> + */
> +static void
> +setup_interface_filter(void)
> +{
> + char *saveptr;
> + char *token;
> + size_t count = 0;
> + size_t i;
> + char *env;
> +
> + /* We set this up before (on another wl_display_connect probably) so we
> +  * don't have to do anything this time
> +  */
> + if (debug_client_interfaces)
> + return;
> +
> + if (!(env = getenv("WAYLAND_DEBUG_INTERFACES")))
> + return;
> +
> + if (!(env = strdup(env))) {
> + wl_log("error: Could not allocate memory for 
> WAYLAND_DEBUG_INTERFACES\n");
> + return;
> + }
> +
> + for (i = 0; env[i]; ++i) {
> + if (env[i] == ',')
> + ++count;
> + }
> +
> + /* The maximum possible interfaces is the number of ',' found + 1 */
> + debug_client_interfaces = calloc(count + 1, sizeof(char *));
> + if (!debug_client_interfaces) {
> + wl_log("error: Could not allocate memory for 
> WAYLAND_DEBUG_INTERFACES\n");
> + free(env);
> 

Re: client: remove definition of wl_global

[Markus Ongyerth]

Hi,

I once again seem to be missing the original message for some reason, so if I 
failed to properly fix up my headers again, I'm refering to [1]

Reviewed-By: Markus Ongyerth 

[1] https://patchwork.freedesktop.org/patch/204915/

Cheers,
ongy


signature.asc
Description: PGP signature
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH v3 wayland-protocols] virtual-keyboard: Add new virtual keyboard protocol

[Markus Ongyerth]

On 2018/6月/22 08:00, Dorota Czaplejewicz wrote:
> On Fri, 22 Jun 2018 12:36:16 -0400
> Simon Ser  wrote:
> 
> > On June 22, 2018 4:20 PM, Dorota Czaplejewicz  
> > wrote:
> > > Provides the ability to emulate keyboards by applications. Complementary 
> > > to input-method protocol.
> > >
> > > The interface is a mirror copy of wl_keyboard, with removed serials, and 
> > > added seat binding.
> > > ---
> > > Hi,
> > >
> > > this patch is another improvement to the previously sent virtual keyboard 
> > > protocol. Changes compared to v2:
> > >
> > > - readded enum names
> > > - removed suggestion to listen to modifiers
> > > - clarified untrusted client behaviour
> > > - added destructor on virtual_keyboard_manager
> > > - fixed text wrapping
> > >
> > > First off, the new version of wayland-scanner looks up enums found in 
> > > other xml files, so the textual reference are gone and actual enum values 
> > > are used.
> > >
> > > Secondly, there was a suggestion that if the client wants to know global 
> > > modifiers, it should listen to wl_keyboard.modifiers. In reality, this 
> > > can only be done when the client has keyboard focus. I decided to remove 
> > > this suggestion.
> > >
> > > Clarified that create_virtual_keyboard must error out with unauthorized 
> > > when an untrusted client connects. That doesn't preclude making the whole 
> > > virtual_keyboard_manager interface unavailable when the client is 
> > > determined as untrusted ahead of time.
> > >
> > > Lastly, added a missing destructor as per Simon Ser's suggestion.
> > >
> > > I hope that we're getting closer to perfect with this revision! As usual, 
> > > feedback is welcome.
> > >
> > > Cheers,
> > > Dorota Czaplejewicz  
> > 
> > Hi,
> > 
> > Thanks for this updated version. A few comments below.
> > 
>  snip 
> > > +
> > > +  
> > > +
> > > +  A virtual keyboard manager allows an application to provide 
> > > keyboard
> > > +  input events as if they came from a physical keyboard.
> > > +
> > > +  If the compositor enables a keyboard to perform arbitrary actions, 
> > > it
> > > +  should prevent untrusted clients from using this interface.
> > > +
> > > +
> > > +
> > > +   > > +summary="client not authorized to use the interface"/>
> > > +  
> > 
> > This is more of a general metaphorical question than anything else: I 
> > wonder how
> > we should handle unauthorized clients, and if adding an error for them is a 
> > good
> > idea. Generally errors are meant for protocol violations: it's clear from 
> > the
> > protocol spec that e.g. sending a request with a negative value will 
> > trigger a
> > protocol error. Also, errors are unrecoverable, they abort the whole client
> > (though this is being worked on).
> > 
> > So here we use an error, and the conditions in which the error is sent are
> > implementation-defined. I wonder if there's a better way to handle this
> > situation?
> > 
> Speaking from a position of someone without a lot of Wayland experience: are 
> Wayland errors meant to be recoverable by a client? It's obvious that if an 
> application doing its primary task and then trying to use a restricted 
> protocol as a secondary action crashes, that's undesireable.
> 
> As a more concrete example, an automation application may use e.g. DBus API 
> to automate tasks, and display a window to control it. It may request a 
> virtual keyboard to extend its possibilities, but it shouldn't suddenly stop 
> working if it's refused by the compositor.
Currently the errors will result in an abort() by libwayland. There is a patch 
series that changes this, but it will still be fatal on the wayland connection 
as far as I'm aware. Which isn't quite as bad, but can still be a problem, if 
an application uses the same wayland connection for more than one feature.
> 
> That brings me to the question: should applications using restricted 
> protocols create additional connections which may be broken and recovered 
> from individually or should they rather use one connection?
I'd personally prefer single fat connection. Mostly because it makes some 
bookkeeping easier, and some of the things I want to try out from the 
compositor side work better this way. But it's more robust (or rather will be) 
to have multiple, especially if one feature is expected to result in errors 
for some reason (the only expected errors should be design mistakes IMO, but 
this might still happen)

> If the latter is required for some use cases, then authorization and 
> probing/graceful rejection should be specified inside protocols.  
> Unfortunately, I'm not sure where to look for examples. Perhaps chat 
> applications where screen sharing may be decided ad hoc check the marks?
This is a good usecase for defaulting to query the user IMO. The client would 
try to access the interface at that time (either bind late, or have the first 
real request) at which point the compositor shows the user a query window 

Re: Session suspension and restoration protocol

[Markus Ongyerth]

On 2018/6月/19 01:39, Simon McVittie wrote:
> On Tue, 19 Jun 2018 at 13:56:22 +0200, Markus Ongyerth wrote:
> > P.S. I just thought about this ab it more, and something else came to my 
> > mind:
> > How is env passed with dbus activation? afaik the session bus does not run 
> > in 
> > under the compositor, so if we aren't on wayland-0 (which can happen very 
> > well 
> > in dev situations, and multi seats, and ...) we'd have to tell the client 
> > as 
> > well.
> 
> The session bus can run either inside or outside the compositor. It
> depends how you have designed your OS, and what you consider "the session"
> to mean. Desktop infrastructure can tell the dbus-daemon what environment
> variables it should pass to its child processes (but there can only be
> one answer at a time).
> 
> In the terminology of
> https://lists.freedesktop.org/archives/dbus/2015-January/016522.html
> a "session = login session" world would have one of these:
> 
> user session for uid 1000 (owns XDG_RUNTIME_DIR)
> \- login session c1
> \- compositor
> \- compositor-launched app
> \- session bus
> \- D-Bus-activated app
> 
> or
> 
> user session for for uid 1000 (owns XDG_RUNTIME_DIR)
> \- login session c1
> \- session bus
> \- D-Bus-activated app
> \- compositor
> \- compositor-launched app
> 
> (in the latter case you'd probably want to use
> UpdateActivationEnvironment() to send the WAYLAND_DISPLAY to the session
> bus)
> 
> while a "session = user session" world would have this:
> 
> user session for for uid 1000 (owns XDG_RUNTIME_DIR)
> \- systemd --user (optional)
> \- systemd-launched app (possibly done via D-Bus)
> \- session bus
> \- traditionally D-Bus-activated app
> \- login session c1
> \- compositor
> \- compositor-launched app
> 
> (again, you could use UpdateActivationEnvironment() to send the
> WAYLAND_DISPLAY to the session bus if necessary)
Sounds racy if there's more than one session on the same bus.
Locking ourselves into a single-compositor setup is quite annoying. And IMO we 
shouldn't rely on changes that affect the entire system setup.
It would at the very least increase the hurdle to get new devs, maybe even 
deter existing devs from touching any feature there.
> 
> The Activate() method defined by the Desktop Entry Specification passes
> a key:value map of arbitrary "platform data" to the activated process.
> It would be reasonable to include something like
> {'wayland_display':<'wayland-1'>}, meaning "use this Wayland display if
> possible, overriding the environment". (However, if the app only supports
> connecting to one Wayland display at a time, you're still stuck.)
I wonder if it's that much better to define wayland specific variables on that 
end, than it is to specify dbus specific things on this end.
In the optimal case they would be largely independant.
> 
> > Thinking about clients that may have multiple instances, but not shared 
> > sessions, e.g. some IDE with two projects opened.
> > iiuc the activation would start one process, which could then either 
> > restore, 
> > or open as normal, but when another project is openend, the process is 
> > already 
> > running and has to start another instance.
> > Over just having a single point of entry where it's started with 
> > restoration, 
> > or without.
> 
> Clients with multiple instances (an IDE with a process per project) are a
> better fit for a fork-and-exec design, yes.
> 
> Conversely, clients with a single instance (an IDE with one big process
> that can host multiple projects each in their own top-level window,
> like emacs with emacsclient, or GNOME Builder) are a better fit for an
> instance-as-a-service design.
Those have their own private IPC, and as far as I have seen, every application 
supports launching it and passing the arguments over an internal channel.  
Which might be configured to not do this, or evaluate over some other env 
(e.g. WAYLAND_DISPLAY) if it should do that.
> Clients with a single top-level process
> that manages a hierarchy of internal "helper" subprocesses, like web
> browsers, are probably also a better fit for the design.
There's also an issue with the protocol design here. We can only have one 
restoration id per wayland client. So this needs to be changed for those 
either way.
> 
> smcv
> ___
> wayland-devel mailing list
> wayland-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/wayland-devel


signature.asc
Description: PGP signature
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: Session suspension and restoration protocol

[Markus Ongyerth]

On 2018/6月/19 01:22, Simon McVittie wrote:
> On Tue, 19 Jun 2018 at 11:18:17 +0200, Markus Ongyerth wrote:
> > On 2018/6月/18 05:05, Roman Gilg wrote:
> > > * using D-Bus interface only to secure against sandboxed clients
> > What? Why exactly? When I first read this, I expected that the client is 
> > supposed to use the portal stuff to call out of a sandbox, but reading 
> > through 
> > the actual protocol, I think the only usage of D-Bus is to launch the 
> > applications from the compositor?
> 
> Any app implementing DBusActivatable (see the Desktop Entry Specification
Why would we arbitrarily limit this mechanism to this subset of applications?
This is a wayland extension protocol, and unless there's a good reason to 
limit it further, this should be avoided
> for more details) needs to be able to receive method calls from desktop
> infrastructure outside its sandbox anyway.
> For instance, Flatpak always
> allows method calls into the sandbox, while restricting method calls
> out of the sandbox according to app-specific rules.
> 
> > Since this needs explicit client support either way, IMO this could be 
> > implmented by adding a required cmdline argument (e.g.
> > --org-freedsktop-restore=UID) instead.
> 
> In desktop environments that support it, it's advantageous to launch apps
> as D-Bus services, or even as systemd user services if they support
> being launched like that (dbus-daemon will automatically delegate
> activation to systemd --user where supported), so that they always run
> in a reasonably consistent execution environment. If various different
> components (compositor, main menu if it's separate from the compositor,
> random app that wants to open a web page, etc.) launch an app using
> fork-and-exec with the Exec= command line from its .desktop file, then
> the app will inherit its execution environment (environment variables,
> file descriptors, rlimits, other process flags) from an arbitrary parent
> process, depending on the precise details of how they were launched. If
> they're DBusActivatable, they'll all be children of systemd --user (if
> supported) or dbus-daemon --session (if not), making their execution
> environment a lot more predictable.
Most of the compositor env makes sense for applications. Pushing open fds 
(outside std fds) into the child might be a compositor bug, or may be 
intended. E.g. for preconfigured wl_clients.
Some others make sense to prevent, like the rlimits, i agree. But the 
compositor isn't some arbitrary client either.
> 
> This indirect launching also avoids doing a fork-and-exec in the
> compositor or session manager, which can be problematic under low-memory
> conditions: the kernel needs to allocate enough virtual memory space for
> a second copy of the parent process (even though the physical memory that
> backs it is going to be copy-on-write, and in fork-and-exec it only exists
> briefly and is mostly unmodified), and when a compositor has your GPU's
> address space mmapped, that's actually a significant practical problem.
> I think Endless have had trouble with this in GNOME Shell on 32-bit -
> there's easily enough address space to have the GPU mmapped once, but
> if GNOME Shell does a fork-and-exec, the child process briefly has the
> GPU mmapped for a second time, and there isn't always enough address
> space free to do that.
Yea, that's indeed a problem.
> 
> The DBusActivatable protocol doesn't support sending arbitrary
> command-line arguments to the launched app, because in general,
> command-line arguments are not meaningful for an app that is already
> running.
This is for restoration, and the activation is only used for exactly restoring 
the application after it's closed.
Sending this to an already running application is the weird case that only 
exists because of using dbus here.

> (It does support sending the URLs of files to be opened.)
> Assuming we want to avoid always having to fall back to fork-and-exec,
> which is undesirable for the reasons above, we'll need some way to
> pass the state to restore via D-Bus - either a new method call as Roman
> proposed here, or the platform_data argument to Activate() as I suggested.
> 
> It would be possible to have a D-Bus preferred path and a fork-and-exec
> fallback path that are defined to be equivalent, like the Desktop
> Entry Specification does now for ordinary launching and for "Desktop
> Actions". That would have to work via either a command-line argument or an
> environment variable, because in fork-and-exec world those are the only
> tools we have. However, the meanings (and even syntax) of command-line
> arguments are conceptually "owned" by the app author, not by some third
> party like freedesktop.org; and environm

Re: Session suspension and restoration protocol

[Markus Ongyerth]

On 2018/6月/19 11:18, Markus Ongyerth wrote:
> Hey Roman,
> 
> first a general remark:
> I don't see any mention of which state is supposed to be restored. The 
> workspace (virtual desktops), geometry (session-restore), just client-side 
> window (the embedded usecase)?
> Is this left implementation-defined intentionally? If so, I think it would be 
> nice to have a note about that.
> 
> While the sleep version of this has a way for a client ot overwrite and 
> restore itself, I'm missing that functionality on quit/shutdown.
> The current state is always compositor triggered on those. While it might 
> make 
> sense for e.g. multi-window applications or other fancy things to use a 
> restoration protocol (at least for window geometry) on their own startup.
> It *should* work as is, but I'd like to see it explicitly.
> 
> Some more remarks inline.
> 
> Cheers,
> ongy
> 
> 
> On 2018/6月/18 05:05, Roman Gilg wrote:
> > Hi all,
> > 
> > I have worked the past few days on a protocol for client session
> > suspension and restoration and I need to get some feedback if I'm
> > running in the right direction. I did get some input from Sway and
> > GNOME devs at the start on what they would want such a protocol to do
> > in general, and I did try to respect that when working on the
> > protocol. Main features of the protocol as pasted below are:
> > * provides an extensible object creation mechanism to let the
> > compositor announce support for a subset of three increasing levels of
> > suspension and let the client opt-in to some or all of these supported
> > suspension levels
> > * these three levels are:
> >   1. sleep: allows destroying surfaces and their later restoration
> >   2. quit: allows connection closing and restart of the client by the
> >  compositor via an implementation-independent D-Bus interface
> >   3. shutdown: client will get restored after a server shutdown in a
> >  new Wayland session via the same D-Bus interface.
> > * using D-Bus interface only to secure against sandboxed clients
> What? Why exactly? When I first read this, I expected that the client is 
> supposed to use the portal stuff to call out of a sandbox, but reading 
> through 
> the actual protocol, I think the only usage of D-Bus is to launch the 
> applications from the compositor?
> Since this needs explicit client support either way, IMO this could be 
> implmented by adding a required cmdline argument (e.g.  
> --org-freedsktop-restore=UID) instead.
> Parsing .desktop files, and launching applications in a helper shouldn't bee 
> too much of a burdon.
> I know that GNOME/KDE employ D-Bus either way, but others want to avoid it as 
> much as possible.
P.S. I just thought about this ab it more, and something else came to my mind:
How is env passed with dbus activation? afaik the session bus does not run in 
under the compositor, so if we aren't on wayland-0 (which can happen very well 
in dev situations, and multi seats, and ...) we'd have to tell the client as 
well.

A second concern here would be iplementation cost in the client.
Thinking about clients that may have multiple instances, but not shared 
sessions, e.g. some IDE with two projects opened.
iiuc the activation would start one process, which could then either restore, 
or open as normal, but when another project is openend, the process is already 
running and has to start another instance.
Over just having a single point of entry where it's started with restoration, 
or without.

> > * if the client opts-in to level 2, 3 or both, the compositor might
> >   try to also restore the client after a client or compositor crash
> > * the program flow would be always:
> >   1. compositor announces supported suspension levels
> >   2. client opts-in to one or several of the supported suspension
> >  levels by creating protocol objects for the respective levels
> >   3. when the compositor wants to suspend the client (for example on
> >  low memory or shutdown) it sends an event to the respective
> >  suspension object, which the client can acknowledge or it must
> >  destroy the object to cancel the suspension.
> >   So while client and server agree on a subset of usable suspension
> >   levels, in the end only the compositor activates a suspension.
> > 
> > The protocol is written from scratch, but it shares some similarities
> > with Mike's proposed xdg-session-management protocol
> > (https://lists.freedesktop.org/archives/wayland-devel/2018-February/037236.html).
> > 
> > In advance thank you for your feedback!
> > 
> > 
> > 
> > 
> >   
> > Copyright © 2018 Roman Gilg
&

Re: Session suspension and restoration protocol

[Markus Ongyerth]

Hey Roman,

first a general remark:
I don't see any mention of which state is supposed to be restored. The 
workspace (virtual desktops), geometry (session-restore), just client-side 
window (the embedded usecase)?
Is this left implementation-defined intentionally? If so, I think it would be 
nice to have a note about that.

While the sleep version of this has a way for a client ot overwrite and 
restore itself, I'm missing that functionality on quit/shutdown.
The current state is always compositor triggered on those. While it might make 
sense for e.g. multi-window applications or other fancy things to use a 
restoration protocol (at least for window geometry) on their own startup.
It *should* work as is, but I'd like to see it explicitly.

Some more remarks inline.

Cheers,
ongy


On 2018/6月/18 05:05, Roman Gilg wrote:
> Hi all,
> 
> I have worked the past few days on a protocol for client session
> suspension and restoration and I need to get some feedback if I'm
> running in the right direction. I did get some input from Sway and
> GNOME devs at the start on what they would want such a protocol to do
> in general, and I did try to respect that when working on the
> protocol. Main features of the protocol as pasted below are:
> * provides an extensible object creation mechanism to let the
> compositor announce support for a subset of three increasing levels of
> suspension and let the client opt-in to some or all of these supported
> suspension levels
> * these three levels are:
>   1. sleep: allows destroying surfaces and their later restoration
>   2. quit: allows connection closing and restart of the client by the
>  compositor via an implementation-independent D-Bus interface
>   3. shutdown: client will get restored after a server shutdown in a
>  new Wayland session via the same D-Bus interface.
> * using D-Bus interface only to secure against sandboxed clients
What? Why exactly? When I first read this, I expected that the client is 
supposed to use the portal stuff to call out of a sandbox, but reading through 
the actual protocol, I think the only usage of D-Bus is to launch the 
applications from the compositor?
Since this needs explicit client support either way, IMO this could be 
implmented by adding a required cmdline argument (e.g.  
--org-freedsktop-restore=UID) instead.
Parsing .desktop files, and launching applications in a helper shouldn't bee 
too much of a burdon.
I know that GNOME/KDE employ D-Bus either way, but others want to avoid it as 
much as possible.
> * if the client opts-in to level 2, 3 or both, the compositor might
>   try to also restore the client after a client or compositor crash
> * the program flow would be always:
>   1. compositor announces supported suspension levels
>   2. client opts-in to one or several of the supported suspension
>  levels by creating protocol objects for the respective levels
>   3. when the compositor wants to suspend the client (for example on
>  low memory or shutdown) it sends an event to the respective
>  suspension object, which the client can acknowledge or it must
>  destroy the object to cancel the suspension.
>   So while client and server agree on a subset of usable suspension
>   levels, in the end only the compositor activates a suspension.
> 
> The protocol is written from scratch, but it shares some similarities
> with Mike's proposed xdg-session-management protocol
> (https://lists.freedesktop.org/archives/wayland-devel/2018-February/037236.html).
> 
> In advance thank you for your feedback!
> 
> 
> 
> 
>   
> Copyright © 2018 Roman Gilg
> 
> Permission is hereby granted, free of charge, to any person obtaining a
> copy of this software and associated documentation files (the "Software"),
> to deal in the Software without restriction, including without limitation
> the rights to use, copy, modify, merge, publish, distribute, sublicense,
> and/or sell copies of the Software, and to permit persons to whom the
> Software is furnished to do so, subject to the following conditions:
> 
> The above copyright notice and this permission notice (including the next
> paragraph) shall be included in all copies or substantial portions of the
> Software.
> 
> THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
> DEALINGS IN THE SOFTWARE.
>   
>   
> A protocol for suspending and restoring clients. Multiple modes allow
> clients and server to match their common support for different levels of
> suspension from suspending and restoring only the 

Re: [PATCH] virtual-keyboard: Add new virtual keyboard protocol

[Markus Ongyerth]

On 2018/5月/18 07:23, roderick.colenbran...@sony.com wrote:
> Hi Dorota,
> 
> Thanks for sharing your proposal. Ourselves we have interest in this 
> kind of capability as well. Looking at our own use cases, I wonder if 
> this proposal goes far enough.
> 
> We are basically interested in the ability to inject keyboard, mouse, 
> touch (and gamepad). On regular X some of that worked through XTest 
> protocol. From my perspective any virtual keyboard proposal, would make 
> sense to be flexible enough to handle other input devices as well.
All of these devices are different enough, that they need a dedicated API in 
some way (keys/analog axis/relative movements). This could be implemented as 
different interfaces in the same protocol, or separate protocols.
IMO splitting it up will be nicer, so compositors and clients can update the 
versions they support indipendently, and we aren't forced to bump a version on 
virtual keyboards to add to virtual gamepads.

> 
> On the other hand some may bring up, why virtual device support should 
> be in Wayland as input devices can also be spoofed through uinput.
Doing this as a wayland protocol has the (IMO huge) advantage that it doesn't 
require root permissions, and can be well confined.
Adding an input device into a test session with uinput sounds like a hassle as 
well, since the user session will pick it up as well by default. As wayland 
protocol it can just be spawned into the correct session.

Cheers,
ongy
> 
> Thanks,
> 
> Roderick Colenbrander
> Sr Manager Hardware & Systems Engineering
> Sony Interactive Entertainment
> 
> On 05/17/2018 09:55 AM, Dorota Czaplejewicz wrote:
> > Provides the ability to emulate keyboards by applications. Complementary to 
> > input-method protocol.
> > 
> > The interface is a mirror copy of wl_keyboard, with removed serials, and 
> > added seat binding.
> > ---
> > 
> > This proposal is another one needed by Purism to support on screen 
> > keyboards on a phone screen.
> > 
> > Virtual-keyboard is a protocol to let applications send arbitrary keyboard 
> > events. The need for this protocol comes from the fact that the 
> > input-method protocol combines two separate input responsibilities. It 
> > currently deals both with text input and raw keyboard events. I hope to 
> > split input-method along this line into virtual-keyboard and the rest of 
> > input-method. I'm going to submit the updated input-method for review soon.
> > 
> > Applications should be able to control both interfaces at the same time. A 
> > screen keyboard supporting autocorrect (input-method) still wants to send 
> > arrow keys (vityual-keyboard) correctly. Because of this, both kinds of 
> > events at minimum must be sent to the same seat. I made the seat binding 
> > explicitly done by the application, taking inspiration from text-input 
> > protocol, which assumes per-seat binding as well.
> > 
> > Input welcome.
> > 
> > Cheers,
> > Dorota
> > 
> >   Makefile.am|  1 +
> >   .../virtual-keyboard-unstable-v1.xml   | 97 
> > ++
> >   2 files changed, 98 insertions(+)
> >   create mode 100644 
> > unstable/virtual-keyboard/virtual-keyboard-unstable-v1.xml
> > 
> > diff --git a/Makefile.am b/Makefile.am
> > index 4b9a901..51a47a2 100644
> > --- a/Makefile.am
> > +++ b/Makefile.am
> > @@ -17,6 +17,7 @@ unstable_protocols =  
> > \
> > 
> > unstable/keyboard-shortcuts-inhibit/keyboard-shortcuts-inhibit-unstable-v1.xml
> >  \
> > unstable/xdg-output/xdg-output-unstable-v1.xml  
> > \
> > unstable/input-timestamps/input-timestamps-unstable-v1.xml  \
> > +   nstable/virtual-keyboard/virtual-keyboard-unstable-v1.xml   \
> > $(NULL)
> >   
> >   stable_protocols =
> > \
> > diff --git a/unstable/virtual-keyboard/virtual-keyboard-unstable-v1.xml 
> > b/unstable/virtual-keyboard/virtual-keyboard-unstable-v1.xml
> > new file mode 100644
> > index 000..18130e2
> > --- /dev/null
> > +++ b/unstable/virtual-keyboard/virtual-keyboard-unstable-v1.xml
> > @@ -0,0 +1,97 @@
> > +
> > +
> > +  
> > +Copyright © 2008-2011  Kristian Høgsberg
> > +Copyright © 2010-2013  Intel Corporation
> > +Copyright © 2012-2013  Collabora, Ltd.
> > +Copyright © 2018   Purism SPC
> > +
> > +Permission is hereby granted, free of charge, to any person obtaining a
> > +copy of this software and associated documentation files (the 
> > "Software"),
> > +to deal in the Software without restriction, including without 
> > limitation
> > +the rights to use, copy, modify, merge, publish, distribute, 
> > sublicense,
> > +and/or sell copies of the Software, and to permit persons to whom the
> > +Software is furnished to do so, subject to the following conditions:
> > +
> > +The above copyright notice and this 

Re: libinput varlink implementation?

[Markus Ongyerth]

A quick glance at varlink makes me like it more than dbus, but I'm not sure 
it's the best choice to provide debug information about libinput configuration 
in compositors.

All compositors I'm aware of, provide an IPC method for some (more or less) 
internals. GNOME (and afaik KDE) have dbus, sway has the i3-ipc protocol, 
waymonad provides a FUSE filesystem.
They also all speak wayland (duh) and there may be more I'm not aware of 
(pipewire?).
Adding varlink would be, at least the 3rd generic IPC mechanism (after 
wayland/dbus/custom) that the compositors provide. IFF we can expect varlink 
to replace dbus for the environment I'd welcome it, but at the current time 
I'd rather not introduce another generic IPC library/mechanism.

I'd also prefer to have a generic configuration interface in the future, which 
IMO would be best to be based on a read-only introspection method.  And I 
think we got the best chances to provide decent UX for querying the user for 
permission if we use wayland extension protocols. 
Partially because we already have a need to secure them, and this could be 
plugged into that method, and because this provides a strong coupling of 
windows of a client to requests it makes (over the wl-client).
This would allow us to show a UAC like query that shows the client in question 
(via its windows) and what protocol is to be accessed.
If any other IPC method is used, we lose this strong, unique association.

For the reasons stated above, I think we would be better suited with an 
interface defined as wayland extension. The downside is, that it has to be 
proxied and implemented by the compositor, but I think the advantages outweigh 
this.



Never the less, a few pointers to the varlink approach from my side:
From your quick outline in the mail, I asume you plan to have the varlink fd 
libinput owned? I don't think that fits the current scope of libinput (or that 
it's a good idea). If it can be integrated into a general connection of the 
compositor, that would be better.

Another thing I'd like to see (maybe even more? honestly not sure) would be a 
mode to just take a stream/memory-buffer from the application and pass back 
memory-buffers to it for communication, this may require some support from the 
library, but it would allow compositors to integrate the functionality into 
whatever IPC they provide.
It would make the dumping tool a little compositor specific though, so it may 
be an anti-goal.

Cheers,
ongy

P.S. I'm aware that some people claim that the security issue can be solved 
with the portal/dbus/container combination.
a) I don't like the though of running everything in a container (yet)
b) We can apply the same (or IMO even better) filters over wayland sockets

On 2018/May/10 03:07, Peter Hutterer wrote:
> one of the issues we have with libinput in the wayland world is that
> debugging it is a bit harder than in X. There we can just run xinput
> list-props and get an idea of what settings are applied to each device which
> helps narrow down where the issue really is. Under Wayland, that is not
> available, for all the obvious reasons.
> 
> Last year Carlos and I had a chat about this and thought about maybe
> exposing the devices via DBus from mutter. This way we could at least have
> some generic tool dumping state for debugging. This was not intended as a
> configuration interface. I never got to write those patches, but I vaguely
> remember KDE having something like this already, Martin could probably
> commment more here.
> 
> Recently, varlink is a bit in the news (http://varlink.org) and it is simple
> enough. and tempting... The current (very early) thought is to have
> something in the form of:
> 
> libinput_varlink_init_service(libinput, "compositor-name")
> 
> which sets everything up as part of libinput's epoll. It then allows for
> this:
> 
> varlink call unix:@libinput-mutter.socket/org.freedesktop.libinput.Devices
> {
>   "devices": [
> "event2",
> "event5",
> "event0",
> "event1",
> [...]
>   ]
> }
> 
> Followed by more detailed calls.
> 
> The bit above works locally, it's around 100 LOC. Easy enough to extend
> with device information and configuration options. But there are many
> details to be sorted, amongs them the name of the socket (we may have
> multiple libinput instances running). It's obviously a backchannel around
> the compositor which is why I intend to only give it read-only access.
> 
> From libinput's POV this would allow for a generic "libinput inspect" tool
> (or something like that) to query the actual libinput context used.
> Again, not with write access, but to query which devices are available, what
> configuration options are set, etc. This would help for debugging but
> varlink is not something I would generally expose without the compositor's
> permission. Which means I need a 'yep' from at least some of you (and those
> I forgot to add to the CC :)
> 
> Thoughts? Obviously varlink is still in its very 

Re: [PATCH] Add layer-shell-unstable-v1.xml

[Markus Ongyerth]

Hi, butting in

On 2018/May/08 04:17, Daniel Stone wrote:
> 
> > [... details snipped ...]
> >
> > These ideas are still somewhat nebulous but I'm confident enough that
> > it'll work that I think we can proceed with the discussion on protocols
> > like layer-shell. Compositors which are interested in implementing these
> > protocols and securing them today can do something simple like
> > weston_client_launch as a temporary solution and make it more accessible
> > to third parties later.
> 
> I think they sound like a fine idea; is anyone working on implementing them?

If I remember correctly and you are talking about the spawn with filters, I 
have a basic implementation/PoC in waymonad. I haven't implemented it too far 
for accesibility yet, but it works reasonably well from my testing.
I also have plans to implement this for containers (based on additional 
sockets to accept on "manually"), but haven't gotten around to it.

Cheers,
ongy


signature.asc
Description: PGP signature
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH v2 1/2] weston-info: Add support for tablet-unstable-v2

[Markus Ongyerth]

Hi,

thanks for the feedback, I'll provide a v3 shortly.
I just have a few follow up questions, before I do so.


On 2018/April/27 04:55, Peter Hutterer wrote:
> On Thu, Apr 26, 2018 at 05:01:23PM +0200, w...@ongy.net wrote:
> > From: Markus Ongyerth <w...@ongy.net>
> >  
> > +struct tablet_v2_path {
> > +   struct wl_list link;
> > +   char *path;
> > +};
> > +
> > +struct tablet_tool_info {
> > +   struct wl_list link;
> > +   struct zwp_tablet_tool_v2 *tool;
> > +
> > +   uint64_t hardware_serial;
> > +   uint64_t hardware_id_wacom;
> > +   enum zwp_tablet_tool_v2_type type;
> > +   
> > +   int tilt;
> > +   int pressure;
> > +   int distance;
> > +   int rotation;
> > +   int slider;
> > +   int wheel;
> 
> can we make those bool? and ideally, rename them to has_tilt, has_pressure,
> etc.

The bool type is a C99 feature, I haven't tested it, but the file looks like 
it should be C90 compatible.
Looking at the Makefile.am/configure.ac I didn't find any -std= flag. What's 
the C standard weston (clients) should follow?

> > +static void
> > +destroy_tablet_pad_info(struct tablet_pad_info *info)
> > +{
> > +   struct tablet_v2_path *path;
> > +   struct tablet_v2_path *tmp_path;
> > +   struct tablet_pad_group_info *group;
> > +   struct tablet_pad_group_info *tmp_group;
> 
> nitpick: this is a case where collating the two lines would be acceptable,
> imo, i.e.  struct tablet_v2_path *path, *tmp_path;

I really dislike that syntax, and when in doubt I refer to kernel guidelines 
which forbids it. Unless there's a strong wish to change it, I'd prefer to 
keep it as is for style reasons.

> > +   if (wl_list_empty(>pads) &&
> > +   wl_list_empty(>tablets) &&
> > +   wl_list_empty(>tools)) {
> 
> confusing indentation, imo, line up with the first wl_list_empy

I'm not attached to the current indention style, but I'm not a fan of that 
alignment. Many (un/badly -configured) viewers display tabs as 4 spaces these 
days (I just notcied, so does my email client), which will align the content 
with the continuating clauses since `if (` is 4 chars.
While it's not a problem for proper working environments, it's something I'd 
like to avoid for times where code is read on the cgit (not sure how that's 
configured) or github.
I'll take a look at other files in the repository and how it's done there.


signature.asc
Description: PGP signature
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH v2 0/3] Deal with destroy signal use after free issues

[Markus Ongyerth]

Hi,

Thanks for getting to this. I was waiting for the release, but I'm currently 
not at full capacity, so you got it before me.

The commit message of patch 1 contains a lie. The second paragraph should 
contain "IF there was only one listener object", which the testcase in Patch 3 
shows. But I don't think that's a deal breaker.

For Patch 1/2:
Reviewed-by: Markus Ongyerth <w...@ongy.net>

I'm fine with moving/resubmit of my code and am happy I could provide the 
testcase that shows an issue.
Since it's originally authored by me, I guess my R-B would be weird there :)

Cheers,
ongy

On 2018/April/16 03:00, Derek Foreman wrote:
> Now that the release is out, I'd like to dig back into this mess.
> This is a round up of some patches that were on list shortly before
> the release to deal with a problem where many existing libwayland
> users don't delete their destroy signal listeners before freeing
> them.
> 
> These leads to a bit of a mess (as Markus' test illustrates) if there
> are multiple destroy listeners.
> 
> I've included:
> My test patch to ensure the existing behaviour continues to work
> (users like weston and enlightenment can free during destroy
> listener)
> 
> The special case destroy emit path for wl_priv_signal - this is
> an attempt to "fix" the problem, by making the destroy signal
> emit operate without ever touching potentially free()d elements
> again.
> 
> Markus' test that would fail without patch 2/3, as it catches the
> free() without removal case we've all come to know any love.
> 
> Derek Foreman (2):
>   tests: Test for use after free in resource destruction signals
>changes since first appearance: none
> 
>   server: Add special case destroy signal emitter
>changes since first appearance:  stop trying to maintain a list head
> 
> Markus Ongyerth (1):
>   tests: Add free-without-remove test
>changes since first appearance: I moved it into an existing file
> 
>  src/wayland-private.h  |  3 +++
>  src/wayland-server.c   | 46 +++---
>  tests/resources-test.c | 39 +++
>  3 files changed, 85 insertions(+), 3 deletions(-)
> 
> -- 
> 2.14.3
> 
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH wayland-protcols v3] unstable: add xdg-toplevel-decoration protocol

[Markus Ongyerth]

Sorry, I messed up my quoting, cutting down the mail =.=

I wanted to keep the part that ended in:

> Yes, but I think this reinforces my point. If an IVI, phone or
> set-top-box compositor suddenly started sticking decorations on the
> surfaces it found, it wouldn't be useful. Saying 'but the clients
> never asked _not_ to be decorated' wouldn't really help you get out of
> it either.
Which is in a specialised environment that usually wants no deco (probably 
fullscreen shell by default etc.) but may run xdg-shell for reasons.

On 2018/3月/18 03:40, Markus Ongyerth wrote:
> On 2018/3月/18 01:45, Daniel Stone wrote:
> > Hi Drew,
> > 
> > On 15 March 2018 at 16:53, Drew DeVault <s...@cmpwn.com> wrote:
> > >> > In fact, I have done so. Before we started working on this protocol,
> > >> > Sway did exactly this. We have provided users the means of overriding
> > >> > the approach to decorations, including what ends up being double
> > >> > decorations sometimes.
> > >>
> > >> OK, but that doesn't seem like the kind of user experience to aim for 
> > >> ... ?
> > >
> > > Yeah, it's not ideal. The impetus for this protocol is to solve this
> > > problem by getting software written by both camps to negotiate. It's
> > > clear we're not going to get either side to agree to buy into the other,
> > > so in the interests of the user we're trying to accomodate both.
> > 
> > I understand that, and the compromise is good. But my view here is
> > that compositors sticking server-side decorations on unwilling clients
> > are the ones upsetting the status quo. Your view seems to be that
> > there _is_ no status quo, so either approach is equally valid in the
> > absence of explicit negotiation.
> I think we have quite different ideas of what this situation would look like.
> The (specialised) compositor doing things that don't fit it's environment is 
> something I wouldn't expect.
> 
> If a set-top-box or phone wanted to run e.g. gvim it wouldn't want that to 
> draw it's own decorations, which it would do in the normal case.
> And if a specialised application was run on a "normal desktop" compositor 
> (for 
> whatever reason), the provided SSD would be the easiest way for it to support 
> this. (I kind of doubt they use xdg-shell for those though)
> 
> I'm interested in where this disparity in worries stems from.
> Do you go by the asumption that the compositor might change in these 
> settings, 
> while the clients stay specialised?
> 
> Cheers,
> ongy




signature.asc
Description: PGP signature
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH wayland-protcols v3] unstable: add xdg-toplevel-decoration protocol

[Markus Ongyerth]

On 2018/3月/18 01:45, Daniel Stone wrote:
> Hi Drew,
> 
> On 15 March 2018 at 16:53, Drew DeVault  wrote:
> >> > In fact, I have done so. Before we started working on this protocol,
> >> > Sway did exactly this. We have provided users the means of overriding
> >> > the approach to decorations, including what ends up being double
> >> > decorations sometimes.
> >>
> >> OK, but that doesn't seem like the kind of user experience to aim for ... ?
> >
> > Yeah, it's not ideal. The impetus for this protocol is to solve this
> > problem by getting software written by both camps to negotiate. It's
> > clear we're not going to get either side to agree to buy into the other,
> > so in the interests of the user we're trying to accomodate both.
> 
> I understand that, and the compromise is good. But my view here is
> that compositors sticking server-side decorations on unwilling clients
> are the ones upsetting the status quo. Your view seems to be that
> there _is_ no status quo, so either approach is equally valid in the
> absence of explicit negotiation.
I think we have quite different ideas of what this situation would look like.
The (specialised) compositor doing things that don't fit it's environment is 
something I wouldn't expect.

If a set-top-box or phone wanted to run e.g. gvim it wouldn't want that to 
draw it's own decorations, which it would do in the normal case.
And if a specialised application was run on a "normal desktop" compositor (for 
whatever reason), the provided SSD would be the easiest way for it to support 
this. (I kind of doubt they use xdg-shell for those though)

I'm interested in where this disparity in worries stems from.
Do you go by the asumption that the compositor might change in these settings, 
while the clients stay specialised?

Cheers,
ongy


signature.asc
Description: PGP signature
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH wayland-protcols v3] unstable: add xdg-toplevel-decoration protocol

[Markus Ongyerth]

On 2018/3月/18 10:50, Eike Hein wrote:
> 
> 
> On 03/16/2018 12:43 AM, Daniel Stone wrote:> No, it really has. GTK+ has
> always - well, until you got the patches
> > for this protocol merged a month or two ago - decorated its own
> > windows under Wayland. Same with Qt. Same with EFL. These are toolkits
> > which have been around and deployed for several years, doing exactly
> > what I'm describing. Sticking server-side decorations on them gives
> > you two completely different titlebars, and anyone trying to use it
> > justifiably thinking the result is an incoherent mess.
> 
> There's also millions of Wayland-based products in the market in which
> Wayland clients do not render decorations, e.g. in TVs and IVI systems.
> Grated, not many of them use much or any of the xdg-* protocols.
> 
> Still, compositors do not reject clients which don't render decorations.
> The core protocol as-yet doesn't make an attempt to define what a
> decoration is, or needs to have in it, on the basis of which it could do
> so, either. If it did, the above Wayland systems would suddenly be in
> violation. I think we agree these Wayland systems are actually fine and
> nice to have.
> 
> I'm personally fine with assuming that many or even all of the initial
> Wayland contributors assumed CSDs on desktop systems. But I think this
> debate over the historic record is obscuring the real needs of this
> review, which should be about making sure the protocol under review is
> durable, flexible and future-proof, and doesn't introduce unintended
> side-effects.
> 
> So here's the problem I see: This protocol introduces a definition of
> what a decoration is and can do (move, resize, change state). A server
> advertising is commits to supporting both, but when a client asks for
> SSDs, the server can respond with "use CSDs"
> 
> So we have two aspects to this:
> 
> a) People are concerned a protocol like this could open the floodgates
> to clients which don't implement CSDs at all and indiscriminately ask
> servers for SSDs whenever they can.
> 
> b) Yet this protocol also allows servers to force clients which prefer
> SSDs to force them into CSD mode, and in fact holds them to implementing
> CSDs with a number of features (which don't make sense in every system).
> 
> In my eyes (b) is pretty clearly more prohibitively costly.
This is an optional protocol that builds upon xdg-shell.
Which, as you said, should not be present on most of these devices.

If a client never wants any decoration, it can just not implement this.
Which should be enough for all Smart-TV apps and the like.

The only problematic case would be applications intended for a normal desktop 
workflow that ask for decor if possible, but never want to draw their own.

> 
> Servers which want to indiscriminately force all clients to provide CSDs
> can just not advertize this interface, which is essentially about
> letting clients know they have a preferred deco to show.
> 
> If they do advertise it, they should allow the client to express a
> preference, not abuse the spirit of the protocol - essentially to
> properly enable compositors which aim to establish a particular workflow
> via decorations (as many existing Wayland compositors do!) - to override
> it. Clients which stubbornly refuse to implement CSDs (e.g. mpv) are
> unlikely to keep up their end of the bargain anyway, but I think it's
> dangerous to run with such a vague definition of "decoration" with
> far-reaching implications.
> 
> S. Ser said "the CSD mode allows the client to do not draw decorations
> at all" in the discussion of v1, but the protocol text certainly doesn't
> agree here by defining what a deco is and requiring clients to support
> one. I agree with M. Blumenkrantz that the 'mode' thing seems wrong as
> it is now. I think the way to go would be:
> 
> a) Change the definition of "decoration" to "window controls as deemed
> appropriate by the compositor, for example ...". This leaves what's in a
> server-side deco and what's in a client-side deco up to servers and
> clients, respectively, and allows this protocol to serve more systems.
While a bit of rewording here (probably to the ambigous state it's in 
xdg-shell) wouldn't hurt, this particular sentence would make CSD very weird, 
since the client doesn't know what the compositor deems appropriate.
I think we should avoid the situation where clients try to guess their env, 
not promote it even further.

> 
> b) Make the protocol about negotiating between "please deco me as you
> see fit" and "I prefer to keep doing my own thing" (which can then still
> be "nothing") cases. I don't think any more is needed. I think client
> devs are smart enough to understand making this requests implies they
> should stop drawing their own title bars, and in the latter case they're
> smart enough to know how exactly they want to decorate their window, as
> e.g. the GTK+ designers clearly are, and this protocol shouldn't
> interfere with them. Certainly the 

Re: [PATCH] unstable: add xdg-session-management protocol

[Markus Ongyerth]

On 2018/2月/26 08:58, Mike Blumenkrantz wrote:
> for a variety of cases it's desirable to have a method for negotiating
> the restoration of previously-used states for a client's windows. this
> helps for e.g., a compositor/client crashing (definitely not due to
> bugs) or a backgrounded client deciding to temporarily destroy its
> surfaces in order to conserve resources

What's the intended workflow for the background client?
The protocol states that the session data should be removed on 
xdg_toplevel.destroy, which would not allow clients to reuse state after they 
gracefully exited, or "exited" partially (closing some windows, not the entire 
connection).

> 
> this protocol adds a method for managing such negotiation and is loosely
> based on the Enlightenment "session recovery" protocol which has been
> implemented and functional for roughly two years
> 
> some further reading: https://blogs.s-osg.org/recovery-journey-discovery/
> 
> Signed-off-by: Mike Blumenkrantz 
> Signed-off-by: Jonas Ådahl 
> ---
>  Makefile.am|   1 +
>  unstable/xdg-session-management/README |   4 +
>  .../xdg-session-management-unstable-v1.xml | 181 
> +
>  3 files changed, 186 insertions(+)
>  create mode 100644 unstable/xdg-session-management/README
>  create mode 100644 
> unstable/xdg-session-management/xdg-session-management-unstable-v1.xml
> 
> diff --git a/Makefile.am b/Makefile.am
> index 4b9a901..2b75114 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -17,6 +17,7 @@ unstable_protocols =
> \
>   
> unstable/keyboard-shortcuts-inhibit/keyboard-shortcuts-inhibit-unstable-v1.xml
>  \
>   unstable/xdg-output/xdg-output-unstable-v1.xml  
> \
>   unstable/input-timestamps/input-timestamps-unstable-v1.xml  \
> + unstable/xdg-session-management/xdg-session-management-unstable-v1.xml  
> \
>   $(NULL)
>  
>  stable_protocols =   
> \
> diff --git a/unstable/xdg-session-management/README 
> b/unstable/xdg-session-management/README
> new file mode 100644
> index 000..7bff401
> --- /dev/null
> +++ b/unstable/xdg-session-management/README
> @@ -0,0 +1,4 @@
> +xdg session management protocol
> +
> +Maintainers:
> +Mike Blumenkrantz 
> diff --git 
> a/unstable/xdg-session-management/xdg-session-management-unstable-v1.xml 
> b/unstable/xdg-session-management/xdg-session-management-unstable-v1.xml
> new file mode 100644
> index 000..0c36c16
> --- /dev/null
> +++ b/unstable/xdg-session-management/xdg-session-management-unstable-v1.xml
> @@ -0,0 +1,181 @@
> +
> +
> +  
> +Copyright 2018 Mike Blumenkrantz
> +Copyright 2018 Samsung Electronics Co., Ltd
> +Copyright 2018 Red Hat Inc.
> +
> +Permission is hereby granted, free of charge, to any person obtaining a
> +copy of this software and associated documentation files (the 
> "Software"),
> +to deal in the Software without restriction, including without limitation
> +the rights to use, copy, modify, merge, publish, distribute, sublicense,
> +and/or sell copies of the Software, and to permit persons to whom the
> +Software is furnished to do so, subject to the following conditions:
> +
> +The above copyright notice and this permission notice (including the next
> +paragraph) shall be included in all copies or substantial portions of the
> +Software.
> +
> +THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS 
> OR
> +IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> +FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> +THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR 
> OTHER
> +LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> +FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
> +DEALINGS IN THE SOFTWARE.
> +  
> +  
> +Warning! The protocol described in this file is experimental and backward
> +incompatible changes may be made. Backward compatible changes may be 
> added
> +together with the corresponding interface version bump. Backward
> +incompatible changes are done by bumping the version number in the 
> protocol
> +and interface names and resetting the interface version. Once the 
> protocol
> +is to be declared stable, the 'z' prefix and the version number in the
> +protocol and interface names are removed and the interface version 
> number is
> +reset.
> +  
> +  
> +
> +  The xdg_session_manager interface defines base requests for creating 
> and
> +  managing a session for an application. Sessions persist across 
> application
> +  and compositor restarts unless explicitly destroyed. A session is 
> created
> + 

Re: [PATCH 1/1] tests: Add free-without-remove test

[Markus Ongyerth]

On 2018/2月/23 08:53, Derek Foreman wrote:
> On 2018-02-23 07:44 AM, Markus Ongyerth wrote:
> > On 2018/2月/23 07:31, Derek Foreman wrote:
> > > On 2018-02-23 03:52 AM, w...@ongy.net wrote:
> > > > From: Markus Ongyerth <w...@ongy.net>
> > > > 
> > > > This is related to the discussion earlier on the mailing list and
> > > > demonstrates a problem with current wl_priv_signal_emit and wl_listener
> > > > that free themselves, but do not remove from the list.
> > > > 
> > > > This testcase asserts that the wl_list inside wl_listener is not written
> > > > to anymore after it got emitted.
> > > > ---
> > > >Makefile.am |  3 +++
> > > >tests/free-without-remove.c | 63 
> > > > +
> > > >2 files changed, 66 insertions(+)
> > > >create mode 100644 tests/free-without-remove.c
> > > > 
> > > > diff --git a/Makefile.am b/Makefile.am
> > > > index 322d6b8..c51f328 100644
> > > > --- a/Makefile.am
> > > > +++ b/Makefile.am
> > > > @@ -158,6 +158,7 @@ pkgconfig_DATA += egl/wayland-egl-backend.pc
> > > >built_test_programs =\
> > > > array-test  \
> > > > +   free-without-remove-test\
> > > > client-test \
> > > > display-test\
> > > > connection-test \
> > > > @@ -223,6 +224,8 @@ libtest_runner_la_LIBADD =  \
> > > >array_test_SOURCES = tests/array-test.c
> > > >array_test_LDADD = libtest-runner.la
> > > > +free_without_remove_test_SOURCES = tests/free-without-remove.c
> > > > +free_without_remove_test_LDADD = libtest-runner.la
> > > >client_test_SOURCES = tests/client-test.c
> > > >client_test_LDADD = libtest-runner.la
> > > >display_test_SOURCES = tests/display-test.c
> > > > diff --git a/tests/free-without-remove.c b/tests/free-without-remove.c
> > > > new file mode 100644
> > > > index 000..ff468d7
> > > > --- /dev/null
> > > > +++ b/tests/free-without-remove.c
> > > > @@ -0,0 +1,63 @@
> > > > +/*
> > > > + * Copyright © 2018 Markus Ongyerth
> > > > + *
> > > > + * Permission is hereby granted, free of charge, to any person 
> > > > obtaining
> > > > + * a copy of this software and associated documentation files (the
> > > > + * "Software"), to deal in the Software without restriction, including
> > > > + * without limitation the rights to use, copy, modify, merge, publish,
> > > > + * distribute, sublicense, and/or sell copies of the Software, and to
> > > > + * permit persons to whom the Software is furnished to do so, subject 
> > > > to
> > > > + * the following conditions:
> > > > + *
> > > > + * The above copyright notice and this permission notice (including the
> > > > + * next paragraph) shall be included in all copies or substantial
> > > > + * portions of the Software.
> > > > + *
> > > > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
> > > > + * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
> > > > + * MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
> > > > + * NONINFRINGEMENT.  IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS
> > > > + * BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN
> > > > + * ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN
> > > > + * CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
> > > > + * SOFTWARE.
> > > > + */
> > > > +
> > > > +#include 
> > > > +#include 
> > > > +#include 
> > > > +#include 
> > > > +#include 
> > > > +#include 
> > > > +#include 
> > > > +#include 
> > > > +#include 
> > > > +#include 
> > > > +
> > > > +#include "wayland-private.h"
> > > > +#include "wayland-server.h"
> > > > +#include "test-runner.h"
> > > > +
> > > > +static void
> > > > +display_destroy_notify(struct wl_li

Re: [PATCH 1/1] tests: Add free-without-remove test

[Markus Ongyerth]

On 2018/2月/23 07:31, Derek Foreman wrote:
> On 2018-02-23 03:52 AM, w...@ongy.net wrote:
> > From: Markus Ongyerth <w...@ongy.net>
> > 
> > This is related to the discussion earlier on the mailing list and
> > demonstrates a problem with current wl_priv_signal_emit and wl_listener
> > that free themselves, but do not remove from the list.
> > 
> > This testcase asserts that the wl_list inside wl_listener is not written
> > to anymore after it got emitted.
> > ---
> >   Makefile.am |  3 +++
> >   tests/free-without-remove.c | 63 
> > +
> >   2 files changed, 66 insertions(+)
> >   create mode 100644 tests/free-without-remove.c
> > 
> > diff --git a/Makefile.am b/Makefile.am
> > index 322d6b8..c51f328 100644
> > --- a/Makefile.am
> > +++ b/Makefile.am
> > @@ -158,6 +158,7 @@ pkgconfig_DATA += egl/wayland-egl-backend.pc
> >   built_test_programs = \
> > array-test  \
> > +   free-without-remove-test\
> > client-test \
> > display-test\
> > connection-test \
> > @@ -223,6 +224,8 @@ libtest_runner_la_LIBADD =  \
> >   array_test_SOURCES = tests/array-test.c
> >   array_test_LDADD = libtest-runner.la
> > +free_without_remove_test_SOURCES = tests/free-without-remove.c
> > +free_without_remove_test_LDADD = libtest-runner.la
> >   client_test_SOURCES = tests/client-test.c
> >   client_test_LDADD = libtest-runner.la
> >   display_test_SOURCES = tests/display-test.c
> > diff --git a/tests/free-without-remove.c b/tests/free-without-remove.c
> > new file mode 100644
> > index 000..ff468d7
> > --- /dev/null
> > +++ b/tests/free-without-remove.c
> > @@ -0,0 +1,63 @@
> > +/*
> > + * Copyright © 2018 Markus Ongyerth
> > + *
> > + * Permission is hereby granted, free of charge, to any person obtaining
> > + * a copy of this software and associated documentation files (the
> > + * "Software"), to deal in the Software without restriction, including
> > + * without limitation the rights to use, copy, modify, merge, publish,
> > + * distribute, sublicense, and/or sell copies of the Software, and to
> > + * permit persons to whom the Software is furnished to do so, subject to
> > + * the following conditions:
> > + *
> > + * The above copyright notice and this permission notice (including the
> > + * next paragraph) shall be included in all copies or substantial
> > + * portions of the Software.
> > + *
> > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
> > + * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
> > + * MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
> > + * NONINFRINGEMENT.  IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS
> > + * BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN
> > + * ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN
> > + * CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
> > + * SOFTWARE.
> > + */
> > +
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +
> > +#include "wayland-private.h"
> > +#include "wayland-server.h"
> > +#include "test-runner.h"
> > +
> > +static void
> > +display_destroy_notify(struct wl_listener *l, void *data)
> > +{
> > +   l->link.prev = l->link.next = NULL;
> > +}
> > +
> > +TEST(free_without_remove)
> > +{
> > +   struct wl_display *display;
> > +   struct wl_listener a, b;
> > +
> > +   display = wl_display_create();
> > +   a.notify = display_destroy_notify;
> > +   b.notify = display_destroy_notify;
> > +
> > +   wl_display_add_destroy_listener(display, );
> > +   wl_display_add_destroy_listener(display, );
> > +
> > +   wl_display_destroy(display);
> > +
> > +   assert(a.link.next == a.link.prev && a.link.next == NULL);
> > +   assert(b.link.next == b.link.prev && b.link.next == NULL);
> 
> I feel this would be slightly more clear if it tested
> a.link.next == NULL && a.link.prev == NULL
Possibly.
> 
> As my previously submit test did... is this significantly more rigorous than
> my test here: https

Re: [RFC wayland] server: Add special case destroy signal emitter

[Markus Ongyerth]

Hi, I have a v2 RFC _emit_final based on your idea.
It passes `make check` of libwayland and weston.
It also passes the remove-without-free test I sent in another mail.

---

(This patch isn't quite intended to be merged, but to get feedback on
the approach)

This adds a wl_priv_signal_emit_final and a wl_signal_emit_final.
The `_final` emit functions have the additional asumption that every
listener currently in the list will be removed after the _emit. Either
gracefully, or simply free()d under the asumption that a destroy signal
will not be emitted more than once.

These functions take advantage of this and allow to use both tyles in
the same signal list.
It obsoletes the wl_priv_signal for destroy signals.

Non destroy wl_priv_signals currently have a stronger guarantee than
normal signals, which forces us to either keep the type around, or use
this as destroy signal path, and another safe version for other signals.
---
 src/wayland-private.h |  5 +
 src/wayland-server.c  | 50 +++---
 2 files changed, 52 insertions(+), 3 deletions(-)

diff --git a/src/wayland-private.h b/src/wayland-private.h
index 12b9032..76adb32 100644
--- a/src/wayland-private.h
+++ b/src/wayland-private.h
@@ -250,6 +250,11 @@ wl_priv_signal_add(struct wl_priv_signal *signal, struct 
wl_listener *listener);
 struct wl_listener *
 wl_priv_signal_get(struct wl_priv_signal *signal, wl_notify_func_t notify);
 
+void
+wl_signal_emit_final(struct wl_list *signal, void *data);
+
+void
+wl_priv_signal_emit_final(struct wl_priv_signal *signal, void *data);
 void
 wl_priv_signal_emit(struct wl_priv_signal *signal, void *data);
 
diff --git a/src/wayland-server.c b/src/wayland-server.c
index eb1e500..62fc71a 100644
--- a/src/wayland-server.c
+++ b/src/wayland-server.c
@@ -682,7 +682,7 @@ destroy_resource(void *element, void *data, uint32_t flags)
/* Don't emit the new signal for deprecated resources, as that would
 * access memory outside the bounds of the deprecated struct */
if (!resource_is_deprecated(resource))
-   wl_priv_signal_emit(>destroy_signal, resource);
+   wl_priv_signal_emit_final(>destroy_signal, resource);
 
if (resource->destroy)
resource->destroy(resource);
@@ -841,7 +841,7 @@ wl_client_destroy(struct wl_client *client)
 {
uint32_t serial = 0;
 
-   wl_priv_signal_emit(>destroy_signal, client);
+   wl_priv_signal_emit_final(>destroy_signal, client);
 
wl_client_flush(client);
wl_map_for_each(>objects, destroy_resource, );
@@ -1089,7 +1089,7 @@ wl_display_destroy(struct wl_display *display)
struct wl_socket *s, *next;
struct wl_global *global, *gnext;
 
-   wl_priv_signal_emit(>destroy_signal, display);
+   wl_priv_signal_emit_final(>destroy_signal, display);
 
wl_list_for_each_safe(s, next, >socket_list, link) {
wl_socket_destroy(s);
@@ -1992,6 +1992,50 @@ wl_priv_signal_get(struct wl_priv_signal *signal, 
wl_notify_func_t notify)
return NULL;
 }
 
+/**
+ * This emits a signal under the asumpton that every listener in it will be
+ * removed from the list in some way.
+ * 
+ * It supports listeners that remove themselves without issues.
+ *
+ * The situation where a listener is only free()d, but not removed from the
+ * list is also supported, to not break existing library users.
+ *
+ * The notify function can remove arbitrary elements from the wl_signal, and
+ * the wl_signal will be in a valid state at all times (s.t. later elements in
+ * the list not beeing free()d without removal, but that broke at any time).
+ *
+ * The idea is based on wl_priv_signal_emit, we iterate until a list is empty.
+ * In this case we don't need the emit_list, since we know our actual signal
+ * list will be empty in the end.
+ */
+void
+wl_signal_emit_final(struct wl_list *signal, void *data)
+{
+   struct wl_listener *l;
+   struct wl_list *pos;
+
+   while (!wl_list_empty(signal)) {
+   pos = signal->next;
+   
+   /* Remove and ensure validity of position element */
+   wl_list_remove(pos);
+   wl_list_init(pos);
+
+   l = wl_container_of(pos, l, link);
+   l->notify(l, data);
+   }
+}
+
+/**
+ * see wl_signal_emit_final for more details
+ */
+void
+wl_priv_signal_emit_final(struct wl_priv_signal *signal, void *data)
+{
+   wl_signal_emit_final(>listener_list, data);
+}
+
 /** Emit the signal, calling all the installed listeners
  *
  * Iterate over all the listeners added to this \a signal and call
-- 
2.16.2



On 2018/2月/22 04:02, Derek Foreman wrote:
> In the past much code (weston, efl/enlightenment, mutter) has
> freed structures containing wl_listeners from destroy handlers
> without first removing the listener from the signal.  As the
> destroy notifier only fires once, this has largely gone
> unnoticed until recently.
> 
> 

Re: [PATH] core: implement a safe wl_signal_emit

[Markus Ongyerth]

On 2018/2月/22 12:31, Derek Foreman wrote:
> On 2018-02-22 10:48 AM, Markus Ongyerth wrote:
> > On 2018/2月/22 09:34, Derek Foreman wrote:
> > > On 2018-02-22 08:58 AM, Daniel Stone wrote:
> > > > Hi,
> > > > 
> > > > On 22 February 2018 at 14:14, Markus Ongyerth <w...@ongy.net> wrote:
> > > > > > It seems that this patch makes that assumption invalid, and we would
> > > > > > need patches to weston, enlightenment, and mutter to prevent a
> > > > > > use-after-free during the signal emit?  Now I'm seeing valgrind 
> > > > > > errors
> > > > > > on E and weston during buffer destroy.
> > > > > > 
> > > > > > Personally, I don't think we should change this assumption and 
> > > > > > declare
> > > > > > the existing code that's worked for years suddenly buggy. :/
> > > > > 
> > > > > The code was buggy the whole time. Just because it was never 
> > > > > triggered, does
> > > > > not imply it's not a bug.
> > > > > free()ing these struct wl_list without removing them from the signal 
> > > > > list
> > > > > leaves other struct wl_list that are outside the control of the 
> > > > > current code
> > > > > in an invalid, prone to use-after-free, state.
> > > > 
> > > > There's a difference between something being 'buggy' and a design with
> > > > non-obvious details you might not like. If destroy handlers not
> > > > removing their list elements were buggy, we would be seeing bugs from
> > > > that. But instead it's part of the API contract: when a destroy signal
> > > > is invoked, you are guaranteed that this will be the first and only
> > > > access to your list member. This implies that anyone trying to remove
> > > > their link from the list (accessing other listeners in the list) is
> > > > buggy.
> > > > 
> > > > > Suddenly allowing this is a breaking API change (*some* struct 
> > > > > wl_list inside
> > > > > a wl_listener) can suddenly become invalid for reasons outside the 
> > > > > users
> > > > > control.
> > > > 
> > > > I don't know if I've quite parsed this right, but as above, not
> > > > removing elements of a destroy listener list, when the listener is
> > > > invoked, is our current API.
> > > > 
> > > > > Related to this entire thing:
> > > > > In [1] you added tests for this and promote something, that is in 
> > > > > essence, a
> > > > > breaking change.
> > > > 
> > > > It's not a breaking change though: it's the API we've pushed on 
> > > > everyone so far.
> > > 
> > > Also, it doesn't prevent external libraries from doing whichever they want
> > > if they have complete control of the destroy listener list contents.
> > 
> > So you suggest we break a now mandated api and expose ourselves to funny
> > implementation detail changes that are now justified, because *we break 
> > API*?
> 
> I'm sorry, I'm have a hard time parsing this.
> 
> The suggested mandate is that libwayland internals won't touch the listener
> after you receive the notification.
Libwayland internals?
That would be fine. Then effectivly nobody can rely on this either way, if 
they ever want to have code that can be integrated with other consumers of 
libwayland.

The problem is, that a single library that relies on this will force anyone 
that uses it to adhere to it.
If we now have a listener that does things properly it will use-after-free if 
it ever shares a signal list with that library.

> 
> That on receipt of a destroy notification you can free your stuff without
> removing your listener from the list.

Which is exactly what we don't want. Since that implies whenever we share a 
destroy signal list with a listener from somewhere that's not inherently our 
code, we can't rely on us being allowed to remove ourselves form the list 
(that's everything from libwayland btw.).
And if we do, we'd have to take the blame for any integration that fails, 
since *WE* break API now.

> doing whichever they want
So if you suggest that we jsut break api here, you actually suggest we do 
something that will break as soon as someone wants to integrate a library that 
also works with another codebase that relies on this mandate.

> 
> > > 
> > > What is prevented is libwayland's destroy notifier list walk accessing an
> > &g

Re: [PATH] core: implement a safe wl_signal_emit

[Markus Ongyerth]

On 2018/2月/22 04:53, Daniel Stone wrote:
> Hi ongy,
> 
> On 22 February 2018 at 16:03, Markus Ongyerth <w...@ongy.net> wrote:
> > On 2018/2月/22 02:58, Daniel Stone wrote:
> >> On 22 February 2018 at 14:14, Markus Ongyerth <w...@ongy.net> wrote:
> >> > The code was buggy the whole time. Just because it was never triggered, 
> >> > does
> >> > not imply it's not a bug.
> >> > free()ing these struct wl_list without removing them from the signal list
> >> > leaves other struct wl_list that are outside the control of the current 
> >> > code
> >> > in an invalid, prone to use-after-free, state.
> >>
> >> There's a difference between something being 'buggy' and a design with
> >> non-obvious details you might not like. If destroy handlers not
> >> removing their list elements were buggy, we would be seeing bugs from
> >> that. But instead it's part of the API contract: when a destroy signal
> >> is invoked, you are guaranteed that this will be the first and only
> >> access to your list member.
> >
> > Could you kindly point me towards the point that states this?
> > Also the point that properly distinguishes two kinds of wl_signal_emit
> > semantics for wl_listener?
> >
> > I don't want to say it can't be made part of the API, I'm saying it is not.
> > And at the current point it's a bug. If the decision is to fix these bugs, 
> > by
> > changing the API or the code, is another issue.
> 
> I can't point you to a document where it is written in stone, no. But
> it would break Weston, WebKit, Clutter, and some others. Changing that
> would be a break of our effective API: we've exposed behaviour which
> people have come to rely on, regardless of whether or not it's
> documented.

I don't mind putting up bug reports with all of them for their broken code.
Also I don't really conside Weston an outside consumer, it's developed over 
the same channels as libwayland. If you want the fixes to weston attached in 
the patch series, I'd be glad to whip them up.

Qt relies on this not being the case (they remove listeners from the list in 
destructors), we rely on it not being the case. If I found a python or java 
wrapper, I would expect them to use detructors as well. It's not just a 
feature of nieche languages like Haskell.
IF we want to guarantee any kind of interop between different libraries, we 
need one (and only one) way to do this. Remove ourselves from the list, or 
free and leave the list broken.

I do agree, as long as we keep to ourselves, we could have the policy to just 
do it as we did so far. But what if we have a library that can be used 
standalone? Then we should keep to the official specification on what to do.
Thus all code has to be adjusted. There is no room for private breaking of 
this.

How do you determine which projects to piss off here?
One side has implemented measures to keep code correct and prevent accidently 
leaking memory or produce use-after-free by forcing a removal before memory is 
freed.
The other side broke basic C sanity by freeing memory that's still pointed 
into.

(Side note: I'm of course only slightly biased here, and this is not written 
in any suggestive way at all)

> 
> Equally, you could say that wl_display_get_fd() is 'buggy' because it
> should follow the established convention of duplicating FDs returned
> by a library, so users can be in control of lifetime. You could 'fix'
> that by having wl_display_get_fd() duplicate the FDs it returns, and
> when every Wayland client using an event loop complains that we're now
> causing them to leak FDs, tell them it's their own fault for relying
> on behaviour which was not formally documented as API/ABI. But that's
> really not a helpful answer, and it's a very good way to get people to
> stop using your software.
The doc of that one actually says "the", not "a" or "a copy of", so I'd 
actually say you are breaking things that you shouldn't :)

I have to admit, I'm making asumptions as well. Based on basic sanity (IMO).
It's convention that explicitly state when something returned by a call has to 
befreed by the caller, so not stating that implicitly states things.

I of course also asume memory I have pointers into doesn't get free()'d at 
arbitrary times without any way for me to detect it.
If this is an asumption that should not be may around libwayland I can 
guarantee, you will be free from my complaints.

And see the section above as to why I think we have to have a certain amount 
of breakage in either direction.
We can't just say "do whatever you want" if we expect code sharing instead of 
monolithic applications.

> > We could have a listener on a client destroy signal to clean ourself

Re: [PATH] core: implement a safe wl_signal_emit

[Markus Ongyerth]

On 2018/2月/22 09:34, Derek Foreman wrote:
> On 2018-02-22 08:58 AM, Daniel Stone wrote:
> > Hi,
> > 
> > On 22 February 2018 at 14:14, Markus Ongyerth <w...@ongy.net> wrote:
> > > > It seems that this patch makes that assumption invalid, and we would
> > > > need patches to weston, enlightenment, and mutter to prevent a
> > > > use-after-free during the signal emit?  Now I'm seeing valgrind errors
> > > > on E and weston during buffer destroy.
> > > > 
> > > > Personally, I don't think we should change this assumption and declare
> > > > the existing code that's worked for years suddenly buggy. :/
> > > 
> > > The code was buggy the whole time. Just because it was never triggered, 
> > > does
> > > not imply it's not a bug.
> > > free()ing these struct wl_list without removing them from the signal list
> > > leaves other struct wl_list that are outside the control of the current 
> > > code
> > > in an invalid, prone to use-after-free, state.
> > 
> > There's a difference between something being 'buggy' and a design with
> > non-obvious details you might not like. If destroy handlers not
> > removing their list elements were buggy, we would be seeing bugs from
> > that. But instead it's part of the API contract: when a destroy signal
> > is invoked, you are guaranteed that this will be the first and only
> > access to your list member. This implies that anyone trying to remove
> > their link from the list (accessing other listeners in the list) is
> > buggy.
> > 
> > > Suddenly allowing this is a breaking API change (*some* struct wl_list 
> > > inside
> > > a wl_listener) can suddenly become invalid for reasons outside the users
> > > control.
> > 
> > I don't know if I've quite parsed this right, but as above, not
> > removing elements of a destroy listener list, when the listener is
> > invoked, is our current API.
> > 
> > > Related to this entire thing:
> > > In [1] you added tests for this and promote something, that is in 
> > > essence, a
> > > breaking change.
> > 
> > It's not a breaking change though: it's the API we've pushed on everyone so 
> > far.
> 
> Also, it doesn't prevent external libraries from doing whichever they want
> if they have complete control of the destroy listener list contents.

So you suggest we break a now mandated api and expose ourselves to funny 
implementation detail changes that are now justified, because *we break API*?

> 
> What is prevented is libwayland's destroy notifier list walk accessing an
> element again after it is potentially freed by external code.

Which could be fixed by said node removing itself from a list, instead of 
leaving a list in invalid states for asumed behaviour.

> 
> We can completely replace the internal data structures in libwayland with
> whatever we want, but we must preserve that behaviour.

Why can we change one implementation detail, but have to keep another one?

> 
> It does not mean we can never rework the destroy signal emit path in
> libwayland to allow some items to be removed by the notification handlers
> and others just freed, or to allow a destroy notifier to touch the list.A

There is no destroy signal emit path.
There is a signal emit path, which may be called on destroy signals, and 
suddenly has to follow different semantics because of what exactly?
And how exactly do we expose that to our listeners? By the name of the signal 
in the struct?

> 
> It just makes it easy to verify that attempts to do that don't break
> guarantees we've always made.

Again, kindly point me to the point where an implementation detail made it 
towards a guarantee.
And for which signals exactly. Under which circumstances, oh and what 
guarantee exactly.


It would be nice if I had a proper proposal to take apart for this to be 
explictly added to the API.
Otherwise I can provide a (intentionaly snarky) one :)

Part one ammend [1] (wl_listener) with:
"The wl_list inside a wl_listener can be invalid (pointer towards free'd 
memory) at any time the listener notify is called. For further details see 
wl_signal.

Part two ammend [2] (wl_signal) with:
Signals with the name `*_destroy` have special semantics.
If they are currently emitted, any wl_signal_add/wl_signal_get on the signal 
or wl_list_remove on the link of any listener in it is invalid.
This is also the cause for invalid struct wl_list entries in wl_listener.

[1] https://wayland.freedesktop.org/docs/html/apc.html#Server-structwl__listener
[2] https://wayland.freedesktop.org/docs/html/apc.html#Server-structwl__signal

Sure, the exact way they are specified here i

Re: [PATH] core: implement a safe wl_signal_emit

[Markus Ongyerth]

On 2018/2月/22 02:58, Daniel Stone wrote:
> Hi,
> 
> On 22 February 2018 at 14:14, Markus Ongyerth <w...@ongy.net> wrote:
> >> It seems that this patch makes that assumption invalid, and we would
> >> need patches to weston, enlightenment, and mutter to prevent a
> >> use-after-free during the signal emit?  Now I'm seeing valgrind errors
> >> on E and weston during buffer destroy.
> >>
> >> Personally, I don't think we should change this assumption and declare
> >> the existing code that's worked for years suddenly buggy. :/
> >
> > The code was buggy the whole time. Just because it was never triggered, does
> > not imply it's not a bug.
> > free()ing these struct wl_list without removing them from the signal list
> > leaves other struct wl_list that are outside the control of the current code
> > in an invalid, prone to use-after-free, state.
> 
> There's a difference between something being 'buggy' and a design with
> non-obvious details you might not like. If destroy handlers not
> removing their list elements were buggy, we would be seeing bugs from
> that. But instead it's part of the API contract: when a destroy signal
> is invoked, you are guaranteed that this will be the first and only
> access to your list member.

Could you kindly point me towards the point that states this?
Also the point that properly distinguishes two kinds of wl_signal_emit
semantics for wl_listener?

I don't want to say it can't be made part of the API, I'm saying it is not.
And at the current point it's a bug. If the decision is to fix these bugs, by 
changing the API or the code, is another issue.

(duplicated)
>when a destroy signal
> is invoked, you are guaranteed that this will be the first and only
> access to your list member.

This is wrong btw. This would prevent us from adding to a listener to a
signal that already has listeners.
Or use wl_signal_get on the destroy signal as soon as it has at least one 
listener.

The semantics you want to state is, that it's the last time it is accessed.
And even that gets iffy if someone wants to wl_signal_get a destroy listener 
at any point.

>This implies that anyone trying to remove
> their link from the list (accessing other listeners in the list) is
> buggy.

This would be rather intersting.
It would be a fair point to add this to the emit callback of destroy 
listeners, but do we have any way to determine if a wl_signal_emit is 
currently in flight for a signal?
What about the case where we have a struct that contains multiple destroy 
listeners to different components? How can we determine, the destroy event 
isn't a transitive event of another listener we have?
We could have a listener on a client destroy signal to clean ourself up, and 
one on a seats.
If such a seat is created by a virtual keyboard (I'm going on a limb here, but 
whatever), it would naturally be destroyed with the client.

Now if we are in the seat destroy signal, how do we determine what to do with 
our client listener?
If this is caused by a client destruction, we can't remove ourselves form the 
client anymore.
If this is not caused by a client destruction, we have to remove ourselves.

I can also easily point you towards usecases, where this would lead to 
pointless code duplication, since we can remove our destroy listeners on very 
similar events, with identical behaviour.
Except for whether we are allowed to (and in that cased forced to) 
wl_list_remove our listener, or we'd have to just do nothing with it, in the 
other case.

> 
> > Suddenly allowing this is a breaking API change (*some* struct wl_list 
> > inside
> > a wl_listener) can suddenly become invalid for reasons outside the users
> > control.
> 
> I don't know if I've quite parsed this right, but as above, not
> removing elements of a destroy listener list, when the listener is
> invoked, is our current API.
> 
> > Related to this entire thing:
> > In [1] you added tests for this and promote something, that is in essence, a
> > breaking change.
> 
> It's not a breaking change though: it's the API we've pushed on everyone so 
> far.

It is a breaking change in libwayland.
Or could you kindly point me to what currently forces me to not remove my 
listeners in libwayland?
Current libwayland allows both versions as implementation detail, but one 
follows the wl_listener semantics, the other makes asumptions.

> 
> > It also makes good wrapper implementations into managed languages annoying.
> > For example (admittedly my own) [2] ensures a wl_listener can never be lost
> > and leak memory. It is freed when the Handle is GC'd.
> > To prevent any use-after-free into this wl_listener, it removes the listener
> > from the list beforehand.
> > I would very much like to keep thi

[PATH] core: implement a safe wl_signal_emit

[Markus Ongyerth]

> Since a destroy signal inidicates the object is utterly dead, I don't think 
> it's unreasonable for users to have assumed that they don't have to clean up 
> their listener link.  It's *never* going to fire again, so why should 
> anything need to be removed?

This only implies that the signal is never fired.
If *any* listener that's on the signal wants to remove itself form the list, 
it will access the memory of the recently freed listener.

While this is unlikly in a codebase where the full implementation is in one 
hand, after they made the decision to handle things this way, it's possible if 
a library is used, that doesn't follow the convention.

> It seems that this patch makes that assumption invalid, and we would 
> need patches to weston, enlightenment, and mutter to prevent a 
> use-after-free during the signal emit?  Now I'm seeing valgrind errors 
> on E and weston during buffer destroy.
> 
> Personally, I don't think we should change this assumption and declare 
> the existing code that's worked for years suddenly buggy. :/

The code was buggy the whole time. Just because it was never triggered, does 
not imply it's not a bug.
free()ing these struct wl_list without removing them from the signal list 
leaves other struct wl_list that are outside the control of the current code 
in an invalid, prone to use-after-free, state.
Suddenly allowing this is a breaking API change (*some* struct wl_list inside 
a wl_listener) can suddenly become invalid for reasons outside the users 
control.

Related to this entire thing:
In [1] you added tests for this and promote something, that is in essence, a 
breaking change.

It also makes good wrapper implementations into managed languages annoying.
For example (admittedly my own) [2] ensures a wl_listener can never be lost 
and leak memory. It is freed when the Handle is GC'd.
To prevent any use-after-free into this wl_listener, it removes the listener 
from the list beforehand.
I would very much like to keep this code (since it is perfectly valid on the 
current ABI) and is good design in managed languages.

With kind regards,
ongy


[1] 
https://lists.freedesktop.org/archives/wayland-devel/2018-February/037169.html
[2] 
https://github.com/swaywm/hsroots/blob/master/src/Graphics/Wayland/Signal.hsc

P.S. sorry for format issues, or if doesn't get tagged into the thread 
properly, I wasn't subscribed to the list, when this came in and had to build 
references manually.


signature.asc
Description: PGP signature
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel