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

Hi Pekka,

I revisited my opinion about you suggestion and changed my mind.
will not tell here why exactly, basically I just tried my and you approach.

You solution is quite elegant and I saw that signature is already use
for "since" description
so the wayland code is prepared to deal with some chars in signature
which are not argument.
I will suggest this solution (destroy "tag" in signature, so far I
choose "-" and I adding this as last attribute)
in next days.

best regard
jeka

> best regards
> jeka
> >
> > Thanks,
> > pq