* [Openvpn-devel] [S] Change in openvpn[master]: dco-win: Fix crash when cancelling pending operation
[not found] <gerrit.1743515497000.Ib1db457c42a80f6b8fc0e3ceb4a895d4cf7f0155@...2715...>
@ 2025-04-01 13:51 ` stipa (Code Review)
2025-04-01 18:15 ` cron2 (Code Review)
` (4 subsequent siblings)
5 siblings, 0 replies; 7+ messages in thread
From: stipa (Code Review) @ 2025-04-01 13:51 UTC (permalink / raw)
To: plaisthos <arne-openvpn@; +Cc: openvpn-devel
[-- Attachment #1: Type: text/plain, Size: 4420 bytes --]
Attention is currently required from: flichtenheld, plaisthos.
Hello plaisthos, flichtenheld,
I'd like you to do a code review.
Please visit
http://gerrit.openvpn.net/c/openvpn/+/928?usp=email
to review the following change.
Change subject: dco-win: Fix crash when cancelling pending operation
......................................................................
dco-win: Fix crash when cancelling pending operation
The OVERLAPPED structure must remain valid for the entire duration of an
asynchronous operation. Previously, when a TCP connection was pending
inside the NEW_PEER call, the OVERLAPPED structure was defined as a
local variable within dco_p2p_new_peer().
When CancelIo() was called later from close_tun_handle(), the OVERLAPPED
structure was already out of scope, resulting in undefined behavior and
stack corruption.
This fix moves the OVERLAPPED structure to the tuntap struct, ensuring
it remains valid throughout the operation's lifetime.
GitHub: #715
Change-Id: Ib1db457c42a80f6b8fc0e3ceb4a895d4cf7f0155
Signed-off-by: Lev Stipakov <lev@...515...>
---
M src/openvpn/dco_win.c
M src/openvpn/dco_win.h
M src/openvpn/socket.c
M src/openvpn/tun.h
4 files changed, 7 insertions(+), 6 deletions(-)
git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/28/928/1
diff --git a/src/openvpn/dco_win.c b/src/openvpn/dco_win.c
index 8b47124..a386e53 100644
--- a/src/openvpn/dco_win.c
+++ b/src/openvpn/dco_win.c
@@ -321,7 +321,7 @@
}
void
-dco_p2p_new_peer(HANDLE handle, struct link_socket *sock, struct signal_info *sig_info)
+dco_p2p_new_peer(HANDLE handle, OVERLAPPED *ov, struct link_socket *sock, struct signal_info *sig_info)
{
msg(D_DCO_DEBUG, "%s", __func__);
@@ -395,8 +395,8 @@
ASSERT(0);
}
- OVERLAPPED ov = { 0 };
- if (!DeviceIoControl(handle, OVPN_IOCTL_NEW_PEER, &peer, sizeof(peer), NULL, 0, NULL, &ov))
+ CLEAR(*ov);
+ if (!DeviceIoControl(handle, OVPN_IOCTL_NEW_PEER, &peer, sizeof(peer), NULL, 0, NULL, ov))
{
DWORD err = GetLastError();
if (err != ERROR_IO_PENDING)
@@ -405,7 +405,7 @@
}
else
{
- dco_connect_wait(handle, &ov, get_server_poll_remaining_time(sock->server_poll_timeout), sig_info);
+ dco_connect_wait(handle, ov, get_server_poll_remaining_time(sock->server_poll_timeout), sig_info);
}
}
}
diff --git a/src/openvpn/dco_win.h b/src/openvpn/dco_win.h
index 95c95c8..e8e4e22 100644
--- a/src/openvpn/dco_win.h
+++ b/src/openvpn/dco_win.h
@@ -63,7 +63,7 @@
dco_mp_start_vpn(HANDLE handle, struct link_socket *sock);
void
-dco_p2p_new_peer(HANDLE handle, struct link_socket *sock, struct signal_info *sig_info);
+dco_p2p_new_peer(HANDLE handle, OVERLAPPED *ov, struct link_socket *sock, struct signal_info *sig_info);
void
dco_start_tun(struct tuntap *tt);
diff --git a/src/openvpn/socket.c b/src/openvpn/socket.c
index 09de1b0..beb31fa 100644
--- a/src/openvpn/socket.c
+++ b/src/openvpn/socket.c
@@ -2242,7 +2242,7 @@
}
else
{
- dco_p2p_new_peer(c->c1.tuntap->hand, sock, sig_info);
+ dco_p2p_new_peer(c->c1.tuntap->hand, &c->c1.tuntap->dco_new_peer_ov, sock, sig_info);
}
sock->sockflags |= SF_DCO_WIN;
diff --git a/src/openvpn/tun.h b/src/openvpn/tun.h
index b616f5d..bcc23b4 100644
--- a/src/openvpn/tun.h
+++ b/src/openvpn/tun.h
@@ -215,6 +215,7 @@
#ifdef _WIN32
HANDLE hand;
+ OVERLAPPED dco_new_peer_ov; /* used for async NEW_PEER dco call, which might wait for TCP connect */
struct overlapped_io reads;
struct overlapped_io writes;
struct rw_handle rw_handle;
--
To view, visit http://gerrit.openvpn.net/c/openvpn/+/928?usp=email
To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings
Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: Ib1db457c42a80f6b8fc0e3ceb4a895d4cf7f0155
Gerrit-Change-Number: 928
Gerrit-PatchSet: 1
Gerrit-Owner: stipa <lstipakov@...277...>
Gerrit-Reviewer: flichtenheld <frank@...2641...>
Gerrit-Reviewer: plaisthos <arne-openvpn@...1227...>
Gerrit-CC: openvpn-devel <openvpn-devel@lists.sourceforge.net>
Gerrit-Attention: plaisthos <arne-openvpn@...1227...>
Gerrit-Attention: flichtenheld <frank@...2641...>
Gerrit-MessageType: newchange
[-- Attachment #2: Type: text/html, Size: 6954 bytes --]
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [Openvpn-devel] [S] Change in openvpn[master]: dco-win: Fix crash when cancelling pending operation
[not found] <gerrit.1743515497000.Ib1db457c42a80f6b8fc0e3ceb4a895d4cf7f0155@...2715...>
2025-04-01 13:51 ` [Openvpn-devel] [S] Change in openvpn[master]: dco-win: Fix crash when cancelling pending operation stipa (Code Review)
@ 2025-04-01 18:15 ` cron2 (Code Review)
2025-04-01 18:15 ` [Openvpn-devel] [PATCH v1] " Gert Doering
` (3 subsequent siblings)
5 siblings, 0 replies; 7+ messages in thread
From: cron2 (Code Review) @ 2025-04-01 18:15 UTC (permalink / raw)
To: stipa <lstipakov@; +Cc: plaisthos <arne-openvpn@
[-- Attachment #1: Type: text/plain, Size: 1265 bytes --]
Attention is currently required from: flichtenheld, plaisthos, stipa.
cron2 has posted comments on this change. ( http://gerrit.openvpn.net/c/openvpn/+/928?usp=email )
Change subject: dco-win: Fix crash when cancelling pending operation
......................................................................
Patch Set 1: Code-Review+2
(1 comment)
Patchset:
PS1:
good catch...
--
To view, visit http://gerrit.openvpn.net/c/openvpn/+/928?usp=email
To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings
Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: Ib1db457c42a80f6b8fc0e3ceb4a895d4cf7f0155
Gerrit-Change-Number: 928
Gerrit-PatchSet: 1
Gerrit-Owner: stipa <lstipakov@...277...>
Gerrit-Reviewer: cron2 <gert@...1296...>
Gerrit-Reviewer: flichtenheld <frank@...2641...>
Gerrit-Reviewer: plaisthos <arne-openvpn@...1227...>
Gerrit-CC: openvpn-devel <openvpn-devel@lists.sourceforge.net>
Gerrit-Attention: plaisthos <arne-openvpn@...1227...>
Gerrit-Attention: flichtenheld <frank@...2641...>
Gerrit-Attention: stipa <lstipakov@...277...>
Gerrit-Comment-Date: Tue, 01 Apr 2025 18:15:15 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
[-- Attachment #2: Type: text/html, Size: 2566 bytes --]
^ permalink raw reply [flat|nested] 7+ messages in thread
* [Openvpn-devel] [PATCH v1] dco-win: Fix crash when cancelling pending operation
[not found] <gerrit.1743515497000.Ib1db457c42a80f6b8fc0e3ceb4a895d4cf7f0155@...2715...>
2025-04-01 13:51 ` [Openvpn-devel] [S] Change in openvpn[master]: dco-win: Fix crash when cancelling pending operation stipa (Code Review)
2025-04-01 18:15 ` cron2 (Code Review)
@ 2025-04-01 18:15 ` Gert Doering
2025-04-01 20:52 ` [Openvpn-devel] [PATCH applied] " Gert Doering
2025-04-01 20:47 ` [Openvpn-devel] [PATCH v1] " Gert Doering
` (2 subsequent siblings)
5 siblings, 1 reply; 7+ messages in thread
From: Gert Doering @ 2025-04-01 18:15 UTC (permalink / raw)
To: openvpn-devel
From: Lev Stipakov <lev@...515...>
The OVERLAPPED structure must remain valid for the entire duration of an
asynchronous operation. Previously, when a TCP connection was pending
inside the NEW_PEER call, the OVERLAPPED structure was defined as a
local variable within dco_p2p_new_peer().
When CancelIo() was called later from close_tun_handle(), the OVERLAPPED
structure was already out of scope, resulting in undefined behavior and
stack corruption.
This fix moves the OVERLAPPED structure to the tuntap struct, ensuring
it remains valid throughout the operation's lifetime.
GitHub: #715
Change-Id: Ib1db457c42a80f6b8fc0e3ceb4a895d4cf7f0155
Signed-off-by: Lev Stipakov <lev@...515...>
Acked-by: Gert Doering <gert@...1296...>
---
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/+/928
This mail reflects revision 1 of this Change.
Acked-by according to Gerrit (reflected above):
Gert Doering <gert@...1296...>
diff --git a/src/openvpn/dco_win.c b/src/openvpn/dco_win.c
index 8b47124..a386e53 100644
--- a/src/openvpn/dco_win.c
+++ b/src/openvpn/dco_win.c
@@ -321,7 +321,7 @@
}
void
-dco_p2p_new_peer(HANDLE handle, struct link_socket *sock, struct signal_info *sig_info)
+dco_p2p_new_peer(HANDLE handle, OVERLAPPED *ov, struct link_socket *sock, struct signal_info *sig_info)
{
msg(D_DCO_DEBUG, "%s", __func__);
@@ -395,8 +395,8 @@
ASSERT(0);
}
- OVERLAPPED ov = { 0 };
- if (!DeviceIoControl(handle, OVPN_IOCTL_NEW_PEER, &peer, sizeof(peer), NULL, 0, NULL, &ov))
+ CLEAR(*ov);
+ if (!DeviceIoControl(handle, OVPN_IOCTL_NEW_PEER, &peer, sizeof(peer), NULL, 0, NULL, ov))
{
DWORD err = GetLastError();
if (err != ERROR_IO_PENDING)
@@ -405,7 +405,7 @@
}
else
{
- dco_connect_wait(handle, &ov, get_server_poll_remaining_time(sock->server_poll_timeout), sig_info);
+ dco_connect_wait(handle, ov, get_server_poll_remaining_time(sock->server_poll_timeout), sig_info);
}
}
}
diff --git a/src/openvpn/dco_win.h b/src/openvpn/dco_win.h
index 95c95c8..e8e4e22 100644
--- a/src/openvpn/dco_win.h
+++ b/src/openvpn/dco_win.h
@@ -63,7 +63,7 @@
dco_mp_start_vpn(HANDLE handle, struct link_socket *sock);
void
-dco_p2p_new_peer(HANDLE handle, struct link_socket *sock, struct signal_info *sig_info);
+dco_p2p_new_peer(HANDLE handle, OVERLAPPED *ov, struct link_socket *sock, struct signal_info *sig_info);
void
dco_start_tun(struct tuntap *tt);
diff --git a/src/openvpn/socket.c b/src/openvpn/socket.c
index 09de1b0..beb31fa 100644
--- a/src/openvpn/socket.c
+++ b/src/openvpn/socket.c
@@ -2242,7 +2242,7 @@
}
else
{
- dco_p2p_new_peer(c->c1.tuntap->hand, sock, sig_info);
+ dco_p2p_new_peer(c->c1.tuntap->hand, &c->c1.tuntap->dco_new_peer_ov, sock, sig_info);
}
sock->sockflags |= SF_DCO_WIN;
diff --git a/src/openvpn/tun.h b/src/openvpn/tun.h
index b616f5d..bcc23b4 100644
--- a/src/openvpn/tun.h
+++ b/src/openvpn/tun.h
@@ -215,6 +215,7 @@
#ifdef _WIN32
HANDLE hand;
+ OVERLAPPED dco_new_peer_ov; /* used for async NEW_PEER dco call, which might wait for TCP connect */
struct overlapped_io reads;
struct overlapped_io writes;
struct rw_handle rw_handle;
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [Openvpn-devel] [PATCH v1] dco-win: Fix crash when cancelling pending operation
[not found] <gerrit.1743515497000.Ib1db457c42a80f6b8fc0e3ceb4a895d4cf7f0155@...2715...>
` (2 preceding siblings ...)
2025-04-01 18:15 ` [Openvpn-devel] [PATCH v1] " Gert Doering
@ 2025-04-01 20:47 ` Gert Doering
2025-04-01 20:53 ` [Openvpn-devel] [S] Change in openvpn[master]: " cron2 (Code Review)
2025-04-01 20:53 ` cron2 (Code Review)
5 siblings, 0 replies; 7+ messages in thread
From: Gert Doering @ 2025-04-01 20:47 UTC (permalink / raw)
To: openvpn-devel
From: Lev Stipakov <lev@...515...>
The OVERLAPPED structure must remain valid for the entire duration of an
asynchronous operation. Previously, when a TCP connection was pending
inside the NEW_PEER call, the OVERLAPPED structure was defined as a
local variable within dco_p2p_new_peer().
When CancelIo() was called later from close_tun_handle(), the OVERLAPPED
structure was already out of scope, resulting in undefined behavior and
stack corruption.
This fix moves the OVERLAPPED structure to the tuntap struct, ensuring
it remains valid throughout the operation's lifetime.
GitHub: #715
Change-Id: Ib1db457c42a80f6b8fc0e3ceb4a895d4cf7f0155
Signed-off-by: Lev Stipakov <lev@...515...>
Acked-by: Gert Doering <gert@...1296...>
---
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/+/928
This mail reflects revision 1 of this Change.
Acked-by according to Gerrit (reflected above):
Gert Doering <gert@...1296...>
diff --git a/src/openvpn/dco_win.c b/src/openvpn/dco_win.c
index 8b47124..a386e53 100644
--- a/src/openvpn/dco_win.c
+++ b/src/openvpn/dco_win.c
@@ -321,7 +321,7 @@
}
void
-dco_p2p_new_peer(HANDLE handle, struct link_socket *sock, struct signal_info *sig_info)
+dco_p2p_new_peer(HANDLE handle, OVERLAPPED *ov, struct link_socket *sock, struct signal_info *sig_info)
{
msg(D_DCO_DEBUG, "%s", __func__);
@@ -395,8 +395,8 @@
ASSERT(0);
}
- OVERLAPPED ov = { 0 };
- if (!DeviceIoControl(handle, OVPN_IOCTL_NEW_PEER, &peer, sizeof(peer), NULL, 0, NULL, &ov))
+ CLEAR(*ov);
+ if (!DeviceIoControl(handle, OVPN_IOCTL_NEW_PEER, &peer, sizeof(peer), NULL, 0, NULL, ov))
{
DWORD err = GetLastError();
if (err != ERROR_IO_PENDING)
@@ -405,7 +405,7 @@
}
else
{
- dco_connect_wait(handle, &ov, get_server_poll_remaining_time(sock->server_poll_timeout), sig_info);
+ dco_connect_wait(handle, ov, get_server_poll_remaining_time(sock->server_poll_timeout), sig_info);
}
}
}
diff --git a/src/openvpn/dco_win.h b/src/openvpn/dco_win.h
index 95c95c8..e8e4e22 100644
--- a/src/openvpn/dco_win.h
+++ b/src/openvpn/dco_win.h
@@ -63,7 +63,7 @@
dco_mp_start_vpn(HANDLE handle, struct link_socket *sock);
void
-dco_p2p_new_peer(HANDLE handle, struct link_socket *sock, struct signal_info *sig_info);
+dco_p2p_new_peer(HANDLE handle, OVERLAPPED *ov, struct link_socket *sock, struct signal_info *sig_info);
void
dco_start_tun(struct tuntap *tt);
diff --git a/src/openvpn/socket.c b/src/openvpn/socket.c
index 09de1b0..beb31fa 100644
--- a/src/openvpn/socket.c
+++ b/src/openvpn/socket.c
@@ -2242,7 +2242,7 @@
}
else
{
- dco_p2p_new_peer(c->c1.tuntap->hand, sock, sig_info);
+ dco_p2p_new_peer(c->c1.tuntap->hand, &c->c1.tuntap->dco_new_peer_ov, sock, sig_info);
}
sock->sockflags |= SF_DCO_WIN;
diff --git a/src/openvpn/tun.h b/src/openvpn/tun.h
index b616f5d..bcc23b4 100644
--- a/src/openvpn/tun.h
+++ b/src/openvpn/tun.h
@@ -215,6 +215,7 @@
#ifdef _WIN32
HANDLE hand;
+ OVERLAPPED dco_new_peer_ov; /* used for async NEW_PEER dco call, which might wait for TCP connect */
struct overlapped_io reads;
struct overlapped_io writes;
struct rw_handle rw_handle;
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [Openvpn-devel] [PATCH applied] Re: dco-win: Fix crash when cancelling pending operation
2025-04-01 18:15 ` [Openvpn-devel] [PATCH v1] " Gert Doering
@ 2025-04-01 20:52 ` Gert Doering
0 siblings, 0 replies; 7+ messages in thread
From: Gert Doering @ 2025-04-01 20:52 UTC (permalink / raw)
To: Lev Stipakov <lev@; +Cc: openvpn-devel
I haven't tested it, just stared at the code, and test compiled with
mingw. The change itself is straightforward - move the OVERLAPPED
structure inside struct tuntap, so it will share the same lifetime
(diagnosing this from a crash trying to close tt->hand is more
impressive ;-) )
Reference URLs point to the sf.net list archive again, because
mail-archive.org is refusing to acknowledge existence of this mail...
Your patch has been applied to the master branch.
commit f60a49362515a87ccf8db406ef422499adf34eb7
Author: Lev Stipakov
Date: Tue Apr 1 20:15:30 2025 +0200
dco-win: Fix crash when cancelling pending operation
Signed-off-by: Lev Stipakov <lev@...515...>
Acked-by: Gert Doering <gert@...1296...>
Message-Id: <20250401181535.7854-1-gert@...1296...>
URL: https://sourceforge.net/p/openvpn/mailman/message/59168247/
URL: https://gerrit.openvpn.net/c/openvpn/+/928
Signed-off-by: Gert Doering <gert@...1296...>
--
kind regards,
Gert Doering
^ permalink raw reply [flat|nested] 7+ messages in thread
* [Openvpn-devel] [S] Change in openvpn[master]: dco-win: Fix crash when cancelling pending operation
[not found] <gerrit.1743515497000.Ib1db457c42a80f6b8fc0e3ceb4a895d4cf7f0155@...2715...>
` (3 preceding siblings ...)
2025-04-01 20:47 ` [Openvpn-devel] [PATCH v1] " Gert Doering
@ 2025-04-01 20:53 ` cron2 (Code Review)
2025-04-01 20:53 ` cron2 (Code Review)
5 siblings, 0 replies; 7+ messages in thread
From: cron2 (Code Review) @ 2025-04-01 20:53 UTC (permalink / raw)
To: stipa <lstipakov@; +Cc: openvpn-devel
[-- Attachment #1: Type: text/plain, Size: 4615 bytes --]
cron2 has uploaded a new patch set (#2) to the change originally created by stipa. ( http://gerrit.openvpn.net/c/openvpn/+/928?usp=email )
The following approvals got outdated and were removed:
Code-Review+2 by cron2
Change subject: dco-win: Fix crash when cancelling pending operation
......................................................................
dco-win: Fix crash when cancelling pending operation
The OVERLAPPED structure must remain valid for the entire duration of an
asynchronous operation. Previously, when a TCP connection was pending
inside the NEW_PEER call, the OVERLAPPED structure was defined as a
local variable within dco_p2p_new_peer().
When CancelIo() was called later from close_tun_handle(), the OVERLAPPED
structure was already out of scope, resulting in undefined behavior and
stack corruption.
This fix moves the OVERLAPPED structure to the tuntap struct, ensuring
it remains valid throughout the operation's lifetime.
Github: closes OpenVPN/openvpn#715
Change-Id: Ib1db457c42a80f6b8fc0e3ceb4a895d4cf7f0155
Signed-off-by: Lev Stipakov <lev@...515...>
Acked-by: Gert Doering <gert@...1296...>
Message-Id: <20250401181535.7854-1-gert@...1296...>
URL: https://sourceforge.net/p/openvpn/mailman/message/59168247/
URL: https://gerrit.openvpn.net/c/openvpn/+/928
Signed-off-by: Gert Doering <gert@...1296...>
---
M src/openvpn/dco_win.c
M src/openvpn/dco_win.h
M src/openvpn/socket.c
M src/openvpn/tun.h
4 files changed, 7 insertions(+), 6 deletions(-)
git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/28/928/2
diff --git a/src/openvpn/dco_win.c b/src/openvpn/dco_win.c
index 8b47124..a386e53 100644
--- a/src/openvpn/dco_win.c
+++ b/src/openvpn/dco_win.c
@@ -321,7 +321,7 @@
}
void
-dco_p2p_new_peer(HANDLE handle, struct link_socket *sock, struct signal_info *sig_info)
+dco_p2p_new_peer(HANDLE handle, OVERLAPPED *ov, struct link_socket *sock, struct signal_info *sig_info)
{
msg(D_DCO_DEBUG, "%s", __func__);
@@ -395,8 +395,8 @@
ASSERT(0);
}
- OVERLAPPED ov = { 0 };
- if (!DeviceIoControl(handle, OVPN_IOCTL_NEW_PEER, &peer, sizeof(peer), NULL, 0, NULL, &ov))
+ CLEAR(*ov);
+ if (!DeviceIoControl(handle, OVPN_IOCTL_NEW_PEER, &peer, sizeof(peer), NULL, 0, NULL, ov))
{
DWORD err = GetLastError();
if (err != ERROR_IO_PENDING)
@@ -405,7 +405,7 @@
}
else
{
- dco_connect_wait(handle, &ov, get_server_poll_remaining_time(sock->server_poll_timeout), sig_info);
+ dco_connect_wait(handle, ov, get_server_poll_remaining_time(sock->server_poll_timeout), sig_info);
}
}
}
diff --git a/src/openvpn/dco_win.h b/src/openvpn/dco_win.h
index 95c95c8..e8e4e22 100644
--- a/src/openvpn/dco_win.h
+++ b/src/openvpn/dco_win.h
@@ -63,7 +63,7 @@
dco_mp_start_vpn(HANDLE handle, struct link_socket *sock);
void
-dco_p2p_new_peer(HANDLE handle, struct link_socket *sock, struct signal_info *sig_info);
+dco_p2p_new_peer(HANDLE handle, OVERLAPPED *ov, struct link_socket *sock, struct signal_info *sig_info);
void
dco_start_tun(struct tuntap *tt);
diff --git a/src/openvpn/socket.c b/src/openvpn/socket.c
index 4fb6fe6..bd7a449 100644
--- a/src/openvpn/socket.c
+++ b/src/openvpn/socket.c
@@ -2247,7 +2247,7 @@
}
else
{
- dco_p2p_new_peer(c->c1.tuntap->hand, sock, sig_info);
+ dco_p2p_new_peer(c->c1.tuntap->hand, &c->c1.tuntap->dco_new_peer_ov, sock, sig_info);
}
sock->sockflags |= SF_DCO_WIN;
diff --git a/src/openvpn/tun.h b/src/openvpn/tun.h
index b616f5d..bcc23b4 100644
--- a/src/openvpn/tun.h
+++ b/src/openvpn/tun.h
@@ -215,6 +215,7 @@
#ifdef _WIN32
HANDLE hand;
+ OVERLAPPED dco_new_peer_ov; /* used for async NEW_PEER dco call, which might wait for TCP connect */
struct overlapped_io reads;
struct overlapped_io writes;
struct rw_handle rw_handle;
--
To view, visit http://gerrit.openvpn.net/c/openvpn/+/928?usp=email
To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings
Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: Ib1db457c42a80f6b8fc0e3ceb4a895d4cf7f0155
Gerrit-Change-Number: 928
Gerrit-PatchSet: 2
Gerrit-Owner: stipa <lstipakov@...277...>
Gerrit-Reviewer: cron2 <gert@...1296...>
Gerrit-Reviewer: flichtenheld <frank@...2641...>
Gerrit-Reviewer: plaisthos <arne-openvpn@...1227...>
Gerrit-CC: openvpn-devel <openvpn-devel@lists.sourceforge.net>
Gerrit-MessageType: newpatchset
[-- Attachment #2: Type: text/html, Size: 7160 bytes --]
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [Openvpn-devel] [S] Change in openvpn[master]: dco-win: Fix crash when cancelling pending operation
[not found] <gerrit.1743515497000.Ib1db457c42a80f6b8fc0e3ceb4a895d4cf7f0155@...2715...>
` (4 preceding siblings ...)
2025-04-01 20:53 ` [Openvpn-devel] [S] Change in openvpn[master]: " cron2 (Code Review)
@ 2025-04-01 20:53 ` cron2 (Code Review)
5 siblings, 0 replies; 7+ messages in thread
From: cron2 (Code Review) @ 2025-04-01 20:53 UTC (permalink / raw)
To: stipa <lstipakov@; +Cc: plaisthos <arne-openvpn@
[-- Attachment #1: Type: text/plain, Size: 4404 bytes --]
cron2 has submitted this change. ( http://gerrit.openvpn.net/c/openvpn/+/928?usp=email )
Change subject: dco-win: Fix crash when cancelling pending operation
......................................................................
dco-win: Fix crash when cancelling pending operation
The OVERLAPPED structure must remain valid for the entire duration of an
asynchronous operation. Previously, when a TCP connection was pending
inside the NEW_PEER call, the OVERLAPPED structure was defined as a
local variable within dco_p2p_new_peer().
When CancelIo() was called later from close_tun_handle(), the OVERLAPPED
structure was already out of scope, resulting in undefined behavior and
stack corruption.
This fix moves the OVERLAPPED structure to the tuntap struct, ensuring
it remains valid throughout the operation's lifetime.
Github: closes OpenVPN/openvpn#715
Change-Id: Ib1db457c42a80f6b8fc0e3ceb4a895d4cf7f0155
Signed-off-by: Lev Stipakov <lev@...515...>
Acked-by: Gert Doering <gert@...1296...>
Message-Id: <20250401181535.7854-1-gert@...1296...>
URL: https://sourceforge.net/p/openvpn/mailman/message/59168247/
URL: https://gerrit.openvpn.net/c/openvpn/+/928
Signed-off-by: Gert Doering <gert@...1296...>
---
M src/openvpn/dco_win.c
M src/openvpn/dco_win.h
M src/openvpn/socket.c
M src/openvpn/tun.h
4 files changed, 7 insertions(+), 6 deletions(-)
diff --git a/src/openvpn/dco_win.c b/src/openvpn/dco_win.c
index 8b47124..a386e53 100644
--- a/src/openvpn/dco_win.c
+++ b/src/openvpn/dco_win.c
@@ -321,7 +321,7 @@
}
void
-dco_p2p_new_peer(HANDLE handle, struct link_socket *sock, struct signal_info *sig_info)
+dco_p2p_new_peer(HANDLE handle, OVERLAPPED *ov, struct link_socket *sock, struct signal_info *sig_info)
{
msg(D_DCO_DEBUG, "%s", __func__);
@@ -395,8 +395,8 @@
ASSERT(0);
}
- OVERLAPPED ov = { 0 };
- if (!DeviceIoControl(handle, OVPN_IOCTL_NEW_PEER, &peer, sizeof(peer), NULL, 0, NULL, &ov))
+ CLEAR(*ov);
+ if (!DeviceIoControl(handle, OVPN_IOCTL_NEW_PEER, &peer, sizeof(peer), NULL, 0, NULL, ov))
{
DWORD err = GetLastError();
if (err != ERROR_IO_PENDING)
@@ -405,7 +405,7 @@
}
else
{
- dco_connect_wait(handle, &ov, get_server_poll_remaining_time(sock->server_poll_timeout), sig_info);
+ dco_connect_wait(handle, ov, get_server_poll_remaining_time(sock->server_poll_timeout), sig_info);
}
}
}
diff --git a/src/openvpn/dco_win.h b/src/openvpn/dco_win.h
index 95c95c8..e8e4e22 100644
--- a/src/openvpn/dco_win.h
+++ b/src/openvpn/dco_win.h
@@ -63,7 +63,7 @@
dco_mp_start_vpn(HANDLE handle, struct link_socket *sock);
void
-dco_p2p_new_peer(HANDLE handle, struct link_socket *sock, struct signal_info *sig_info);
+dco_p2p_new_peer(HANDLE handle, OVERLAPPED *ov, struct link_socket *sock, struct signal_info *sig_info);
void
dco_start_tun(struct tuntap *tt);
diff --git a/src/openvpn/socket.c b/src/openvpn/socket.c
index 4fb6fe6..bd7a449 100644
--- a/src/openvpn/socket.c
+++ b/src/openvpn/socket.c
@@ -2247,7 +2247,7 @@
}
else
{
- dco_p2p_new_peer(c->c1.tuntap->hand, sock, sig_info);
+ dco_p2p_new_peer(c->c1.tuntap->hand, &c->c1.tuntap->dco_new_peer_ov, sock, sig_info);
}
sock->sockflags |= SF_DCO_WIN;
diff --git a/src/openvpn/tun.h b/src/openvpn/tun.h
index b616f5d..bcc23b4 100644
--- a/src/openvpn/tun.h
+++ b/src/openvpn/tun.h
@@ -215,6 +215,7 @@
#ifdef _WIN32
HANDLE hand;
+ OVERLAPPED dco_new_peer_ov; /* used for async NEW_PEER dco call, which might wait for TCP connect */
struct overlapped_io reads;
struct overlapped_io writes;
struct rw_handle rw_handle;
--
To view, visit http://gerrit.openvpn.net/c/openvpn/+/928?usp=email
To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings
Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: Ib1db457c42a80f6b8fc0e3ceb4a895d4cf7f0155
Gerrit-Change-Number: 928
Gerrit-PatchSet: 2
Gerrit-Owner: stipa <lstipakov@...277...>
Gerrit-Reviewer: cron2 <gert@...1296...>
Gerrit-Reviewer: flichtenheld <frank@...2641...>
Gerrit-Reviewer: plaisthos <arne-openvpn@...1227...>
Gerrit-CC: openvpn-devel <openvpn-devel@lists.sourceforge.net>
Gerrit-MessageType: merged
[-- Attachment #2: Type: text/html, Size: 6925 bytes --]
^ permalink raw reply related [flat|nested] 7+ messages in thread
end of thread, other threads:[~2025-04-01 20:53 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <gerrit.1743515497000.Ib1db457c42a80f6b8fc0e3ceb4a895d4cf7f0155@...2715...>
2025-04-01 13:51 ` [Openvpn-devel] [S] Change in openvpn[master]: dco-win: Fix crash when cancelling pending operation stipa (Code Review)
2025-04-01 18:15 ` cron2 (Code Review)
2025-04-01 18:15 ` [Openvpn-devel] [PATCH v1] " Gert Doering
2025-04-01 20:52 ` [Openvpn-devel] [PATCH applied] " Gert Doering
2025-04-01 20:47 ` [Openvpn-devel] [PATCH v1] " Gert Doering
2025-04-01 20:53 ` [Openvpn-devel] [S] Change in openvpn[master]: " cron2 (Code Review)
2025-04-01 20:53 ` cron2 (Code Review)
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.