Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

backend: Handle EINTR, partial writes when using sendmsg #647

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ids1024
Copy link
Member

@ids1024 ids1024 commented Aug 10, 2023

Correct use of sendmsg/write, or any such call needs to handle EINTR, and partial writes. Even if this may be rare. wl_connection_flush in libwayland handles these things.

A couple things still seem wrong:

  • If there's a limit to how many fds can be send in one message, that needs to be handled. I don't know if POSIX defines that anywhere.
  • Using a Vec<u32> for the buffer here may be problematic. What if a partial write writes a number of bytes that isn't a multiple of 4? There is in general no guarantee that won't happen.

Correct use of `sendmsg`/`write`, or any such call needs to handle
`EINTR`, and partial writes. Even if this may be rare. `wl_connection_flush`
in libwayland handles these things.

A couple things still seem wrong:
* If there's a limit to how many fds can be send in one message, that
  needs to be handled. I don't know if POSIX defines that anywhere.
* Using a `Vec<u32>` for the buffer here may be problematic. What if a
  partial write writes a number of bytes that isn't a multiple of 4?
  There is in general no guarantee that won't happen.
@elinorbgr
Copy link
Member

If there's a limit to how many fds can be send in one message, that needs to be handled. I don't know if POSIX defines that anywhere.

I don't think this one will be an issue, both libwayland and wayland-backend puts a hard limit on the number of FD that can be sent in a single message at 28, though we might need to check that the current logic does actually respect this limit, I'm not 100% sure...

Using a Vec for the buffer here may be problematic. What if a partial write writes a number of bytes that isn't a multiple of 4? There is in general no guarantee that won't happen.

That a good point, we should change that.

@ids1024
Copy link
Member Author

ids1024 commented Sep 3, 2023

If it's buffering data and file descriptors from multiple messages, before sending on a flush, it could have more fds than the limit for a single message, right?

@elinorbgr
Copy link
Member

Ah sorry, the limit of 28 fds applies to unix datagrams, not wayland messages

@ids1024
Copy link
Member Author

ids1024 commented Sep 3, 2023

Ah, looking at https://www.man7.org/linux/man-pages/man7/unix.7.html:

The kernel constant SCM_MAX_FD defines a limit on the number of file descriptors in the array. Attempting to send an array larger than this limit causes sendmsg(2) to fail with the error EINVAL. SCM_MAX_FD has the value 253 (or 255 before Linux 2.6.38).

It looks like SCM_MAX_FD is a Linux thing, and not part of POSIX? Not sure what limit would apply on BSDs. (Assuming there is one.)

It seems quite unlikely that 253 file descriptors would be in queued messages in wayland-rs before the connection is flushed, so this is unlikely to be a problem in practice. But it would be good to make sure it works correctly anyway. Not a major priority though.

@elinorbgr
Copy link
Member

The constraint is more that the receiving side will not expect more than 28 FDs in a single unix datagram, so sending more than 28 will cause some FD to be lost (recvmsg will drop them if you don't provide a large enough buffer to receive them).

@ids1024
Copy link
Member Author

ids1024 commented Sep 3, 2023

Wayland uses SOCK_STREAM not SOCK_DGRAM, so even if sendmsg/recvmsg are used, I don't think there's really any concept of bytes and fds being parts of discrete datagrams/messages.

But I'm not sure exactly how that works on the receiving end. When a sendmsg call sent more file descriptors than your buffer fits, or multiple sendmsg calls sent more file descriptors than that....

@elinorbgr
Copy link
Member

Right, good point. Well, if I believe recvmsg's man page we don't really have anything to worry about, and in any case there is no point in worrying about that more than what libwayland does.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants