diff mbox series

[Openvpn-devel,v4] PUSH_UPDATE server: bug-fix, reset buffer after processing

Message ID 20251010142002.27308-1-gert@greenie.muc.de
State New
Headers show
Series [Openvpn-devel,v4] PUSH_UPDATE server: bug-fix, reset buffer after processing | expand

Commit Message

Gert Doering Oct. 10, 2025, 2:19 p.m. UTC 
From: Marco Baffo <marco@mandelbit.com>

In the send_single_push_update() function the buffer containing
the message was not reset after processing, so o in a push-update-broad
the messages sent starting from the second client would have been
shrunk (offset advanced and size decreased).

Change-Id: I41d08a9a2e79ac1f1104e72dd5b7b7617e2071a0
Signed-off-by: Marco Baffo <marco@mandelbit.com>
Acked-by: Gert Doering <gert@greenie.muc.de>
Gerrit URL: https://gerrit.openvpn.net/c/openvpn/+/1264
---

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/+/1264
This mail reflects revision 4 of this Change.

Acked-by according to Gerrit (reflected above):
Gert Doering <gert@greenie.muc.de>

Comments

Gert Doering Oct. 10, 2025, 2:30 p.m. UTC | #1 
Discovered the problem while testing #1255 with multiple clients online
at the same time, with different capabilities - and it only sent anything
to the very first (capable) client, and then "empty string" because
the buffer in msgs[i] was buf_advance()'d and it shouldn't have been...

So this fixes things by doing a shallow copy to a temp "buffer", which
is safe as long as no writes to the memory is done - reading & advancing
the pointers is fine (we discussed various approaches on IRC, as can
be seen in v1/v3/v4 in gerrit ;-) - and a "make documentation for this
API" issue also appeared...).

Tested this + #1255, and now things behave correctly.

Your patch has been applied to the master branch.

commit 107f80b8e3102cca3a2cc008d37895f96ec2f17c
Author: Marco Baffo
Date:   Fri Oct 10 16:19:56 2025 +0200

     PUSH_UPDATE server: bug-fix, reset buffer after processing

     Signed-off-by: Marco Baffo <marco@mandelbit.com>
     Acked-by: Gert Doering <gert@greenie.muc.de>
     Gerrit URL: https://gerrit.openvpn.net/c/openvpn/+/1264
     Message-Id: <20251010142002.27308-1-gert@greenie.muc.de>
     URL: https://sourceforge.net/p/openvpn/mailman/message/59244933/
     Signed-off-by: Gert Doering <gert@greenie.muc.de>


--
kind regards,

Gert Doering
diff mbox series

Patch

diff --git a/src/openvpn/push_util.c b/src/openvpn/push_util.c
index f306104..b475d2e 100644
--- a/src/openvpn/push_util.c
+++ b/src/openvpn/push_util.c
@@ -170,9 +170,12 @@ 
          * inside `process_incoming_push_msg()`. However, we don't need
          * to check the return value here because we just want to `advance`,
          * meaning we skip the `push_update_cmd' we added earlier.
+         * Also we need to make a temporary copy so we can buf_advance()
+         * without modifying original buffer.
          */
-        buf_string_compare_advance(&msgs[i], push_update_cmd);
-        if (process_incoming_push_update(c, pull_permission_mask(c), option_types_found, &msgs[i], true) == PUSH_MSG_ERROR)
+        struct buffer tmp_msg = msgs[i];
+        buf_string_compare_advance(&tmp_msg, push_update_cmd);
+        if (process_incoming_push_update(c, pull_permission_mask(c), option_types_found, &tmp_msg, true) == PUSH_MSG_ERROR)
         {
             msg(M_WARN, "Failed to process push update message sent to client ID: %u",
                 c->c2.tls_multi ? c->c2.tls_multi->peer_id : UINT32_MAX);