From patchwork Mon Feb 16 16:22:31 2026 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Gert Doering X-Patchwork-Id: 4769 Return-Path: Delivered-To: patchwork@openvpn.net Received: by 2002:a05:7001:ab03:b0:838:aef6:1aff with SMTP id xi3csp53800mab; Mon, 16 Feb 2026 08:33:19 -0800 (PST) X-Forwarded-Encrypted: i=2; AJvYcCVaQg+d9bCTZnb0ktA5PE4mP4/rAE8kVYvwifBcr6IV4FyrVySFw2/Q1ZqT1EHESzLQlwiokNAtyuo=@openvpn.net X-Received: by 2002:a05:6830:6288:b0:7cf:dd08:356b with SMTP id 46e09a7af769-7d4c4b5cc12mr7737307a34.38.1771259598979; Mon, 16 Feb 2026 08:33:18 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1771259598; cv=none; d=google.com; s=arc-20240605; b=bJ+aMcx0lhQvrLRwarKL73ou9D3MwpHqz9pB0b0MrZsMftLVEeXnfyrHAVHTLQS2XU 0p1DqkvLSeh4Ou84La/ClhC1bPVtDPZyGD14eYOVsyu4s2mGxOYMIeh9SGMUW4sbqilN wgY2chHVkouppjKeQnSzuLk9Cg/+dEQ5Q0J+nvxqbxFo5Atxe/FMz9HrS4cHKbXj7uDM 9rsyTF2DZ+qwcIzMlvzEA4C8JMabRZh4P7Him7GqO/aHaCHa1crlRSE7UduH3NX9uXlX 0arGG3ykDK7lYr/0HpfYXUGdkQuF6eWNGyd52dy21hyFs/mjN4ZLsUx7HIZu+fNRfPfU YyRA== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20240605; h=errors-to:content-transfer-encoding:list-subscribe:list-help :list-post:list-archive:list-unsubscribe:list-id:precedence:subject :mime-version:references:in-reply-to:message-id:date:to:from :dkim-signature:dkim-signature:dkim-signature; bh=SFw4R1tf/1IJbZYlGb4lAo8qefh/CEZ+ASELk5TBJmA=; fh=4NbAC/LsuMLI0S0hprUlLSLCiHwg6SCAifhH718Jh0Q=; b=e3GtfiWe7RXoZqr0PDl+wHUpzb8KabLV8I4qIlwB2cKANe1qT3BXd0q5bE1jFosQoA HITdhskPtJViVOm3Dy6zKOR0KLRwHMjTeHgneoVse124A95KgqsVD5A6ZZmi9JznoyJZ 0belCHEPBNg6VYSo8/VJ4zW90JpHiUGk6iqn5h06OJr8+7eHa2qkGyuUHQwDBSGZshUh FGHezrpbPf9K/3mdnr2OdL9yEUwkKtL3VRCp7Tl/NDq9AHdVQfvIY5oMpsPlI+GfK9BG XK05ykalIlSBqKbzB895hqJGEn7BZgAEoLEt0ML96K1hgWqEKit6jX9dPAGI5p/xXzi1 Xfqg==; dara=google.com ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@lists.sourceforge.net header.s=beta header.b=IAWKuWx9; dkim=neutral (body hash did not verify) header.i=@sourceforge.net header.s=x header.b=RWN36sRs; dkim=neutral (body hash did not verify) header.i=@sf.net header.s=x header.b=Ioq2Et8V; spf=pass (google.com: domain of openvpn-devel-bounces@lists.sourceforge.net designates 216.105.38.7 as permitted sender) smtp.mailfrom=openvpn-devel-bounces@lists.sourceforge.net; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=muc.de Received: from lists.sourceforge.net (lists.sourceforge.net. [216.105.38.7]) by mx.google.com with ESMTPS id 46e09a7af769-7d4c4e27461si6317760a34.20.2026.02.16.08.33.18 (version=TLS1_2 cipher=ECDHE-ECDSA-AES128-GCM-SHA256 bits=128/128); Mon, 16 Feb 2026 08:33:18 -0800 (PST) Received-SPF: pass (google.com: domain of openvpn-devel-bounces@lists.sourceforge.net designates 216.105.38.7 as permitted sender) client-ip=216.105.38.7; Authentication-Results: mx.google.com; dkim=pass header.i=@lists.sourceforge.net header.s=beta header.b=IAWKuWx9; dkim=neutral (body hash did not verify) header.i=@sourceforge.net header.s=x header.b=RWN36sRs; dkim=neutral (body hash did not verify) header.i=@sf.net header.s=x header.b=Ioq2Et8V; spf=pass (google.com: domain of openvpn-devel-bounces@lists.sourceforge.net designates 216.105.38.7 as permitted sender) smtp.mailfrom=openvpn-devel-bounces@lists.sourceforge.net; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=muc.de DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.sourceforge.net; s=beta; h=Content-Transfer-Encoding:Content-Type: List-Subscribe:List-Help:List-Post:List-Archive:List-Unsubscribe:List-Id: Subject:MIME-Version:References:In-Reply-To:Message-ID:Date:To:From:Sender: Reply-To:Cc:Content-ID:Content-Description:Resent-Date:Resent-From: Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=SFw4R1tf/1IJbZYlGb4lAo8qefh/CEZ+ASELk5TBJmA=; b=IAWKuWx97ckaWIBv8FFsqF4iZb tErdwXra0Gwla6jIRHECxsgfc8Ba9YVgSRrLqPfjtBZsIRa1WM3c7CmV4no4wilhgGTYwFqiEXyAl zpjWepzte1p4sTYi6S0WQZy576xbwWgKq1vMXwxcOW76AnoBckCmPCpsowr6t0U2P+iA=; Received: from [127.0.0.1] (helo=sfs-ml-1.v29.lw.sourceforge.com) by sfs-ml-1.v29.lw.sourceforge.com with esmtp (Exim 4.95) (envelope-from ) id 1vs1X6-00068b-Lc; Mon, 16 Feb 2026 16:33:12 +0000 Received: from [172.30.29.66] (helo=mx.sourceforge.net) by sfs-ml-1.v29.lw.sourceforge.com with esmtps (TLS1.2) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.95) (envelope-from ) id 1vs1X4-00068L-ST for openvpn-devel@lists.sourceforge.net; Mon, 16 Feb 2026 16:33:10 +0000 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=sourceforge.net; s=x; h=Content-Transfer-Encoding:MIME-Version:References: In-Reply-To:Message-ID:Date:Subject:To:From:Sender:Reply-To:Cc:Content-Type: Content-ID:Content-Description:Resent-Date:Resent-From:Resent-Sender: Resent-To:Resent-Cc:Resent-Message-ID:List-Id:List-Help:List-Unsubscribe: List-Subscribe:List-Post:List-Owner:List-Archive; bh=wVB326qxtL8OPPmzthEPDCYC16KFofGQSgLHPmk+uuQ=; b=RWN36sRsGsNPfeEUppvN6p4ux4 xu48Ro71b7AQlAvCRSsNYjKroHkxWQO9Zr/PdOqBYRdExAwWXMIsjQPvIkrd5OyxGMO7SGyuECe7Q gW6GfNaSzJvL1Fpbw8y1Hp9an5hmOwQrQeyTB5VI1pVUGQ4FBI9g0P9qc7ZL52ssUsJ4=; DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=sf.net; s=x ; h=Content-Transfer-Encoding:MIME-Version:References:In-Reply-To:Message-ID: Date:Subject:To:From:Sender:Reply-To:Cc:Content-Type:Content-ID: Content-Description:Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc :Resent-Message-ID:List-Id:List-Help:List-Unsubscribe:List-Subscribe: List-Post:List-Owner:List-Archive; bh=wVB326qxtL8OPPmzthEPDCYC16KFofGQSgLHPmk+uuQ=; b=Ioq2Et8VnhMdJHN/XwVtt/cm4E OqcmkNGWxKuoVluvCl5Hq/EqgfQTPRaXMnxwzcw+oV+Ebo+20WwA5DSmGGdDSzqZcVPEz9z7sOwB5 9mOCsn5a9Er85HQ/CYUJ1RJPcdjgZZ6FBsxApc2fY/3wVPSaPZtUgFhFcZXuZq6DxIBI=; Received: from [193.149.48.129] (helo=blue.greenie.muc.de) by sfi-mx-2.v28.lw.sourceforge.com with esmtps (TLS1.2:ECDHE-RSA-AES256-GCM-SHA384:256) (Exim 4.95) id 1vs1N3-0003Xh-6s for openvpn-devel@lists.sourceforge.net; Mon, 16 Feb 2026 16:22:50 +0000 Received: from blue.greenie.muc.de (localhost [127.0.0.1]) by blue.greenie.muc.de (8.18.1/8.18.1) with ESMTP id 61GGMb7a022319 for ; Mon, 16 Feb 2026 17:22:37 +0100 Received: (from gert@localhost) by blue.greenie.muc.de (8.18.1/8.18.1/Submit) id 61GGMbgq022318 for openvpn-devel@lists.sourceforge.net; Mon, 16 Feb 2026 17:22:37 +0100 From: Gert Doering To: openvpn-devel@lists.sourceforge.net Date: Mon, 16 Feb 2026 17:22:31 +0100 Message-ID: <20260216162236.22304-1-gert@greenie.muc.de> X-Mailer: git-send-email 2.52.0 In-Reply-To: References: MIME-Version: 1.0 X-Spam-Score: 1.3 (+) X-Spam-Report: Spam detection software, running on the system "sfi-spamd-2.hosts.colo.sdot.me", has NOT identified this incoming email as spam. The original message has been attached to this so you can view it or label similar future email. If you have any questions, see the administrator of that system for details. Content preview: From: Arne Schwabe The stream_buf_set_next prepares a buffer in the stream_buf structure that will be retrieved by stream_buf_get the next time it is used. This temporary copy of the buffer is unnecessary as the buffer next can also be constructed on the fly. Content analysis details: (1.3 points, 5.0 required) pts rule name description ---- ---------------------- -------------------------------------------------- 1.3 RDNS_NONE Delivered to internal network by a host with no rDNS X-Headers-End: 1vs1N3-0003Xh-6s Subject: [Openvpn-devel] [PATCH v7] Merge stream_buf_get_next and stream_buf_set_next X-BeenThere: openvpn-devel@lists.sourceforge.net X-Mailman-Version: 2.1.21 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: openvpn-devel-bounces@lists.sourceforge.net X-getmail-retrieved-from-mailbox: Inbox X-GMAIL-THRID: =?utf-8?q?1857300305220734524?= X-GMAIL-MSGID: =?utf-8?q?1857300305220734524?= From: Arne Schwabe The stream_buf_set_next prepares a buffer in the stream_buf structure that will be retrieved by stream_buf_get the next time it is used. This temporary copy of the buffer is unnecessary as the buffer next can also be constructed on the fly. This also fixes a rare crash when read buffer are not initialised and read is still signalled as the initialisation of next will now happen whenever it is required. This assertion happens when we do not expect a read event from the socket and then in link_socket_read_tcp the function stream_buf_get_next can trigger an assert on ASSERT(buf_defined(&sb->next)); To avoid this weird corner case, just always initialise the read buffer whether or not we expect a read to occur. This also adds documentation about the methods and field associated with the stream_buf structure. Reproducing this bug requires very special circumstances. The reproduction is openvpn --cert server-secp384r1.crt --key server-secp384r1.key \ --dev ml-tan --dev-type tap --tun-mtu 1400 --config ~/fp --topology subnet \ --port 2201 --verb 3 --mode server --tls-server --proto tcp6-server --ifconfig 10.0.0.1 255.255.255.0 as the server side and openvpn --client --proto tcp --remote poseidon --port 2201 --dev tap The client side must be on Linux. Other platforms do not reproduce this bug. Note that the client will not configure any IP or IPv6 on the interface and will also not bring up the interface. The server must also send at least one real data packet to the client (no keepalive ping). Just having the interface up normally produces enough traffic. Now forcefully reset the TCP connection. E.g. by executing on the server sudo ss --kill sport 2201 This will now trigger the assertion. This happens since OpenVPN waits forever to get a write back from the poll from the tun/tap device but this never happens since the device is not up. As long as we do not get back the tun device for writing, we also do not put the socket back into the EVENT_READ state. And this also means that code to initialise the read buffer (stream_buf_set_next) is never run. But the reset on the TCP socket triggers the TCP socket to be available for read, even if it is just for a read of 0 bytes to indicate the reset. So the function link_socket_read_tcp will run into the assert. Change-Id: Ifd3e953104a67c8bf2a225e179865e3dbd0dbfbc Signed-off-by: Arne Schwabe Acked-by: Frank Lichtenheld Gerrit URL: https://gerrit.openvpn.net/c/openvpn/+/1477 --- This change was reviewed on Gerrit and approved by at least one developer. I request to merge it to master. Gerrit URL: https://gerrit.openvpn.net/c/openvpn/+/1477 This mail reflects revision 7 of this Change. Signed-off-by line for the author was added as per our policy. Acked-by according to Gerrit (reflected above): Frank Lichtenheld diff --git a/src/openvpn/socket.c b/src/openvpn/socket.c index c463041..bdb98b9 100644 --- a/src/openvpn/socket.c +++ b/src/openvpn/socket.c @@ -44,9 +44,7 @@ bool sockets_read_residual(const struct context *c) { - int i; - - for (i = 0; i < c->c1.link_sockets_num; i++) + for (int i = 0; i < c->c1.link_sockets_num; i++) { if (c->c2.link_sockets[i]->stream_buf.residual_fully_formed) { @@ -2053,13 +2051,19 @@ * stream connection. */ +/** + * resets the stream buffer to be set up for the next round of + * reassembling a packet + * + * But still leaves the current packet in \c sb->buf to be potentially + * read. + */ static inline void stream_buf_reset(struct stream_buf *sb) { dmsg(D_STREAM_DEBUG, "STREAM: RESET"); sb->residual_fully_formed = false; sb->buf = sb->buf_init; - buf_reset(&sb->next); sb->len = -1; } @@ -2081,19 +2085,35 @@ dmsg(D_STREAM_DEBUG, "STREAM: INIT maxlen=%d", sb->maxlen); } -static inline void -stream_buf_set_next(struct stream_buf *sb) +/** + * Return a buffer that is backed by the same backend as sb->buf that + * determines where the next read should be done by also having the + * right offset into \c sb->buf. + * @param sb the stream buffer from which to construct the next buffer + */ +static inline struct buffer +stream_buf_get_next(struct stream_buf *sb) { /* set up 'next' for next i/o read */ - sb->next = sb->buf; - sb->next.offset = sb->buf.offset + sb->buf.len; - sb->next.len = (sb->len >= 0 ? sb->len : sb->maxlen) - sb->buf.len; - dmsg(D_STREAM_DEBUG, "STREAM: SET NEXT, buf=[%d,%d] next=[%d,%d] len=%d maxlen=%d", - sb->buf.offset, sb->buf.len, sb->next.offset, sb->next.len, sb->len, sb->maxlen); - ASSERT(sb->next.len > 0); - ASSERT(buf_safe(&sb->buf, sb->next.len)); + struct buffer next; + next = sb->buf; + next.offset = sb->buf.offset + sb->buf.len; + next.len = (sb->len >= 0 ? sb->len : sb->maxlen) - sb->buf.len; + dmsg(D_STREAM_DEBUG, "STREAM: GET NEXT, buf=[%d,%d] next=[%d,%d] len=%d maxlen=%d", + sb->buf.offset, sb->buf.len, next.offset, next.len, sb->len, sb->maxlen); + ASSERT(next.len > 0); + ASSERT(buf_safe(&sb->buf, next.len)); + return next; } +/** + * Sets the parameter buf to the current buffer of \c sb->buf. + * This function assumes that caller already checked if the packet in \c sb->buf + * is fully assembled. + * + * @param sb stream buffer to operate on + * @param buf buffer to point to the contents of buf + */ static inline void stream_buf_get_final(struct stream_buf *sb, struct buffer *buf) { @@ -2102,14 +2122,6 @@ *buf = sb->buf; } -static inline void -stream_buf_get_next(struct stream_buf *sb, struct buffer *buf) -{ - dmsg(D_STREAM_DEBUG, "STREAM: GET NEXT len=%d", buf_defined(&sb->next) ? sb->next.len : -1); - ASSERT(buf_defined(&sb->next)); - *buf = sb->next; -} - bool stream_buf_read_setup_dowork(struct stream_buf *sb) { @@ -2122,13 +2134,30 @@ sb->residual_fully_formed ? "YES" : "NO", sb->residual.len); } - if (!sb->residual_fully_formed) - { - stream_buf_set_next(sb); - } return !sb->residual_fully_formed; } +/** + * This will determine if \c sb->buf contains a full packet. It will also + * move anything in \c sb->buf beyond a full packet to \c sb->residual. + * + * The first time the function is called with a valid buffer and port sharing + * is enabled, the function will also determine if the buffer contains + * OpenVPN protocol data and store the result in \c sb->port_share_state. + * + * If a packet outside the allowed range is detected, the error state + * on \c sb is set. + * + * Since the buffer in \c sb->buf is modified from the outside (via + * \c stream_buf_get_next) the parameter \p length_added needs to be set + * to the amount of bytes that have been written to this buffer. If the + * buffer was not modified but should still be analysed and potentially + * split to \c sb->residual, the parameter \p length_added should be 0. + * + * @param sb the stream buffer + * @param length_added The length that has been added to \c sb->buf + * @return true if \c sb->buf contains fully reassembled packet + */ static bool stream_buf_added(struct stream_buf *sb, int length_added) { @@ -2191,7 +2220,6 @@ else { dmsg(D_STREAM_DEBUG, "STREAM: ADD returned FALSE (have=%d need=%d)", sb->buf.len, sb->len); - stream_buf_set_next(sb); return false; } } @@ -2261,8 +2289,7 @@ sockethandle_t sh = { .s = sock->sd }; len = sockethandle_finalize(sh, &sock->reads, buf, NULL); #else - struct buffer frag; - stream_buf_get_next(&sock->stream_buf, &frag); + struct buffer frag = stream_buf_get_next(&sock->stream_buf); len = recv(sock->sd, BPTR(&frag), BLEN(&frag), MSG_NOSIGNAL); #endif @@ -2538,7 +2565,7 @@ } else if (proto_is_tcp(sock->info.proto)) { - stream_buf_get_next(&sock->stream_buf, &sock->reads.buf); + sock->reads.buf = stream_buf_get_next(&sock->stream_buf); } else { diff --git a/src/openvpn/socket.h b/src/openvpn/socket.h index 3f46dc6..ebaf776 100644 --- a/src/openvpn/socket.h +++ b/src/openvpn/socket.h @@ -95,23 +95,41 @@ int mtu_changed; /* Set to true when mtu value is changed */ }; -/* - * Used to extract packets encapsulated in streams into a buffer, - * in this case IP packets embedded in a TCP stream. +/** + * struct used to extract packets encapsulated in streams into a buffer, + * in this case OpenVPN packets (data or control) embedded in a TCP stream. + * + * This struct is used to packetise the TCP stream into the + * OpenVPN packet. Each OpenVPN packet has a two-byte header determining + * the length of the packet. */ struct stream_buf { + /* Buffer to hold the initial buffer that will be used to reset buf */ struct buffer buf_init; + + /** buffer holding the excess bytes that are not part of the + * packet. */ struct buffer residual; + + /** Maximum length of a packet that we accept */ int maxlen; + + /** The buffer in buf contains a full packet without a header. Any + * extra data is in residual */ bool residual_fully_formed; + /** Holds the data of the current packet. This might be a partial packet */ struct buffer buf; - struct buffer next; - int len; /* -1 if not yet known */ - bool error; /* if true, fatal TCP error has occurred, - * requiring that connection be restarted */ + /** -1 if not yet known. Otherwise holds the length of the + * packet. If >= 0, buf is already moved past the initial + * size header */ + int len; + + /** if true, a fatal TCP error has occurred, + * requiring that connection be restarted */ + bool error; #if PORT_SHARE #define PS_DISABLED 0 #define PS_ENABLED 1