From 9033a84be53a2d22577914df0b14728cbdec4937 Mon Sep 17 00:00:00 2001
From: Dimitri Papadopoulos
 <3350651-DimitriPapadopoulos@users.noreply.gitlab.com>
Date: Tue, 23 Nov 2021 10:56:32 +0100
Subject: [PATCH] Windows: fix instability with Wintun as tunnel device driver

In https://gitlab.com/openconnect/openconnect/-/issues/338, multiple users
reported that connections using Wintun as the tunnel device driver become
non-functional after 20-30 minutes of operation, without
any message from OpenConnect at all.

We analyzed this issue as follows:

1. There was an off-by-one error in the check of outgoing packet size
   against the tunnel device's MTU (`tun_len < pkt->len`). Outgoing packets
   of exactly the MTU size would be considered errors and silently discarded
   by OpenConnect.
2. However, OpenConnect failed to instruct the driver to release these
   discarded packets. They would accumulate in the Wintun driver buffer and
   probably cause an out-of-memory condition, eventually freezing the
   driver.

We fixed the issued as follows:

1. Fix the off-by-one error, changing to `tun_len <= pkt->len`.
2. Always release outgoing packets, even if discarded.
3. Print extended debugging messages when receiving/sending packets. Such
   messages would have helped us diagnose the error much sooner.

Developers and users have confirmed that, with these changes, Wintun
connections run stably for at least 60 minutes.  (See
https://gitlab.com/openconnect/openconnect/-/merge_requests/306#note_745393731).

Also, in
https://gitlab.com/openconnect/openconnect/-/merge_requests/300?commit_id=b5ff6f3fb1b8d06cf56426b13c7af96e25cd922b,
we reverted to TAP-Windows as the default driver on Windows due to the
aforementioned stability issues.  Although Wintun connections now appear to
be stable, we are not quite ready yet to un/re-revert, and make Wintun the
default driver again.

Signed-off-by: Dimitri Papadopoulos <3350651-DimitriPapadopoulos@users.noreply.gitlab.com>
Signed-off-by: Daniel Lenski <dlenski@gmail.com>
---
 wintun.c | 39 ++++++++++++++++++++++++++++++---------
 1 file changed, 30 insertions(+), 9 deletions(-)

diff --git a/wintun.c b/wintun.c
index 79411841..5c544ab2 100644
--- a/wintun.c
+++ b/wintun.c
@@ -166,25 +166,46 @@ int os_read_wintun(struct openconnect_info *vpninfo, struct pkt *pkt)
 	DWORD tun_len;
 	BYTE *tun_pkt = WintunReceivePacket(vpninfo->wintun_session,
 					    &tun_len);
-	if (tun_pkt && tun_len < pkt->len) {
+	if (!tun_pkt) {
+		DWORD err = GetLastError();
+		if (err != ERROR_NO_MORE_ITEMS) {
+			const char *errstr = openconnect__win32_strerror(err);
+			vpn_progress(vpninfo, PRG_ERR, _("Could not retrieve packet from Wintun adapter '%S': %s\n"),
+				     vpninfo->ifname_w, errstr);
+			free(errstr);
+		}
+		return -1;
+	}
+
+	int ret = 0;
+	if (tun_len <= pkt->len) {
 		memcpy(pkt->data, tun_pkt, tun_len);
 		pkt->len = tun_len;
-		WintunReleaseReceivePacket(vpninfo->wintun_session, tun_pkt);
-		return 0;
+	} else {
+		vpn_progress(vpninfo, PRG_ERR, _("Drop oversized packet retrieved from Wintun adapter '%S' (%ld > %d)\n"),
+			     vpninfo->ifname_w, tun_len, pkt->len);
+		ret = -1;
 	}
-	return -1;
+	WintunReleaseReceivePacket(vpninfo->wintun_session, tun_pkt);
+	return ret;
 }
 
 int os_write_wintun(struct openconnect_info *vpninfo, struct pkt *pkt)
 {
 	BYTE *tun_pkt = WintunAllocateSendPacket(vpninfo->wintun_session,
 						 pkt->len);
-	if (tun_pkt) {
-		memcpy(tun_pkt, pkt->data, pkt->len);
-		WintunSendPacket(vpninfo->wintun_session, tun_pkt);
-		return 0;
+	if (!tun_pkt) {
+		DWORD err = GetLastError();
+		const char *errstr = openconnect__win32_strerror(err);
+		vpn_progress(vpninfo, PRG_ERR, _("Could not send packet through Wintun adapter '%S': %s\n"),
+			     vpninfo->ifname_w, errstr);
+		free(errstr);
+		return -1;
 	}
-	return -1;
+
+	memcpy(tun_pkt, pkt->data, pkt->len);
+	WintunSendPacket(vpninfo->wintun_session, tun_pkt);
+	return 0;
 }
 
 void os_shutdown_wintun(struct openconnect_info *vpninfo)
-- 
2.50.1