git: 3b93d3597cc3 - main - unix: Fix some bugs in the SOCK_STREAM receive path
- Go to: [ bottom of page ] [ top of archives ] [ this month ]
Date: Mon, 10 Aug 2026 17:40:29 UTC
The branch main has been updated by markj:
URL: https://cgit.FreeBSD.org/src/commit/?id=3b93d3597cc336d5637e12ade8642d589cb0bc8a
commit 3b93d3597cc336d5637e12ade8642d589cb0bc8a
Author: Mark Johnston <markj@FreeBSD.org>
AuthorDate: 2026-08-10 14:41:33 +0000
Commit: Mark Johnston <markj@FreeBSD.org>
CommitDate: 2026-08-10 17:31:21 +0000
unix: Fix some bugs in the SOCK_STREAM receive path
The main problem is with the handling of errors from unp_externalize().
It turns out that this was quite broken, and unfortunately it's easy to
trigger such errors (e.g., by setting a low per-process fd limit with
setrlimit()).
In non-peek mode, uipc_soreceive_stream_or_seqpacket() cuts a bunch of
mbufs from the head of the socket buffer, to be consumed by userspace.
When unp_externalize() returns an error, we splice the removed mbuf
chain back onto the head of the socket buffer. This is expensive, but
that's ok since such errors are rare.
The problem is that this cutting is not correctly implemented: it does
not clear the "next" pointer for the last mbuf in the chain, so it
still points to the first mbuf still resident in the socket buffer.
This means that mc_init_m() creates a chain that still includes the rest
of the socket buffer, so splicing the chain back into the socket buffer
does not work properly.
Fix this: fully detach the control chain from the socket buffer so that
we can safely use mc_init_m(). Then, incrementally add data mbufs,
taking care to handle "part".
Fix some related bugs while here:
- Don't swallow the error if unp_externalize() fails and there's nothing
left in the socket buffer (i.e., control->m_next == NULL).
- Roll back changes to the partially read mbuf.
Reviewed by: glebius
MFC after: 1 week
Sponsored by: The FreeBSD Foundation
Differential Revision: https://reviews.freebsd.org/D58695
---
sys/kern/uipc_usrreq.c | 39 +++++++++++--
sys/sys/mbuf.h | 7 +++
tests/sys/kern/unix_passfd_test.c | 120 ++++++++++++++++++++++++++++++++++++++
3 files changed, 162 insertions(+), 4 deletions(-)
diff --git a/sys/kern/uipc_usrreq.c b/sys/kern/uipc_usrreq.c
index df9568015724..0e3f0d13c12d 100644
--- a/sys/kern/uipc_usrreq.c
+++ b/sys/kern/uipc_usrreq.c
@@ -1451,14 +1451,26 @@ restart:
ctl = 0;
first = STAILQ_FIRST(&sb->uxst_mbq);
if (first->m_type == MT_CONTROL) {
+ struct mbuf *prev;
+
control = first;
+ prev = NULL;
+
+ /*
+ * Unlink control messages from the socket buffer. The head of
+ * the socket buffer queue is updated below.
+ */
STAILQ_FOREACH_FROM(first, &sb->uxst_mbq, m_stailq) {
- if (first->m_type != MT_CONTROL)
+ if (first->m_type != MT_CONTROL) {
+ if (!peek && prev != NULL)
+ STAILQ_NEXT(prev, m_stailq) = NULL;
break;
+ }
ctl += first->m_len;
mbcnt += MSIZE;
if (first->m_flags & M_EXT)
mbcnt += first->m_ext.ext_size;
+ prev = first;
}
} else
control = NULL;
@@ -1553,10 +1565,25 @@ restart:
*/
error = unp_externalize(so, control, controlp, flags);
control = m_free(control);
- if (__predict_false(error && control != NULL)) {
+ if (__predict_false(error != 0)) {
struct mchain cmc;
- mc_init_m(&cmc, control);
+ /*
+ * Build an mbuf chain containing the remainder
+ * of the control messages and the subsequent
+ * data, to be prepended back to the socket
+ * buffer.
+ */
+ if (control != NULL)
+ mc_init_m(&cmc, control);
+ else
+ mc_init(&cmc);
+ for (m = first; datalen > 0 && m != part;
+ m = next) {
+ datalen -= m->m_len;
+ next = STAILQ_NEXT(m, m_stailq);
+ mc_append(&cmc, m);
+ }
SOCK_RECVBUF_LOCK(so);
if (__predict_false(
@@ -1566,7 +1593,7 @@ restart:
/*
* While the lock was dropped and we
* were failing in unp_externalize(),
- * the peer could has a) disconnected,
+ * the peer could have a) disconnected,
* b) filled the buffer so that we
* can't prepend data back.
* These are two edge conditions that
@@ -1590,6 +1617,10 @@ restart:
sb->sb_mbcnt = 0;
STAILQ_FOREACH(m, &sb->uxst_mbq, m_stailq) {
if (m->m_type == MT_DATA) {
+ if (m == part) {
+ m->m_len += partlen;
+ m->m_data -= partlen;
+ }
sb->sb_acc += m->m_len;
sb->sb_ccc += m->m_len;
} else {
diff --git a/sys/sys/mbuf.h b/sys/sys/mbuf.h
index 05fd522b7618..076cfecb1f99 100644
--- a/sys/sys/mbuf.h
+++ b/sys/sys/mbuf.h
@@ -1758,6 +1758,13 @@ mc_dec(struct mchain *mc, struct mbuf *m)
}
}
+static inline void
+mc_init(struct mchain *mc)
+{
+ STAILQ_INIT(&mc->mc_q);
+ mc->mc_len = mc->mc_mlen = 0;
+}
+
/*
* Get mchain from a classic mbuf chain linked by m_next. Two hacks here:
* we use the fact that m_next is alias to m_stailq, we use internal queue(3)
diff --git a/tests/sys/kern/unix_passfd_test.c b/tests/sys/kern/unix_passfd_test.c
index 665fce767eb2..56893750354f 100644
--- a/tests/sys/kern/unix_passfd_test.c
+++ b/tests/sys/kern/unix_passfd_test.c
@@ -1094,6 +1094,125 @@ ATF_TC_BODY(copyout_rights_error, tc)
closesocketpair(fd);
}
+/*
+ * Exercise handling of errors from unp_externalize().
+ */
+ATF_TC_WITHOUT_HEAD(externalize_error_partial_read);
+ATF_TC_BODY(externalize_error_partial_read, tc)
+{
+ struct iovec iovec;
+ struct msghdr msghdr;
+ struct rlimit rl, orl;
+ struct stat sb;
+ char cmsgbuf[CMSG_SPACE(sizeof(int))];
+ char msg1[16];
+ char *fill, *rbuf;
+ size_t fillsz;
+#if TEST_PROTO == SOCK_STREAM
+ size_t got;
+#endif
+ ssize_t len;
+ int fd[2], nfds, putfd;
+
+ memset(msg1, 'A', sizeof(msg1));
+
+ domainsocketpair(fd);
+ devnull(&putfd);
+ dofstat(putfd, &sb);
+ nfds = getnfds();
+
+#if TEST_PROTO == SOCK_STREAM
+ fillsz = (size_t)getrecvspace() * 3 / 5;
+#elif TEST_PROTO == SOCK_DGRAM
+ fillsz = 128;
+#endif
+
+ fill = malloc(fillsz);
+ ATF_REQUIRE(fill != NULL);
+ memset(fill, 'B', fillsz);
+ rbuf = malloc(sizeof(msg1) + fillsz);
+ ATF_REQUIRE(rbuf != NULL);
+
+ /*
+ * The first message carries the rights and a small payload; the second
+ * queues more data behind it, so that the read below leaves the receive
+ * buffer non-empty.
+ */
+ len = sendfd_payload(fd[0], putfd, msg1, sizeof(msg1));
+ ATF_REQUIRE_MSG(len == (ssize_t)sizeof(msg1),
+ "sendmsg: %zd bytes sent; expected %zu: %s", len, sizeof(msg1),
+ strerror(errno));
+ len = send(fd[0], fill, fillsz, 0);
+ ATF_REQUIRE_MSG(len == (ssize_t)fillsz,
+ "send: %zd bytes sent; expected %zu: %s", len, fillsz,
+ strerror(errno));
+
+ /*
+ * Use fd limits to force receive to fail.
+ */
+ ATF_REQUIRE_MSG(getrlimit(RLIMIT_NOFILE, &orl) == 0,
+ "getrlimit failed: %s", strerror(errno));
+ rl = orl;
+ rl.rlim_cur = 1;
+ ATF_REQUIRE_MSG(setrlimit(RLIMIT_NOFILE, &rl) == 0,
+ "setrlimit failed: %s", strerror(errno));
+
+ bzero(&msghdr, sizeof(msghdr));
+ iovec.iov_base = rbuf;
+ iovec.iov_len = sizeof(msg1);
+ msghdr.msg_iov = &iovec;
+ msghdr.msg_iovlen = 1;
+ msghdr.msg_control = cmsgbuf;
+ msghdr.msg_controllen = sizeof(cmsgbuf);
+
+ ATF_REQUIRE_ERRNO(EMFILE, recvmsg(fd[1], &msghdr, 0) == -1);
+
+ ATF_REQUIRE_MSG(setrlimit(RLIMIT_NOFILE, &orl) == 0,
+ "setrlimit failed: %s", strerror(errno));
+
+ /* The rights must have been disposed of rather than installed. */
+ ATF_REQUIRE_MSG(getnfds() == nfds, "descriptor leaked");
+
+ /*
+ * The failed read must leave the socket usable with both payloads still
+ * queued.
+ */
+#if TEST_PROTO == SOCK_STREAM
+ for (got = 0; got < sizeof(msg1) + fillsz; got += (size_t)len) {
+ len = recv(fd[1], rbuf + got, sizeof(msg1) + fillsz - got, 0);
+ if (len <= 0)
+ break;
+ }
+ ATF_REQUIRE_MSG(got == sizeof(msg1) + fillsz,
+ "recovered %zu of %zu bytes after the failed read: %s", got,
+ sizeof(msg1) + fillsz, strerror(errno));
+ ATF_REQUIRE_MSG(memcmp(rbuf, msg1, sizeof(msg1)) == 0,
+ "first payload corrupted");
+ ATF_REQUIRE_MSG(memcmp(rbuf + sizeof(msg1), fill, fillsz) == 0,
+ "second payload corrupted");
+#elif TEST_PROTO == SOCK_DGRAM
+ /*
+ * For datagrams, soreceive_dgram() dequeues the record before
+ * processing control messages, so the first datagram's payload is
+ * consumed even when externalize fails. Only the second datagram
+ * should remain queued.
+ */
+ len = recv(fd[1], rbuf, fillsz, 0);
+ ATF_REQUIRE_MSG(len == (ssize_t)fillsz,
+ "second datagram: got %zd bytes, expected %zu: %s", len, fillsz,
+ strerror(errno));
+ ATF_REQUIRE_MSG(memcmp(rbuf, fill, fillsz) == 0,
+ "second payload corrupted");
+#endif
+
+ dofstat(putfd, &sb);
+
+ free(rbuf);
+ free(fill);
+ close(putfd);
+ closesocketpair(fd);
+}
+
/*
* Verify that we can handle empty rights messages.
*/
@@ -1424,6 +1543,7 @@ ATF_TP_ADD_TCS(tp)
ATF_TP_ADD_TC(tp, rights_creds_payload);
ATF_TP_ADD_TC(tp, truncated_rights);
ATF_TP_ADD_TC(tp, copyout_rights_error);
+ ATF_TP_ADD_TC(tp, externalize_error_partial_read);
ATF_TP_ADD_TC(tp, empty_rights_message);
ATF_TP_ADD_TC(tp, control_creates_records);
ATF_TP_ADD_TC(tp, cross_jail_dirfd);