On 12/04/17 09:07 AM, Pekka Paalanen wrote:
On Wed, 12 Apr 2017 11:53:02 +0800
Jonas Ådahl <jad...@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.
As far as I can tell there's no promise of that - it looks like a
SOCK_STREAM is a byte stream with no atomicity/boundary guarantees...
If someone out there has deeper knowledge and can quote spec that gets
us out of this, please speak up. :)
If this is really a concern we could also just kill any client with >
50(arbitrary number) fds lingering in its fds_in, because that's just
anti-social behaviour, right?
I think a dmabuf using client could force the compositor to hold open a
large amount of fds as well, without being bounded by the 1024 fd
capacity of the fds_in buffer anyway - so I'm really not sure how much
we have to care about this...
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.
Ok, we're up to two votes that this is an abi break, countered by my own
apathy and a powerful shrug from an anonymous third party. I'm going to
treat this as a real bug I've introduced that needs to be properly
addressed.
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.
This seems reasonable... With this and a map walk at client disconnect
to reap any zombies that hadn't received delete_id yet, I think we're good.
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.
Are we that concerned about allocations/frees? It seems a little
confusing to do this.
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.
I hadn't actually considered that, I'll keep it in mind for the next
revision.
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.
Yeah, I don't think there's any flexibility in the wire format at all,
anything we might try to do will break something.
I'm going to go with fd counts for opcodes for v2, and see what that
looks like...
Thanks,
pq
_______________________________________________
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel
