[PATCH] client: Allow send error recovery without an abort

[Pekka Paalanen]

On Tue, 19 Jun 2018 12:10:36 -0700
Lloyd Pique <lpique at google.com> wrote:

> Hi Pekka,
> 
> On Tue, Jun 19, 2018 at 5:12 AM Pekka Paalanen <ppaalanen at gmail.com> wrote:
> 

> > If I'm reading the code right, MAX_FDS_OUT is only a limit for a single
> > sendmsg() call. The fds_out buffer is capable of holding 1k fds.
> >  
> 
> > We have 4kB message data buffer (what I've been calling the
> > soft-buffer, as opposed to in-kernel buffers). A minimal request to
> > carry an fd would be header (8B) plus fd (4B), so that's 12B. That
> > makes at most 341 fds until the soft-buffer is full. That should well
> > fit into the 1k fds that fds_out can hold.
> >
> > One could have more fds in a single request. Let's say four. The
> > request size becomes 24B carrying 4 fds. That makes 680 fds for a full
> > soft-buffer. Should still be fine.  
> 
> 
> The limit is both the maximum amount per sendmsg() call, as well as the
> maximum amount that can be sent by a single send request.
> 
> The fd's are added once at a time by a call to wl_connection_put_fd(),
> which forces a flush once the limit is reached. Those calls are made by
> copy_fds_to_connection(), called once per message send from
> wl_closure_send().

Hi,

ah, so there is an artificial limit there as well. I missed that. So
that is what prevents the fds from accumulating in fds_out indefinitely.

> On a flush (forced or not), the fd's are closed by close_fds(), which then
> advances the tail to the head, emptying the ring buffer again.
> 
> That the output fd butter is over-large at 4k just allows much of the ring
> buffer code to be reused. It will never actually hold 1024 fds enqueued for
> output though.
> 
> (The input fd buffer may allow 1k fd's. I did not examine that carefully to
> be sure).
> 
> I think this limit is still a problem. I could change it to actually allow
> 1k fd's to be queued up, but then that will make the dup() more likely to
> fail (more in the other reply).

Yes, upping it to 1k for real would be my suggestion. I think it should
be large enough to accommodate any amount of fds so that the limiting
factor will be the soft-buffer size.

I think the 4 fds in a minimal message is a reasonable upper limit. It
is very rare to find even two fds in a single message, because such a
message cannot be sent at all without all the declared fds. There is no
way to send a "null" fd, so protocol design should favour one message
per fd when the number of fds is variable.


> > However, wl_connection_flush() attempts to write out at most
> > MAX_FDS_OUT fds at a time, but all of the message data. OTOH, one
> > cannot even try to flush out fds without any message data, IIRC
> > sendmsg() doesn't support that. So it can run out of message data while
> > fds accumulate and finally fill fds_out up, causing irrecoverable
> > failure.
> >
> > Do your observations support this theory?
> >  
> 
> I haven't observed a problem case like that. Any normal individual request
> will probably only use a few fd's, and I don't think there is a way for a
> request to not carry some non-fd data.
> 
> I can imagine someone creating a new protocol message that does take more
> than 28 fd's in a request, where that would then fail to flush if there
> were no other normal message data also already queued up. But that seems a
> very abnormal use. wl_closure_marshal() would be the best place for a check
> (it already checks for null objects -- see "goto err_null"), and I didn't
> find other bound check on the number of fd's in a request on a quick look
> through the code.
> 
> But I'll study the code a bit more in case there is some subtlety that
> escapes me.

Yeah, there probably isn't any upper limit coded for number of fds in a
single message, or number of arguments, aside from a generic send
failure.

I believe it would be good to introduce an upper limit of maybe 4 fds
per message. Or at least suggest it. It should be done in both
wayland-scanner and libwayland.

> > It seems we should somehow flush less message data so that we can call
> > sendmsg() enough times to handle all the fds and avoid them piling up.
> > But if we do so, we must be very careful with any potential assumptions
> > on receiving side on the ordering between message data and related fds.
> >  
> 
> I expect that the receiving code will just pull the next fd out of the
> fds_in ring buffer when turning the messages back into wl_closures for each
> message, as it realizes an fd is needed for any argument.
> 
> But I haven't looked at the code. Another thing I will check to be safe.

That is my understanding as well, which means that fds must arrive with
or before the corresponding other message data. They cannot arrive
later. It should be ok to lift this restriction if necessary, but maybe
it's better to make sure data cannot arrive before fds.


Thanks,
pq

> > Even the author of the client would usually not know. It would make a
> > very hard API to use correctly.
> >  
> 
> Ok, I won't introduce anything like that.
> 
> 
> > My objection is to the automatic blocking in general. We have a pretty
> > nicely asynchronous protocol and we try to teach programmers to write
> > their apps in a non-blocking way, so blocking inside libwayland-client
> > is not nice. We have very few functions that are documented as
> > potentially blocking. Extending that to all request sending functions
> > is not acceptable in my mind.
> >  
> 
> I agree and appreciate that the core library is non-blocking, and really
> don't want to change that, which is why my original patch made that
> blocking at least opt-in, and even then NOT even part of the core client
> code.
> 
> - Lloyd

-------------- 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/20180621/1ef64439/attachment.sig>