On Thu, Jul 12, 2018 at 06:05:14PM +0200, Jeremie Courreges-Anglas wrote: > --8<-- > space = sbspace(so, &so->so_snd); > if (flags & MSG_OOB) > space += 1024; > if ((atomic && resid > so->so_snd.sb_hiwat) || > (so->so_proto->pr_domain->dom_family != AF_UNIX && > clen > so->so_snd.sb_hiwat)) > snderr(EMSGSIZE); > if (space < clen || > (space - clen < resid && > (atomic || space < so->so_snd.sb_lowat))) { > if ((so->so_state & SS_NBIO) || (flags & MSG_DONTWAIT)) > snderr(EWOULDBLOCK); > sbunlock(so, &so->so_snd); > error = sbwait(so, &so->so_snd); > so->so_state &= ~SS_ISSENDING; > if (error) > goto out; > goto restart; > } > space -= clen; > -->8-- > > I hate those conditions. I do not fully understand their true intent > and I feel they are already brittle; I'm afraid that adding more > conditionals will worsen clarity. How are those three (actually way > more than three) conditions actually supposed to interact? > > If people have a clearer idea than I do about what exactly should happen > here, please share.
If it is a temporary problem, that will go away when the content of the socket buffer is sent away, we should block or return EWOULDBLOCK. For a permanent problem return EMSGSIZE. Non atomic operations can be split in smaller chunks, so there are no permanent problems. Control messages are considerd atomic. AF_UNIX needs special treatment as file descriptor passing is dificult. On top of that consider integer overflow. revision 1.100 date: 2012/04/24 16:35:08; author: deraadt; state: Exp; lines: +13 -3; In sosend() for AF_UNIX control message sending, correctly calculate the size (internalized ones can be larger on some architectures) for fitting into the socket. Avoid getting confused by sb_hiwat as well. This fixes a variety of issues where sendmsg() would fail to deliver a fd set or fail to wait; even leading to file leakage. Worked on this with claudio for about a week... revision 1.145 date: 2016/01/06 10:06:50; author: stefan; state: Exp; lines: +27 -30; Prevent integer overflows in sosend() and soreceive() by converting min()+uiomovei() to ulmin()+uiomove() and re-arranging space computations in sosend(). The soreceive() part was also reported by Martin Natano. ok bluhm@ and also discussed with tedu@ So first of all we should split the AF_UNIX cases to keep it readable. And I don't want to change the AF_UNIX code as the commit message indicates that it was hard to develop the current solution. >From the bug reports it seems that we should check that the UDP packets and the IP_SENDSRCADDR fit into the socket buffer. If not it is a permanent EMSGSIZE error. So make sure that resid + clen <= so->so_snd.sb_hiwat. Then write it the other way around to prevent signed integer overflow. The result of this considerations is the diff below. I have not tested it. Does the orignal bug go away with it? Some hackathons ago jeremy@ mentioned that the ruby test suite found a bug in this area. So maybe we should try it. Does this make sense? bluhm Index: kern/uipc_socket.c =================================================================== RCS file: /data/mirror/openbsd/cvs/src/sys/kern/uipc_socket.c,v retrieving revision 1.225 diff -u -p -r1.225 uipc_socket.c --- kern/uipc_socket.c 5 Jul 2018 14:45:07 -0000 1.225 +++ kern/uipc_socket.c 12 Jul 2018 17:24:28 -0000 @@ -462,10 +462,14 @@ restart: space = sbspace(so, &so->so_snd); if (flags & MSG_OOB) space += 1024; - if ((atomic && resid > so->so_snd.sb_hiwat) || - (so->so_proto->pr_domain->dom_family != AF_UNIX && - clen > so->so_snd.sb_hiwat)) - snderr(EMSGSIZE); + if (so->so_proto->pr_domain->dom_family == AF_UNIX) { + if (atomic && resid > so->so_snd.sb_hiwat) + snderr(EMSGSIZE); + } else { + if (clen > so->so_snd.sb_hiwat || + (atomic && resid > so->so_snd.sb_hiwat - clen)) + snderr(EMSGSIZE); + } if (space < clen || (space - clen < resid && (atomic || space < so->so_snd.sb_lowat))) {