diff mbox series

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

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

Commit Message

Selva Nair Dec. 29, 2022, 4:07 a.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

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, 11:13 a.m. UTC | #1 
Hi,

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

Good catch!

> Fixes #206

Strangely enough, I am not able to reproduce this bug - I posted logs
to the GitHub issue.

Fix indeed makes sense. Indeed, according to docs, UnmapViewOfFile()
accepts return value from MapViewOfFile(),
not from CreateFileMapping(), like it is implemented in interactive
service. Interesting that in tun.c implementation
is correct.

> -    if (handle && *handle && *handle != INVALID_HANDLE_VALUE)
> +    if (ring && *ring)
>      {
> -        UnmapViewOfFile(*handle);
> -        *handle = INVALID_HANDLE_VALUE;
> +        UnmapViewOfFile(*ring);
>      }
> -    return INVALID_HANDLE_VALUE;
> +    *ring = NULL;

It makes sense to move NULL assignment inside if { } to handle
unlikely case when the passed "ring" is NULL.
diff mbox series

Patch

diff --git a/src/openvpnserv/interactive.c b/src/openvpnserv/interactive.c
index 5b396e01..2fe41f09 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);
     }
-    return INVALID_HANDLE_VALUE;
+    *ring = NULL;
 }
 
 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();