additional race in the wayland protocols?

[Eugen Friedrich]

>
> 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.

Hi Pekka,

ach yes, currently I see a need for two new functions:
 - destroying without and
 - destroying with new_id arguments
>
> We shouldn't use marshal_constructor_destroy() as "maybe create". When a
> _constructor_ is called, there should be exactly one new_id argument.
>

agree, also the generated code looks very different, in case we do not
have new_id
argument we would need to end up in returning NULL, ugly!

> > 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.
>

ups, did not read carefully , but the workaround works at wayland 1.13
for sure and should work on master (code review)
seams libwayland-client is not checking for validity, anyway I will
not mention this again, in any case it is a dirty workaround

> > 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.
>

I see, will then add those two and see how to carefully implement this
in scanner.c

> ***
>
> 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 i
> 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.
>

doesn't look like good solution to me:
 - code generation is different if we have a new_id arguments, we
returning a new object in this case
 - currently "destroy" request is just another type of message and not
reflected anywhere else, scanner is checking for it explicitly and it
is allowed to have many destructors.
Scanner checks only if request with the name destroy is of type destructor.
To use you "wild idea" from above we would need to "miss use"
arguments signature for message type and take of it correctly in all
of the places in the wayland-client code.
Backwards compatibility should be possible but in general I would not
follow this solution, at least for now.
will continue with two new API's...

best regards
jeka
>
> Thanks,
> pq