diff mbox series

[Openvpn-devel,v2] Properly unmap ring buffer file-map in interactive service

Message ID 20221229134729.1474034-1-selva.nair@gmail.com
State Accepted
Headers show
Series [Openvpn-devel,v2] Properly unmap ring buffer file-map in interactive service | expand

Commit Message

Selva Nair Dec. 29, 2022, 1:47 p.m. UTC 
From: Selva Nair <selva.nair@gmail.com>

The return value of MapViewOfFile must be passed to UnmapViewofFile,
instead of the file handle.

Fixes #206

v2: move *ring = NULL inside if {}

Signed-off-by: Selva Nair <selva.nair@gmail.com>
---
 src/openvpnserv/interactive.c | 28 +++++++++++++++-------------
 1 file changed, 15 insertions(+), 13 deletions(-)

Comments

Lev Stipakov Dec. 29, 2022, 2:10 p.m. UTC | #1 
Acked-by: Lev Stipakov <lstipakov@gmail.com>

to 29. jouluk. 2022 klo 15.48 selva.nair@gmail.com kirjoitti:
>
> From: Selva Nair <selva.nair@gmail.com>
>
> The return value of MapViewOfFile must be passed to UnmapViewofFile,
> instead of the file handle.
>
> Fixes #206
>
> v2: move *ring = NULL inside if {}
>
> Signed-off-by: Selva Nair <selva.nair@gmail.com>
> ---
>  src/openvpnserv/interactive.c | 28 +++++++++++++++-------------
>  1 file changed, 15 insertions(+), 13 deletions(-)
>
> diff --git a/src/openvpnserv/interactive.c b/src/openvpnserv/interactive.c
> index 5b396e01..47bcd72a 100644
> --- a/src/openvpnserv/interactive.c
> +++ b/src/openvpnserv/interactive.c
> @@ -109,6 +109,8 @@ typedef struct {
>      HANDLE send_tail_moved;
>      HANDLE receive_tail_moved;
>      HANDLE device;
> +    struct tun_ring *send_ring;
> +    struct tun_ring *receive_ring;
>  } ring_buffer_handles_t;
>
>
> @@ -165,15 +167,14 @@ CloseHandleEx(LPHANDLE handle)
>      return INVALID_HANDLE_VALUE;
>  }
>
> -static HANDLE
> -OvpnUnmapViewOfFile(LPHANDLE handle)
> +static void
> +OvpnUnmapViewOfFile(struct tun_ring **ring)
>  {
> -    if (handle && *handle && *handle != INVALID_HANDLE_VALUE)
> +    if (ring && *ring)
>      {
> -        UnmapViewOfFile(*handle);
> -        *handle = INVALID_HANDLE_VALUE;
> +        UnmapViewOfFile(*ring);
> +        *ring = NULL;
>      }
> -    return INVALID_HANDLE_VALUE;
>  }
>
>  static void
> @@ -182,8 +183,10 @@ CloseRingBufferHandles(ring_buffer_handles_t *ring_buffer_handles)
>      CloseHandleEx(&ring_buffer_handles->device);
>      CloseHandleEx(&ring_buffer_handles->receive_tail_moved);
>      CloseHandleEx(&ring_buffer_handles->send_tail_moved);
> -    OvpnUnmapViewOfFile(&ring_buffer_handles->send_ring_handle);
> -    OvpnUnmapViewOfFile(&ring_buffer_handles->receive_ring_handle);
> +    OvpnUnmapViewOfFile(&ring_buffer_handles->send_ring);
> +    OvpnUnmapViewOfFile(&ring_buffer_handles->receive_ring);
> +    CloseHandleEx(&ring_buffer_handles->receive_ring_handle);
> +    CloseHandleEx(&ring_buffer_handles->send_ring_handle);
>  }
>
>  static HANDLE
> @@ -1354,8 +1357,6 @@ HandleRegisterRingBuffers(const register_ring_buffers_message_t *rrb, HANDLE ovp
>                            ring_buffer_handles_t *ring_buffer_handles)
>  {
>      DWORD err = 0;
> -    struct tun_ring *send_ring;
> -    struct tun_ring *receive_ring;
>
>      CloseRingBufferHandles(ring_buffer_handles);
>
> @@ -1365,13 +1366,13 @@ HandleRegisterRingBuffers(const register_ring_buffers_message_t *rrb, HANDLE ovp
>          return err;
>      }
>
> -    err = DuplicateAndMapRing(ovpn_proc, rrb->send_ring_handle, &ring_buffer_handles->send_ring_handle, &send_ring);
> +    err = DuplicateAndMapRing(ovpn_proc, rrb->send_ring_handle, &ring_buffer_handles->send_ring_handle, &ring_buffer_handles->send_ring);
>      if (err != ERROR_SUCCESS)
>      {
>          return err;
>      }
>
> -    err = DuplicateAndMapRing(ovpn_proc, rrb->receive_ring_handle, &ring_buffer_handles->receive_ring_handle, &receive_ring);
> +    err = DuplicateAndMapRing(ovpn_proc, rrb->receive_ring_handle, &ring_buffer_handles->receive_ring_handle, &ring_buffer_handles->receive_ring);
>      if (err != ERROR_SUCCESS)
>      {
>          return err;
> @@ -1389,7 +1390,8 @@ HandleRegisterRingBuffers(const register_ring_buffers_message_t *rrb, HANDLE ovp
>          return err;
>      }
>
> -    if (!register_ring_buffers(ring_buffer_handles->device, send_ring, receive_ring,
> +    if (!register_ring_buffers(ring_buffer_handles->device, ring_buffer_handles->send_ring,
> +                               ring_buffer_handles->receive_ring,
>                                 ring_buffer_handles->send_tail_moved, ring_buffer_handles->receive_tail_moved))
>      {
>          err = GetLastError();
> --
> 2.34.1
>
>
>
> _______________________________________________
> Openvpn-devel mailing list
> Openvpn-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Gert Doering Dec. 29, 2022, 3:40 p.m. UTC | #2 
I have not tested this beyond "push to github, see MSVC compile succeed"
(but trust Selva and Lev to get this right, and the code change seems
to match the discussion in the issue)

Your patch has been applied to the master and release/2.6 branch.

commit 64f8833e119e31cf01dfe198538fbb5566fabf8f (master)
commit f87aed6ae2f6c4165b19c00fe4d39c44232cf44e (release/2.6)
Author: Selva Nair
Date:   Thu Dec 29 08:47:29 2022 -0500

     Properly unmap ring buffer file-map in interactive service

     Signed-off-by: Selva Nair <selva.nair@gmail.com>
     Acked-by: Lev Stipakov <lstipakov@gmail.com>
     Message-Id: <20221229134729.1474034-1-selva.nair@gmail.com>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg25859.html
     Signed-off-by: Gert Doering <gert@greenie.muc.de>


--
kind regards,

Gert Doering
diff mbox series

Patch

diff --git a/src/openvpnserv/interactive.c b/src/openvpnserv/interactive.c
index 5b396e01..47bcd72a 100644
--- a/src/openvpnserv/interactive.c
+++ b/src/openvpnserv/interactive.c
@@ -109,6 +109,8 @@  typedef struct {
     HANDLE send_tail_moved;
     HANDLE receive_tail_moved;
     HANDLE device;
+    struct tun_ring *send_ring;
+    struct tun_ring *receive_ring;
 } ring_buffer_handles_t;
 
 
@@ -165,15 +167,14 @@  CloseHandleEx(LPHANDLE handle)
     return INVALID_HANDLE_VALUE;
 }
 
-static HANDLE
-OvpnUnmapViewOfFile(LPHANDLE handle)
+static void
+OvpnUnmapViewOfFile(struct tun_ring **ring)
 {
-    if (handle && *handle && *handle != INVALID_HANDLE_VALUE)
+    if (ring && *ring)
     {
-        UnmapViewOfFile(*handle);
-        *handle = INVALID_HANDLE_VALUE;
+        UnmapViewOfFile(*ring);
+        *ring = NULL;
     }
-    return INVALID_HANDLE_VALUE;
 }
 
 static void
@@ -182,8 +183,10 @@  CloseRingBufferHandles(ring_buffer_handles_t *ring_buffer_handles)
     CloseHandleEx(&ring_buffer_handles->device);
     CloseHandleEx(&ring_buffer_handles->receive_tail_moved);
     CloseHandleEx(&ring_buffer_handles->send_tail_moved);
-    OvpnUnmapViewOfFile(&ring_buffer_handles->send_ring_handle);
-    OvpnUnmapViewOfFile(&ring_buffer_handles->receive_ring_handle);
+    OvpnUnmapViewOfFile(&ring_buffer_handles->send_ring);
+    OvpnUnmapViewOfFile(&ring_buffer_handles->receive_ring);
+    CloseHandleEx(&ring_buffer_handles->receive_ring_handle);
+    CloseHandleEx(&ring_buffer_handles->send_ring_handle);
 }
 
 static HANDLE
@@ -1354,8 +1357,6 @@  HandleRegisterRingBuffers(const register_ring_buffers_message_t *rrb, HANDLE ovp
                           ring_buffer_handles_t *ring_buffer_handles)
 {
     DWORD err = 0;
-    struct tun_ring *send_ring;
-    struct tun_ring *receive_ring;
 
     CloseRingBufferHandles(ring_buffer_handles);
 
@@ -1365,13 +1366,13 @@  HandleRegisterRingBuffers(const register_ring_buffers_message_t *rrb, HANDLE ovp
         return err;
     }
 
-    err = DuplicateAndMapRing(ovpn_proc, rrb->send_ring_handle, &ring_buffer_handles->send_ring_handle, &send_ring);
+    err = DuplicateAndMapRing(ovpn_proc, rrb->send_ring_handle, &ring_buffer_handles->send_ring_handle, &ring_buffer_handles->send_ring);
     if (err != ERROR_SUCCESS)
     {
         return err;
     }
 
-    err = DuplicateAndMapRing(ovpn_proc, rrb->receive_ring_handle, &ring_buffer_handles->receive_ring_handle, &receive_ring);
+    err = DuplicateAndMapRing(ovpn_proc, rrb->receive_ring_handle, &ring_buffer_handles->receive_ring_handle, &ring_buffer_handles->receive_ring);
     if (err != ERROR_SUCCESS)
     {
         return err;
@@ -1389,7 +1390,8 @@  HandleRegisterRingBuffers(const register_ring_buffers_message_t *rrb, HANDLE ovp
         return err;
     }
 
-    if (!register_ring_buffers(ring_buffer_handles->device, send_ring, receive_ring,
+    if (!register_ring_buffers(ring_buffer_handles->device, ring_buffer_handles->send_ring,
+                               ring_buffer_handles->receive_ring,
                                ring_buffer_handles->send_tail_moved, ring_buffer_handles->receive_tail_moved))
     {
         err = GetLastError();