[PATCH wayland 8/9] client: Replace the singleton zombie with bespoke zombies

[Pekka Paalanen]

On Wed, 12 Apr 2017 11:53:02 +0800
Jonas Ådahl <jadahl at gmail.com> wrote:

> On Tue, Apr 11, 2017 at 10:37:28AM -0500, Derek Foreman wrote:
> > On 07/04/17 03:27 PM, Derek Foreman wrote:  
> > > Using the singleton zombie object doesn't allow us to posthumously retain
> > > object interface information, which makes it difficult to properly inter
> > > future events destined for the recently deceased proxy.
> > > 
> > > Notably, this makes it impossible for zombie proxy destined file
> > > descriptors to be properly consumed.
> > > 
> > > Instead of having a singleton zombie object, we add a new wl_map flag
> > > (valid only on the client side where zombies roam - the existing
> > > "legacy" flag was only ever used on the server side) to indicate that a
> > > map entry is now a zombie.
> > > 
> > > We still have to refcount and potentially free the proxy immediately or
> > > we're left with situations where it can be leaked forever.  If we clean
> > > up on delete_id, for example, a forced disconnect will result in a leaked
> > > proxy (breaking much of the test suite).
> > > 
> > > So, since we must free the zombied proxy immediately, we store just the
> > > interface for it in its previous map location, so signature information
> > > can be retained for zombie-destined events.  
> > 
> > So even with this in place it's still possible for a malicious application
> > to consume 1000fds (the number of fds that fit in fds_in) and go to sleep
> > and hold them - on client disconnect they should be freed.  I don't really
> > see a way to prevent that sort of crap anyway - a client could use sendmsg
> > directly, send a single byte of regular data (ie: not enough to trigger
> > demarshalling, the server will assume there's more to come), and a buffer's
> > worth of file descriptors, then never speak again.
> > 
> > This appears indistinguishable from reasonable behaviour?

Possibly, unless we can prove that we always send complete messages so
that we could require receiving complete messages (and AF_UNIX
guarantees that) in a single recvmsg() call.

> > There's also an interesting side effect to this patch that I noticed
> > yesterday - it places a requirement on the client to keep the wl_interfaces
> > around in memory long after it destroys the proxy - up until the delete_id
> > occurs.  We have no way to hook delete_id in the client.  This turned into a
> > crash in EFL clients using the text input protocol, as the input method code
> > in EFL is a module that's unloaded on shutdown.  It was easily fixed in EFL,
> > but I'd like some input - is a client allowed to unmap the memory that a
> > wl_interface is stored in at the moment it deletes the relevant proxy?
> > 
> > This isn't terribly difficult to fix, but I won't bother if the behaviour is
> > universally concluded to be a client bug. :)  

Even though it's EFL, I think it's actually something people might be
using. Unloading plugins, or libEGL.so or whatever, while keeping the
Wayland connection alive.

Hence I would be hesitant to add the requirement that wl_interface data
needs to be kept around until the connection is closed.

> We could instead introduce a struct wl_zombie_object that constains the
> data needed for doing a proper cleanup. This data could be for example
> be number of fds per event opcode, or strdup:ed event signatures or
> something. All of this would be known at zombification, and we'd avoid
> any ABI change as this patch, as you say, seems to introduce.
> 
> If we'd care about minimizing allocations, we could also insert the old
> WL_ZOMBIE_OBJECT when there are no fds to clean up, and handle that as a
> another special case on clean up.

If we need to allocate stuff to ensure clean-up, that allocation should
be done at the time the wl_proxy is created. Allocation may
theoretically fail, so destruction paths shouldn't allocate.
Essentially that means we should copy what we need at the time of
setting the interface data.

Since we can receive multiple messages in a single recvmsg() call, we
need to parse them all before we can figure out which fds belong to
which message. I wish there was a way around that, but it looks like we
need the message fd counts in any case. Maybe copying just the fd
counts only for opcodes that have any is the best we can do. Too bad
our wire format does not include the number of fds along with the
message length.


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/20170412/3f4bc034/attachment.sig>