jnlGqCTKTeykPzJMUwyPhTAcZ171wPbnhOHo0EQ1iRsbLP5M9U089nSR5g cHEWiHj4TRe6Ey6J533brxDebWyx1fFfvQFgkKfqHlrOmR1S7yO6nH+gMfbyHdLaFQHHsY PiyJbRmpC/pcEGVylf4US0ItMc/fL/F9mx+9tr8QP5GHwGd/AQvWUAmTqD2s0w== ARC-Authentication-Results: i=1; mx1.freebsd.org; none ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=freebsd.org; s=dkim; t=1786383629; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding; bh=IVyhXVTz8KUW37lrxBsXbmQwpu6Dj0TG6huMUI1OlXs=; b=Q7r6BAdUSozUS6WHLCyTc1WKJGBxQnxDg8pyafU1wPYkGGjdNvRqgFdFtMp3/V/Qu4FHqM SL4eZXVT8liQ0arFRxgciKLECon7crWuzYb9vwjjRI8t4TgkcFN/Mye27lfiJNUZPDOi3R 0O5M8qdm2onGnqhFRtQiE6YLzKg5vXwe86jcIYgJs2Bo20b9c31dY3uvppoLb+lQIPqQNL wGiSzCxQBtKE6oYKX08LezF8p38NvHWhhsrQ2UvxCAAv2uvPfFqKGnCXgyQzcSVVgR5E9h Wn32a2N0upRcGxA4M/zPhp8aEVQqtoyCS3xQry5hXSRXvKnvpC9/MXTJV+uVzw== Received: from gitrepo.freebsd.org (gitrepo.freebsd.org [IPv6:2610:1c1:1:6068::e6a:5]) by mxrelay.nyi.freebsd.org (Postfix) with ESMTP id 4hJhqK19qMz3sx for ; Mon, 10 Aug 2026 17:40:29 +0000 (UTC) (envelope-from git@FreeBSD.org) Received: from git (uid 1279) (envelope-from git@FreeBSD.org) id 3e855 by gitrepo.freebsd.org (DragonFly Mail Agent v0.13+ on gitrepo.freebsd.org); Mon, 10 Aug 2026 17:40:29 +0000 To: src-committers@FreeBSD.org, dev-commits-src-all@FreeBSD.org, dev-commits-src-main@FreeBSD.org From: Mark Johnston Subject: git: 3b93d3597cc3 - main - unix: Fix some bugs in the SOCK_STREAM receive path List-Id: Commit messages for the main branch of the src repository List-Archive: https://lists.freebsd.org/archives/dev-commits-src-main List-Help: List-Post: List-Subscribe: List-Unsubscribe: X-BeenThere: dev-commits-src-main@freebsd.org Sender: owner-dev-commits-src-main@FreeBSD.org List-Id: List-Post: List-Help: List-Subscribe: List-Unsubscribe: List-Owner: Precedence: list MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit X-Git-Committer: markj X-Git-Repository: src X-Git-Refname: refs/heads/main X-Git-Reftype: branch X-Git-Commit: 3b93d3597cc336d5637e12ade8642d589cb0bc8a Auto-Submitted: auto-generated Date: Mon, 10 Aug 2026 17:40:29 +0000 Message-Id: <6a7a0d0d.3e855.45f8f322@gitrepo.freebsd.org> The branch main has been updated by markj: URL: https://cgit.FreeBSD.org/src/commit/?id=3b93d3597cc336d5637e12ade8642d589cb0bc8a commit 3b93d3597cc336d5637e12ade8642d589cb0bc8a Author: Mark Johnston AuthorDate: 2026-08-10 14:41:33 +0000 Commit: Mark Johnston 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);