additional race in the wayland protocols?

[Pekka Paalanen]

On Wed, 27 Mar 2019 22:14:27 +0100
Eugen Friedrich <friedrix at gmail.com> wrote:

> >
> > On Tue, 26 Mar 2019 22:05:28 +0100
> > Eugen Friedrich <friedrix at gmail.com> wrote:
> >  
> > > >
> > > > On Mon, 25 Mar 2019 22:08:38 +0100
> > > > Eugen Friedrich <friedrix at gmail.com> wrote:
> > > >  
> >  
> > > > > > I would also like to see if we can forbid requests that both destroy
> > > > > > the protocol object and create another one in one message. This would
> > > > > > avoid the need for wl_proxy_marshal_constructor_destroy(). Marshalling
> > > > > > is already getting a little bit combinatorial with constructor,
> > > > > > versioned, and array.
> > > > > >  
> > > > > this I did not get!
> > > > > the requests are not in the same message, destroy is an request
> > > > > and create an event in this case, also triggered by different calls:
> > > > > wl_display_flush for sending requrest and
> > > > > wl_display_read/dispatch* to receive the events.
> > > > > Have nothing in mind how to prevent the race, what should be forbidden?  
> > > >
> > > > I wasn't talking about any known existing protocol extension we know
> > > > of. It is just that I suspect it is currently not forbidden to design a
> > > > request that is both a destructor and has a new_id argument, so we
> > > > should assume that someone out there is doing exactly that.
> > > >
> > > > If someone is doing that, wayland-scanner needs to figure out how to
> > > > call the wl_proxy API. To make that use case race-free against
> > > > everything else that might be going on, there would need to be a
> > > > wl_proxy_marshal_constructor_destroy() kind of functionality. I would
> > > > not like to have to add that, it seems too niche.
> > > >
> > > > If it was added, it would need versioned vs. unversioned, and array vs.
> > > > vararags forms as well, so it would be at least four new ABI functions.
> > > >  
> > > maybe it will be possible to forbid any arguments for type=destructor?
> > > at least currently could not find any desctructor with arguments in
> > > the wayland-protocols..  
> >
> > Arguments in destructors are not problematic per se, it's only the
> > new_id arguments.
> >
> > If we went for forbidding either, first would need to check how
> > wayland-scanner handles the case right now. If it is obviously not
> > working, then we can just make it more explicit and intentional with
> > nothing to worry about. If it looks like it may work, then we need a
> > deprecation period with wayland-scanner warning or failing on it to see
> > if anyone was actually using it. We also need a backup plan in case we
> > notice it is something people use and we cannot forbid.
> >
> > There are lots of extensions outside of wayland-protocols, a lot more
> > than in wayland-protocols.
> >  
> I took a look to the scanner and the destructors with arguments and
> also new_id arguments are technically possible, scanner can handle it
> and generate the code.
> also I think I found a use-case where it even can be useful by maybe
> redesigning a bit the protocol :-):
> in the "linux-dmabuf-unstable-v1.xml" protocol the "create_immed"
> request creates a buffer and it could just destroy the param proxy
> within one request, so i guess even described use case is not a
> completely valid one there might be some protocols which could
> implement similar behavior.
> 
> Actually while playing around with the scanner generated code I found
> out that even now to implement the race free destroying we need to add
> a family of new API's in wayland-client.so if arguments are supported:
> some marshal_create*_destroy family.

Hi Eugen,

oh, you mean wl_proxy_marshal*_destroy()? That is exactly the thing
needed to solve your original issue, some arguments or none.

We shouldn't use marshal_constructor_destroy() as "maybe create". When a
_constructor_ is called, there should be exactly one new_id argument.

> to avoid this i see only two
> other options:
> 
> 1. hacky solution which was already proposed with proxy_wrapper-> ugly

I believe that approach is illegal. The doc for
wl_proxy_create_wrapper() says:

 * A proxy wrapper must be destroyed before the proxy it was created from.

You would essentially be using a stale object ID to send out the
request. If libwayland-client actually checked the validity of the ID,
it would fail.

> 2. fix the race only for destructors without parameters and print a
> deprecate warning in case if destructor has some parameter, this we
> should do if we agree to forbid parameters for destructors in the
> future.

The fix for destructors without parameters at all will also fix
destructors with parameters that are not new_id.

Only the new_id case would need yet more new API.

***

A wild idea I have not investigated at all would be to add the
destructor annotation into the message signature string. Then we could
make the existing marshall functions handle the destruction as well. It
might be tricky to pull off in a reliable backwards compatible manner,
but I don't think it would be impossible.

The new_id args are already in the message signature, obviously, so if
we wanted, we could collapse marshal and marshal_constructor calls into
one marshal_maybe_construct function.

If we did both, then in the end we would only have:
- wl_proxy_destroy() for destroy without a request
- wl_proxy_marshal_maybe_construct{,_array} for everything else

I'm not sure that is worth though, given that the old functions must
remain and keep working.


Thanks,
pq
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/wayland-devel/attachments/20190328/4c449d83/attachment.sig>