Recursive dispatch (Re: [PATCH v2] server: Calculate remaining data size after a closure is processed)

[Bryce Harrington]

On Wed, Feb 03, 2016 at 01:37:21PM +0200, Pekka Paalanen wrote:
> On Wed, 3 Feb 2016 10:52:07 +0000
> Auke Booij <auke at tulcod.com> wrote:
> > On 3 February 2016 at 09:34, Jonas Ådahl <jadahl at gmail.com> wrote:
> > > On Wed, Feb 03, 2016 at 11:14:54AM +0200, Pekka Paalanen wrote:  
> > >> On Wed, 3 Feb 2016 09:56:20 +0900
> > >> "Jaeyoon Jung" <jaeyoon.jung at lge.com> wrote:
> > >> > > > If someone starts hardening libwayland against programmer errors,
> > >> > > > I'd expect one thing to be to abort() on recursive dispatch. Can
> > >> > > > you tell me why we should support recursive dispatch?  
> > >> > >
> > >> > > IMHO we should either do that immediately (before the 1.10 release) or
> > >> > > revert this patch now.  Not that anything is wrong with the patch - it
> > >> > > doesn't introduce bugs in libwayland...
> > >> > >
> > >> > > Previously recursive dispatch was impossible, with this patch it's
> > >> > > possible.  The patch was landed to allow recursive dispatch to work,
> > >> > > and it's obvious someone's about to start using it.  
> > >>
> > >> You're right, we should make a statement about this. I'd be happy to
> > >> accept a doc patch to be included in 1.10 as AFAIK we have never
> > >> intended recursive dispatch to work reliably in either server or client
> > >> code. I do not consider it as a change in API/ABI or behaviour, just a
> > >> doc improvement.

Yes, a doc patch would be fine to land right now.

> > >> > To be more exact, previously recursive dispatch allowed a message copy
> > >> > from empty buffer and resulted in an irrelevant error post to the client.
> > >> > If we want to disallow recursive dispatch, it would be nice to raise an
> > >> > error like abort() when it is detected. Reverting this patch is just
> > >> > going back to the past without resolving the issue.  
> > >>
> > >> Raising an error like abort() is a more touchy subject, because it
> > >> indeed has potential to break existing programs that work by luck. We
> > >> probably want an escape hatch for a while, like an environment variable
> > >> to not abort() and just complain. Or maybe complaining is enough at
> > >> first and add the abort() after a couple stable releases.
> > >>
> > >> I won't propose anyone to do that work now, because I am hoping someone
> > >> might do it as a part of GSoC (yes, there are plans for GSoC, just not
> > >> announced yet I think).
> > >>  
> > >> > I agree that recursive dispatch might be harmful and should be avoided.
> > >> > However, I'm not sure if it is something must be done in libwayland.
> > >> > Isn't it sufficient to be documented somewhere and let the implementor
> > >> > make a decision?  
> > >>
> > >> If we document that recursive dispatch is not supported, then we won't
> > >> bother with patches to fix problems with recursive dispatch, which
> > >> implies that we could also just revert this patch since we won't care.
> > >> But the patch is in, and I don't think we win anything by reverting it.
> > >>
> > >> If some usage pattern is known broken, I think it would be nice to
> > >> catch them and at least complain in libwayland. So far we haven't added
> > >> such code for even every obvious mistake, like destroying objects while
> > >> other objects are still referencing them.
> > >>  
> > >> > > If we later intentionally break that code, we're jerks.
> > >> > > -- From the jerk that posted a bunch of patches to do more stringent
> > >> > > checks on resource versions a few days ago...  arguably those have
> > >> > > always been bugs, though. ;)  
> > >>
> > >> Yeah, recursive dispatch is less clear, so we should have a transition
> > >> period.
> > >>  
> > >> > > So, with the release coming so quickly, I think we should either
> > >> > > decide whether recursive dispatch is legal or illegal before the
> > >> > > release, or revert the patch (thus re-breaking recursive dispatch) and
> > >> > > worry about it for 1.11.  
> > >>
> > >> I would declare recursive dispatch as illegal. Does anyone object?  
> > >
> > > I don't object declaring it illegal in libwayland-server since it was
> > > previously broken, but doing so in libwayland-client I don't think we
> > > should (at least yet) since it is used here and there in various clients
> > > already.  
> 
> Ok, let's do it for server since that's what we are looking at right
> now. (Warnings and aborts would be implemented later.)
> 
> Client side is indeed more complicated, since it provably works
> somewhat, we've had it in some Weston demos even, that were since fixed
> AFAIK.

Is there any chance that a client could make use of this to overload the
server?  If there is, would that imply some security implications that
we need to account for?  It is probably wiser to close such holes before
releasing, even if it might risk delaying it.

> Maybe we could start client side sanitation by these steps release by
> release:
> 
> 1. Document that recursive dispatch is problematic and recommend
>    against it.
> 
> 2. Add a warning when a recursive dispatch is detected, maybe once per
>    wl_display (or wl_event_queue?).
> 
> 3. Make the warning be printed every time, not just once.
> 
> 4. Make the warning also abort().
> 
> Now, how far we will go depends on the reception. I think we should do
> 1, and maybe stop at 2. We could try 3 and see how much feedback it
> generates.

The phrase "...deprecated and will be removed..." might add emphasis to
the need for the client to be adjusted.

> It's also possible that we have to stop at 1, because it is too easy to
> end up in an event handler calling a third party library routine which
> waits for an event. Waiting means it has to dispatch. Calling
> eglSwapBuffers from a frame callback handler, for instance.
> 
> Or maybe we should do it per wl_event_queue, so that if dispatching an
> event queue ends up dispatching itself, warn only then. Dispatching the
> same queue is the biggest problem, right?
> 
> Btw. are we sure that client side does not have the same problem as the
> server side?
> 
> I don't think we have any tests on the matter.

Yes, seems like would be worth having coverage for this case.

Bryce