|
From: mrbff (C. Review) <ge...@op...> - 2025-10-27 16:06:57
|
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/+/1316?usp=email
to review the following change.
Change subject: PUSH_UPDATE server: added new unit tests and improved documentation
......................................................................
PUSH_UPDATE server: added new unit tests and improved documentation
Change-Id: Idba419681fe3ccc4e6e2f6ce7592332dcff62cd9
Signed-off-by: Marco Baffo <ma...@ma...>
---
M src/openvpn/push_util.c
M tests/unit_tests/openvpn/test_push_update_msg.c
2 files changed, 76 insertions(+), 4 deletions(-)
git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/16/1316/1
diff --git a/src/openvpn/push_util.c b/src/openvpn/push_util.c
index 552679a..d23b96d 100644
--- a/src/openvpn/push_util.c
+++ b/src/openvpn/push_util.c
@@ -270,6 +270,9 @@
* we want to send exceeds that size we have to split it into smaller messages */
ASSERT(push_bundle_size > extra);
const size_t safe_cap = push_bundle_size - extra;
+ /* the number of messages will be the total message length (just the inner message, the options,
+ * excluding `PUSH_UPDATE,` string and possible `,push-continuation n` string) divided by safe_cap,
+ * plus 1 if there is a remainder */
size_t msgs_num = (strlen(msg) / safe_cap) + ((strlen(msg) % safe_cap) != 0);
struct buffer *msgs = gc_malloc((msgs_num + 1) * sizeof(struct buffer), true, &gc);
diff --git a/tests/unit_tests/openvpn/test_push_update_msg.c b/tests/unit_tests/openvpn/test_push_update_msg.c
index 516e94c..83c27bb 100644
--- a/tests/unit_tests/openvpn/test_push_update_msg.c
+++ b/tests/unit_tests/openvpn/test_push_update_msg.c
@@ -136,10 +136,19 @@
bool
send_control_channel_string(struct context *c, const char *str, msglvl_t msglevel)
{
- if (res && res[i] && strcmp(res[i], str))
+ if (res && res[i])
{
- printf("\n\nexpected: %s\n\n actual: %s\n\n", res[i], str);
- return false;
+ size_t res_len = strlen(res[i]);
+ size_t str_len = strlen(str);
+ if (res_len != str_len || strcmp(res[i], str))
+ {
+ if (res_len != str_len)
+ {
+ printf("\n\nexpected_size: %lu\n actual_size: %lu", res_len, str_len);
+ }
+ printf("\n\nexpected: %s\n\n actual: %s\n\n", res[i], str);
+ return false;
+ }
}
i++;
return true;
@@ -339,6 +348,23 @@
"PUSH_UPDATE,,"
};
+char *r11[] = {
+ "PUSH_UPDATE,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,push-continuation 2",
+ "PUSH_UPDATE,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,push-continuation 2",
+ "PUSH_UPDATE,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,push-continuation 2",
+ "PUSH_UPDATE,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,push-continuation 2",
+ "PUSH_UPDATE,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,push-continuation 1"
+};
+
+char *r12[] = {
+ "PUSH_UPDATE,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,,,,,,a,push-continuation 2",
+ "PUSH_UPDATE,abc,push-continuation 1"
+};
+
+char *r13[] = {
+ "PUSH_UPDATE,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,,,,,,a,"
+};
+
const char *msg0 = "redirect-gateway local,route 192.168.1.0 255.255.255.0";
const char *msg1 = "-dhcp-option,blablalalalalalalalalalalalalf, lalalalalalalalalalalalalalaf,"
@@ -365,6 +391,16 @@
"daisy damage damp dance danger daring dash daughter dawn day deal debate debris decade december decide decline"
"decorate decrease deer defense define defy degree delay deliver demand demise denial";
+const char *msg11 = "a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,"
+ "a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,"
+ "a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,"
+ "a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,"
+ "a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a";
+
+const char *msg12 = "a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,,,,,,a,abc";
+
+const char *msg13 = "a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,,,,,,a,";
+
#define PUSH_BUNDLE_SIZE_TEST 184
static void
@@ -476,6 +512,36 @@
assert_int_equal(send_push_update(m, &cid, msg10, UPT_BY_CID, PUSH_BUNDLE_SIZE_TEST), -EINVAL);
}
+static void
+test_send_push_msg11(void **state)
+{
+ i = 0;
+ res = r11;
+ struct multi_context *m = *state;
+ const unsigned long cid = 0;
+ assert_int_equal(send_push_update(m, &cid, msg11, UPT_BY_CID, PUSH_BUNDLE_SIZE_TEST), 1);
+}
+
+static void
+test_send_push_msg12(void **state)
+{
+ i = 0;
+ res = r12;
+ struct multi_context *m = *state;
+ const unsigned long cid = 0;
+ assert_int_equal(send_push_update(m, &cid, msg12, UPT_BY_CID, PUSH_BUNDLE_SIZE_TEST), 1);
+}
+
+static void
+test_send_push_msg13(void **state)
+{
+ i = 0;
+ res = r13;
+ struct multi_context *m = *state;
+ const unsigned long cid = 0;
+ assert_int_equal(send_push_update(m, &cid, msg13, UPT_BY_CID, PUSH_BUNDLE_SIZE_TEST), 1);
+}
+
#undef PUSH_BUNDLE_SIZE_TEST
static int
@@ -545,7 +611,10 @@
cmocka_unit_test_setup_teardown(test_send_push_msg7, setup2, teardown2),
cmocka_unit_test_setup_teardown(test_send_push_msg8, setup2, teardown2),
cmocka_unit_test_setup_teardown(test_send_push_msg9, setup2, teardown2),
- cmocka_unit_test_setup_teardown(test_send_push_msg10, setup2, teardown2)
+ cmocka_unit_test_setup_teardown(test_send_push_msg10, setup2, teardown2),
+ cmocka_unit_test_setup_teardown(test_send_push_msg11, setup2, teardown2),
+ cmocka_unit_test_setup_teardown(test_send_push_msg12, setup2, teardown2),
+ cmocka_unit_test_setup_teardown(test_send_push_msg13, setup2, teardown2)
#endif
};
--
To view, visit http://gerrit.openvpn.net/c/openvpn/+/1316?usp=email
To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings?usp=email
Gerrit-MessageType: newchange
Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: Idba419681fe3ccc4e6e2f6ce7592332dcff62cd9
Gerrit-Change-Number: 1316
Gerrit-PatchSet: 1
Gerrit-Owner: mrbff <ma...@ma...>
Gerrit-Reviewer: flichtenheld <fr...@li...>
Gerrit-Reviewer: plaisthos <arn...@rf...>
Gerrit-CC: openvpn-devel <ope...@li...>
Gerrit-Attention: plaisthos <arn...@rf...>
Gerrit-Attention: flichtenheld <fr...@li...>
|
|
From: flichtenheld (C. Review) <ge...@op...> - 2025-10-28 10:39:21
|
Attention is currently required from: mrbff, plaisthos. flichtenheld has posted comments on this change by mrbff. ( http://gerrit.openvpn.net/c/openvpn/+/1316?usp=email ) Change subject: PUSH_UPDATE server: added new unit tests and improved documentation ...................................................................... Patch Set 1: Code-Review-1 (1 comment) File tests/unit_tests/openvpn/test_push_update_msg.c: http://gerrit.openvpn.net/c/openvpn/+/1316/comment/c3c8a28a_a6ce6843?usp=email : PS1, Line 147: printf("\n\nexpected_size: %lu\n actual_size: %lu", res_len, str_len); ```suggestion printf("\n\nexpected_size: %zu\n actual_size: %zu", res_len, str_len); ``` -- To view, visit http://gerrit.openvpn.net/c/openvpn/+/1316?usp=email To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings?usp=email Gerrit-MessageType: comment Gerrit-Project: openvpn Gerrit-Branch: master Gerrit-Change-Id: Idba419681fe3ccc4e6e2f6ce7592332dcff62cd9 Gerrit-Change-Number: 1316 Gerrit-PatchSet: 1 Gerrit-Owner: mrbff <ma...@ma...> Gerrit-Reviewer: flichtenheld <fr...@li...> Gerrit-Reviewer: plaisthos <arn...@rf...> Gerrit-CC: openvpn-devel <ope...@li...> Gerrit-Attention: plaisthos <arn...@rf...> Gerrit-Attention: mrbff <ma...@ma...> Gerrit-Comment-Date: Tue, 28 Oct 2025 10:39:06 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes |
|
From: flichtenheld (C. Review) <ge...@op...> - 2025-10-28 10:45:18
|
Attention is currently required from: mrbff, plaisthos. flichtenheld has posted comments on this change by mrbff. ( http://gerrit.openvpn.net/c/openvpn/+/1316?usp=email ) Change subject: PUSH_UPDATE server: added new unit tests and improved documentation ...................................................................... Patch Set 1: Code-Review-2 (1 comment) Patchset: PS1: Actually on second thought this whole code should be replaced with "check_expected". This is exactly what it is intended for. See my recent test_options_parse.c for example -- To view, visit http://gerrit.openvpn.net/c/openvpn/+/1316?usp=email To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings?usp=email Gerrit-MessageType: comment Gerrit-Project: openvpn Gerrit-Branch: master Gerrit-Change-Id: Idba419681fe3ccc4e6e2f6ce7592332dcff62cd9 Gerrit-Change-Number: 1316 Gerrit-PatchSet: 1 Gerrit-Owner: mrbff <ma...@ma...> Gerrit-Reviewer: flichtenheld <fr...@li...> Gerrit-Reviewer: plaisthos <arn...@rf...> Gerrit-CC: openvpn-devel <ope...@li...> Gerrit-Attention: plaisthos <arn...@rf...> Gerrit-Attention: mrbff <ma...@ma...> Gerrit-Comment-Date: Tue, 28 Oct 2025 10:45:04 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes |
|
From: mrbff (C. Review) <ge...@op...> - 2025-10-28 19:28:37
|
Attention is currently required from: mrbff, plaisthos.
Hello flichtenheld, plaisthos,
I'd like you to reexamine a change. Please visit
http://gerrit.openvpn.net/c/openvpn/+/1316?usp=email
to look at the new patch set (#2).
Change subject: PUSH_UPDATE server: invalid read bug-fix and unit-tests improvements
......................................................................
PUSH_UPDATE server: invalid read bug-fix and unit-tests improvements
The number of messages calculated before the call to message_splitter(), used
in the memory allocation in the buffer array, could in certain cases be less
than one than the actual number of messages, thus causing an override of the
sentinel buffer in message_splitter and therefore an invalid read in
send_single_push_update(). The case in question would be, for example, a
sequence of three options "A, B, C" with the size of B equal to safe_cap - 1
and the sum of the sizes of A and C less than safe_cap - 2.
The buffer array was therefore replaced with a list of buffers to completely
avoid calculating the number of messages before it was actually computed.
The test case in question has been added to the unit tests.
The unit tests have been improved using cmocka macros.
Change-Id: Idba419681fe3ccc4e6e2f6ce7592332dcff62cd9
Signed-off-by: Marco Baffo <ma...@ma...>
---
M src/openvpn/push_util.c
M tests/unit_tests/openvpn/test_push_update_msg.c
2 files changed, 145 insertions(+), 32 deletions(-)
git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/16/1316/2
diff --git a/src/openvpn/push_util.c b/src/openvpn/push_util.c
index 552679a..51c7b5f 100644
--- a/src/openvpn/push_util.c
+++ b/src/openvpn/push_util.c
@@ -91,7 +91,7 @@
* Return `false` on failure an `true` on success.
*/
static bool
-message_splitter(const char *s, struct buffer *msgs, struct gc_arena *gc, const size_t safe_cap)
+message_splitter(const char *s, struct buffer_list *msgs, struct gc_arena *gc, const size_t safe_cap)
{
if (!s || !*s)
{
@@ -100,7 +100,6 @@
char *str = gc_strdup(s, gc);
size_t i = 0;
- int im = 0;
while (*str)
{
@@ -115,37 +114,38 @@
}
str[ci] = '\0';
/* copy from i to (ci -1) */
- msgs[im] = forge_msg(str, ",push-continuation 2", gc);
+ struct buffer tmp = forge_msg(str, ",push-continuation 2", gc);
+ buffer_list_push(msgs, BSTR(&tmp));
i = ci + 1;
}
else
{
- if (im)
+ if (msgs->head)
{
- msgs[im] = forge_msg(str, ",push-continuation 1", gc);
+ struct buffer tmp = forge_msg(str, ",push-continuation 1", gc);
+ buffer_list_push(msgs, BSTR(&tmp));
}
else
{
- msgs[im] = forge_msg(str, NULL, gc);
+ struct buffer tmp = forge_msg(str, NULL, gc);
+ buffer_list_push(msgs, BSTR(&tmp));
}
i = strlen(str);
}
str = &str[i];
- im++;
}
return true;
}
/* send the message(s) prepared to one single client */
static bool
-send_single_push_update(struct multi_context *m, struct multi_instance *mi, struct buffer *msgs)
+send_single_push_update(struct multi_context *m, struct multi_instance *mi, struct buffer_list *msgs)
{
- if (!msgs[0].data || !*(msgs[0].data))
+ if (!msgs->head)
{
return false;
}
- int i = -1;
unsigned int option_types_found = 0;
struct context *c = &mi->context;
struct options o;
@@ -160,9 +160,10 @@
o.ifconfig_local = canary;
o.ifconfig_ipv6_local = canary;
- while (msgs[++i].data && *(msgs[i].data))
+ struct buffer_entry *e = msgs->head;
+ while (e)
{
- if (!send_control_channel_string(c, BSTR(&msgs[i]), D_PUSH))
+ if (!send_control_channel_string(c, BSTR(&e->buf), D_PUSH))
{
return false;
}
@@ -182,13 +183,14 @@
* Also we need to make a temporary copy so we can buf_advance()
* without modifying original buffer.
*/
- struct buffer tmp_msg = msgs[i];
+ struct buffer tmp_msg = e->buf;
buf_string_compare_advance(&tmp_msg, push_update_cmd);
unsigned int permission_mask = pull_permission_mask(c);
if (process_push_update(c, &o, permission_mask, &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->peer_id);
}
+ e = e->next;
}
if (option_types_found & OPT_P_UP)
@@ -270,12 +272,11 @@
* we want to send exceeds that size we have to split it into smaller messages */
ASSERT(push_bundle_size > extra);
const size_t safe_cap = push_bundle_size - extra;
- size_t msgs_num = (strlen(msg) / safe_cap) + ((strlen(msg) % safe_cap) != 0);
- struct buffer *msgs = gc_malloc((msgs_num + 1) * sizeof(struct buffer), true, &gc);
+ struct buffer_list *msgs = buffer_list_new();
- msgs[msgs_num].data = NULL;
if (!message_splitter(msg, msgs, &gc, safe_cap))
{
+ buffer_list_free(msgs);
gc_free(&gc);
return -EINVAL;
}
@@ -286,6 +287,7 @@
if (!mi)
{
+ buffer_list_free(msgs);
gc_free(&gc);
return -ENOENT;
}
@@ -293,6 +295,7 @@
if (!support_push_update(mi))
{
msg(M_CLIENT, "PUSH_UPDATE: not sending message to unsupported peer with ID: %u", mi->context.c2.tls_multi->peer_id);
+ buffer_list_free(msgs);
gc_free(&gc);
return 0;
}
@@ -300,11 +303,13 @@
if (!mi->halt
&& send_single_push_update(m, mi, msgs))
{
+ buffer_list_free(msgs);
gc_free(&gc);
return 1;
}
else
{
+ buffer_list_free(msgs);
gc_free(&gc);
return 0;
}
@@ -334,6 +339,7 @@
}
hash_iterator_free(&hi);
+ buffer_list_free(msgs);
gc_free(&gc);
return count;
}
diff --git a/tests/unit_tests/openvpn/test_push_update_msg.c b/tests/unit_tests/openvpn/test_push_update_msg.c
index 516e94c..e450a31 100644
--- a/tests/unit_tests/openvpn/test_push_update_msg.c
+++ b/tests/unit_tests/openvpn/test_push_update_msg.c
@@ -136,11 +136,7 @@
bool
send_control_channel_string(struct context *c, const char *str, msglvl_t msglevel)
{
- if (res && res[i] && strcmp(res[i], str))
- {
- printf("\n\nexpected: %s\n\n actual: %s\n\n", res[i], str);
- return false;
- }
+ check_expected(str);
i++;
return true;
}
@@ -301,44 +297,75 @@
#ifdef ENABLE_MANAGEMENT
char *r0[] = {
- "PUSH_UPDATE,redirect-gateway local,route 192.168.1.0 255.255.255.0"
+ "PUSH_UPDATE,redirect-gateway local,route 192.168.1.0 255.255.255.0",
+ NULL
};
char *r1[] = {
"PUSH_UPDATE,-dhcp-option,blablalalalalalalalalalalalalf, lalalalalalalalalalalalalalaf,push-continuation 2",
"PUSH_UPDATE, akakakakakakakakakakakaf, dhcp-option DNS 8.8.8.8,redirect-gateway local,push-continuation 2",
- "PUSH_UPDATE,route 192.168.1.0 255.255.255.0,push-continuation 1"
+ "PUSH_UPDATE,route 192.168.1.0 255.255.255.0,push-continuation 1",
+ NULL
};
char *r3[] = {
- "PUSH_UPDATE,,,"
+ "PUSH_UPDATE,,,",
+ NULL
};
char *r4[] = {
"PUSH_UPDATE,-dhcp-option, blablalalalalalalalalalalalalf, lalalalalalalalalalalalalalaf,push-continuation 2",
"PUSH_UPDATE, akakakakakakakakakakakaf,dhcp-option DNS 8.8.8.8, redirect-gateway local,push-continuation 2",
- "PUSH_UPDATE, route 192.168.1.0 255.255.255.0,,push-continuation 1"
+ "PUSH_UPDATE, route 192.168.1.0 255.255.255.0,,push-continuation 1",
+ NULL
};
char *r5[] = {
"PUSH_UPDATE,,-dhcp-option, blablalalalalalalalalalalalalf, lalalalalalalalalalalalalalaf,push-continuation 2",
"PUSH_UPDATE, akakakakakakakakakakakaf,dhcp-option DNS 8.8.8.8, redirect-gateway local,push-continuation 2",
- "PUSH_UPDATE, route 192.168.1.0 255.255.255.0,push-continuation 1"
+ "PUSH_UPDATE, route 192.168.1.0 255.255.255.0,push-continuation 1",
+ NULL
};
char *r6[] = {
"PUSH_UPDATE,-dhcp-option,blablalalalalalalalalalalalalf, lalalalalalalalalalalalalalaf,push-continuation 2",
"PUSH_UPDATE, akakakakakakakakakakakaf, dhcp-option DNS 8.8.8.8, redirect-gateway 10.10.10.10,,push-continuation 2",
- "PUSH_UPDATE, route 192.168.1.0 255.255.255.0,,push-continuation 1"
+ "PUSH_UPDATE, route 192.168.1.0 255.255.255.0,,push-continuation 1",
+ NULL
};
char *r7[] = {
"PUSH_UPDATE,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,push-continuation 2",
- "PUSH_UPDATE,,,,,,,,,,,,,,,,,,,push-continuation 1"
+ "PUSH_UPDATE,,,,,,,,,,,,,,,,,,,push-continuation 1",
+ NULL
};
char *r8[] = {
"PUSH_UPDATE,-dhcp-option,blablalalalalalalalalalalalalf, lalalalalalalalalalalalalalaf,push-continuation 2",
"PUSH_UPDATE, akakakakakakakakakakakaf, dhcp-option DNS 8.8.8.8,redirect-gateway\n local,push-continuation 2",
- "PUSH_UPDATE,route 192.168.1.0 255.255.255.0\n\n\n,push-continuation 1"
+ "PUSH_UPDATE,route 192.168.1.0 255.255.255.0\n\n\n,push-continuation 1",
+ NULL
};
char *r9[] = {
- "PUSH_UPDATE,,"
+ "PUSH_UPDATE,,",
+ NULL
};
-
+char *r11[] = {
+ "PUSH_UPDATE,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,push-continuation 2",
+ "PUSH_UPDATE,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,push-continuation 2",
+ "PUSH_UPDATE,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,push-continuation 2",
+ "PUSH_UPDATE,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,push-continuation 2",
+ "PUSH_UPDATE,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,push-continuation 1",
+ NULL
+};
+char *r12[] = {
+ "PUSH_UPDATE,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,,,,,,a,push-continuation 2",
+ "PUSH_UPDATE,abc,push-continuation 1",
+ NULL
+};
+char *r13[] = {
+ "PUSH_UPDATE,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,,,,,,a,",
+ NULL
+};
+char *r14[] = {
+ "PUSH_UPDATE,a,push-continuation 2",
+ "PUSH_UPDATE,aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa,push-continuation 2",
+ "PUSH_UPDATE,a,push-continuation 1",
+ NULL
+};
const char *msg0 = "redirect-gateway local,route 192.168.1.0 255.255.255.0";
const char *msg1 = "-dhcp-option,blablalalalalalalalalalalalalf, lalalalalalalalalalalalalalaf,"
@@ -365,8 +392,29 @@
"daisy damage damp dance danger daring dash daughter dawn day deal debate debris decade december decide decline"
"decorate decrease deer defense define defy degree delay deliver demand demise denial";
+const char *msg11 = "a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,"
+ "a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,"
+ "a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,"
+ "a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,"
+ "a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a";
+
+const char *msg12 = "a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,,,,,,a,abc";
+
+const char *msg13 = "a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,,,,,,a,";
+
+const char *msg14 = "a,aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa,a";
+
#define PUSH_BUNDLE_SIZE_TEST 184
+#define expect_control_channel_strings() \
+ do \
+ { \
+ for (int j = 0; res[j] != NULL; j++) \
+ { \
+ expect_string(send_control_channel_string, str, res[j]); \
+ } \
+ } while (0)
+
static void
test_send_push_msg0(void **state)
{
@@ -374,8 +422,10 @@
res = r0;
struct multi_context *m = *state;
const unsigned long cid = 0;
+ expect_control_channel_strings();
assert_int_equal(send_push_update(m, &cid, msg0, UPT_BY_CID, PUSH_BUNDLE_SIZE_TEST), 1);
}
+
static void
test_send_push_msg1(void **state)
{
@@ -383,6 +433,7 @@
res = r1;
struct multi_context *m = *state;
const unsigned long cid = 0;
+ expect_control_channel_strings();
assert_int_equal(send_push_update(m, &cid, msg1, UPT_BY_CID, PUSH_BUNDLE_SIZE_TEST), 1);
}
@@ -403,6 +454,7 @@
res = r3;
struct multi_context *m = *state;
const unsigned long cid = 0;
+ expect_control_channel_strings();
assert_int_equal(send_push_update(m, &cid, msg3, UPT_BY_CID, PUSH_BUNDLE_SIZE_TEST), 1);
}
@@ -413,6 +465,7 @@
res = r4;
struct multi_context *m = *state;
const unsigned long cid = 0;
+ expect_control_channel_strings();
assert_int_equal(send_push_update(m, &cid, msg4, UPT_BY_CID, PUSH_BUNDLE_SIZE_TEST), 1);
}
@@ -423,6 +476,7 @@
res = r5;
struct multi_context *m = *state;
const unsigned long cid = 0;
+ expect_control_channel_strings();
assert_int_equal(send_push_update(m, &cid, msg5, UPT_BY_CID, PUSH_BUNDLE_SIZE_TEST), 1);
}
@@ -433,6 +487,7 @@
res = r6;
struct multi_context *m = *state;
const unsigned long cid = 0;
+ expect_control_channel_strings();
assert_int_equal(send_push_update(m, &cid, msg6, UPT_BY_CID, PUSH_BUNDLE_SIZE_TEST), 1);
}
@@ -443,6 +498,7 @@
res = r7;
struct multi_context *m = *state;
const unsigned long cid = 0;
+ expect_control_channel_strings();
assert_int_equal(send_push_update(m, &cid, msg7, UPT_BY_CID, PUSH_BUNDLE_SIZE_TEST), 1);
}
@@ -453,6 +509,7 @@
res = r8;
struct multi_context *m = *state;
const unsigned long cid = 0;
+ expect_control_channel_strings();
assert_int_equal(send_push_update(m, &cid, msg8, UPT_BY_CID, PUSH_BUNDLE_SIZE_TEST), 1);
}
@@ -463,6 +520,7 @@
res = r9;
struct multi_context *m = *state;
const unsigned long cid = 0;
+ expect_control_channel_strings();
assert_int_equal(send_push_update(m, &cid, msg9, UPT_BY_CID, PUSH_BUNDLE_SIZE_TEST), 1);
}
@@ -476,6 +534,50 @@
assert_int_equal(send_push_update(m, &cid, msg10, UPT_BY_CID, PUSH_BUNDLE_SIZE_TEST), -EINVAL);
}
+static void
+test_send_push_msg11(void **state)
+{
+ i = 0;
+ res = r11;
+ struct multi_context *m = *state;
+ const unsigned long cid = 0;
+ expect_control_channel_strings();
+ assert_int_equal(send_push_update(m, &cid, msg11, UPT_BY_CID, PUSH_BUNDLE_SIZE_TEST), 1);
+}
+
+static void
+test_send_push_msg12(void **state)
+{
+ i = 0;
+ res = r12;
+ struct multi_context *m = *state;
+ const unsigned long cid = 0;
+ expect_control_channel_strings();
+ assert_int_equal(send_push_update(m, &cid, msg12, UPT_BY_CID, PUSH_BUNDLE_SIZE_TEST), 1);
+}
+
+static void
+test_send_push_msg13(void **state)
+{
+ i = 0;
+ res = r13;
+ struct multi_context *m = *state;
+ const unsigned long cid = 0;
+ expect_control_channel_strings();
+ assert_int_equal(send_push_update(m, &cid, msg13, UPT_BY_CID, PUSH_BUNDLE_SIZE_TEST), 1);
+}
+
+static void
+test_send_push_msg14(void **state)
+{
+ i = 0;
+ res = r14;
+ struct multi_context *m = *state;
+ const unsigned long cid = 0;
+ expect_control_channel_strings();
+ assert_int_equal(send_push_update(m, &cid, msg14, UPT_BY_CID, PUSH_BUNDLE_SIZE_TEST), 1);
+}
+
#undef PUSH_BUNDLE_SIZE_TEST
static int
@@ -535,6 +637,7 @@
cmocka_unit_test_setup_teardown(test_incoming_push_message_mix, setup, teardown),
cmocka_unit_test_setup_teardown(test_incoming_push_message_mix2, setup, teardown),
#ifdef ENABLE_MANAGEMENT
+
cmocka_unit_test_setup_teardown(test_send_push_msg0, setup2, teardown2),
cmocka_unit_test_setup_teardown(test_send_push_msg1, setup2, teardown2),
cmocka_unit_test_setup_teardown(test_send_push_msg2, setup2, teardown2),
@@ -545,7 +648,11 @@
cmocka_unit_test_setup_teardown(test_send_push_msg7, setup2, teardown2),
cmocka_unit_test_setup_teardown(test_send_push_msg8, setup2, teardown2),
cmocka_unit_test_setup_teardown(test_send_push_msg9, setup2, teardown2),
- cmocka_unit_test_setup_teardown(test_send_push_msg10, setup2, teardown2)
+ cmocka_unit_test_setup_teardown(test_send_push_msg10, setup2, teardown2),
+ cmocka_unit_test_setup_teardown(test_send_push_msg11, setup2, teardown2),
+ cmocka_unit_test_setup_teardown(test_send_push_msg12, setup2, teardown2),
+ cmocka_unit_test_setup_teardown(test_send_push_msg13, setup2, teardown2),
+ cmocka_unit_test_setup_teardown(test_send_push_msg14, setup2, teardown2)
#endif
};
--
To view, visit http://gerrit.openvpn.net/c/openvpn/+/1316?usp=email
To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings?usp=email
Gerrit-MessageType: newpatchset
Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: Idba419681fe3ccc4e6e2f6ce7592332dcff62cd9
Gerrit-Change-Number: 1316
Gerrit-PatchSet: 2
Gerrit-Owner: mrbff <ma...@ma...>
Gerrit-Reviewer: flichtenheld <fr...@li...>
Gerrit-Reviewer: plaisthos <arn...@rf...>
Gerrit-CC: openvpn-devel <ope...@li...>
Gerrit-Attention: plaisthos <arn...@rf...>
Gerrit-Attention: mrbff <ma...@ma...>
|
|
From: mrbff (C. Review) <ge...@op...> - 2025-10-28 19:52:58
|
Attention is currently required from: flichtenheld, plaisthos. mrbff has posted comments on this change by mrbff. ( http://gerrit.openvpn.net/c/openvpn/+/1316?usp=email ) Change subject: PUSH_UPDATE server: invalid read bug-fix and unit-tests improvements ...................................................................... Patch Set 2: (1 comment) File tests/unit_tests/openvpn/test_push_update_msg.c: http://gerrit.openvpn.net/c/openvpn/+/1316/comment/c55bce38_3e8b3f8e?usp=email : PS1, Line 147: printf("\n\nexpected_size: %lu\n actual_size: %lu", res_len, str_len); > ```suggestion […] Done -- To view, visit http://gerrit.openvpn.net/c/openvpn/+/1316?usp=email To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings?usp=email Gerrit-MessageType: comment Gerrit-Project: openvpn Gerrit-Branch: master Gerrit-Change-Id: Idba419681fe3ccc4e6e2f6ce7592332dcff62cd9 Gerrit-Change-Number: 1316 Gerrit-PatchSet: 2 Gerrit-Owner: mrbff <ma...@ma...> Gerrit-Reviewer: flichtenheld <fr...@li...> Gerrit-Reviewer: plaisthos <arn...@rf...> Gerrit-CC: openvpn-devel <ope...@li...> Gerrit-Attention: plaisthos <arn...@rf...> Gerrit-Attention: flichtenheld <fr...@li...> Gerrit-Comment-Date: Tue, 28 Oct 2025 19:52:49 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: flichtenheld <fr...@li...> |
|
From: mrbff (C. Review) <ge...@op...> - 2025-10-29 08:21:01
|
Attention is currently required from: flichtenheld, plaisthos.
Hello flichtenheld, plaisthos,
I'd like you to reexamine a change. Please visit
http://gerrit.openvpn.net/c/openvpn/+/1316?usp=email
to look at the new patch set (#3).
Change subject: PUSH_UPDATE server: invalid read bug-fix and unit-tests improvements
......................................................................
PUSH_UPDATE server: invalid read bug-fix and unit-tests improvements
The number of messages calculated before the call to message_splitter(), used in the memory allocation in the buffer array, could in certain cases be less than one than the actual number of messages, thus causing an override of the sentinel buffer in message_splitter() and therefore an invalid read in send_single_push_update().
The case in question would be, for example, a sequence of three options "A, B, C" with the size of B equal to safe_cap - 1 and the sum of the sizes of A and C less than safe_cap - 2.
The buffer array was therefore replaced with a list of buffers to completely avoid calculating the number of messages before it was actually computed.
The test case in question has been added to the unit tests.
The unit tests have been improved using cmocka macros.
Change-Id: Idba419681fe3ccc4e6e2f6ce7592332dcff62cd9
Signed-off-by: Marco Baffo <ma...@ma...>
---
M src/openvpn/push_util.c
M tests/unit_tests/openvpn/test_push_update_msg.c
2 files changed, 145 insertions(+), 32 deletions(-)
git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/16/1316/3
diff --git a/src/openvpn/push_util.c b/src/openvpn/push_util.c
index 552679a..51c7b5f 100644
--- a/src/openvpn/push_util.c
+++ b/src/openvpn/push_util.c
@@ -91,7 +91,7 @@
* Return `false` on failure an `true` on success.
*/
static bool
-message_splitter(const char *s, struct buffer *msgs, struct gc_arena *gc, const size_t safe_cap)
+message_splitter(const char *s, struct buffer_list *msgs, struct gc_arena *gc, const size_t safe_cap)
{
if (!s || !*s)
{
@@ -100,7 +100,6 @@
char *str = gc_strdup(s, gc);
size_t i = 0;
- int im = 0;
while (*str)
{
@@ -115,37 +114,38 @@
}
str[ci] = '\0';
/* copy from i to (ci -1) */
- msgs[im] = forge_msg(str, ",push-continuation 2", gc);
+ struct buffer tmp = forge_msg(str, ",push-continuation 2", gc);
+ buffer_list_push(msgs, BSTR(&tmp));
i = ci + 1;
}
else
{
- if (im)
+ if (msgs->head)
{
- msgs[im] = forge_msg(str, ",push-continuation 1", gc);
+ struct buffer tmp = forge_msg(str, ",push-continuation 1", gc);
+ buffer_list_push(msgs, BSTR(&tmp));
}
else
{
- msgs[im] = forge_msg(str, NULL, gc);
+ struct buffer tmp = forge_msg(str, NULL, gc);
+ buffer_list_push(msgs, BSTR(&tmp));
}
i = strlen(str);
}
str = &str[i];
- im++;
}
return true;
}
/* send the message(s) prepared to one single client */
static bool
-send_single_push_update(struct multi_context *m, struct multi_instance *mi, struct buffer *msgs)
+send_single_push_update(struct multi_context *m, struct multi_instance *mi, struct buffer_list *msgs)
{
- if (!msgs[0].data || !*(msgs[0].data))
+ if (!msgs->head)
{
return false;
}
- int i = -1;
unsigned int option_types_found = 0;
struct context *c = &mi->context;
struct options o;
@@ -160,9 +160,10 @@
o.ifconfig_local = canary;
o.ifconfig_ipv6_local = canary;
- while (msgs[++i].data && *(msgs[i].data))
+ struct buffer_entry *e = msgs->head;
+ while (e)
{
- if (!send_control_channel_string(c, BSTR(&msgs[i]), D_PUSH))
+ if (!send_control_channel_string(c, BSTR(&e->buf), D_PUSH))
{
return false;
}
@@ -182,13 +183,14 @@
* Also we need to make a temporary copy so we can buf_advance()
* without modifying original buffer.
*/
- struct buffer tmp_msg = msgs[i];
+ struct buffer tmp_msg = e->buf;
buf_string_compare_advance(&tmp_msg, push_update_cmd);
unsigned int permission_mask = pull_permission_mask(c);
if (process_push_update(c, &o, permission_mask, &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->peer_id);
}
+ e = e->next;
}
if (option_types_found & OPT_P_UP)
@@ -270,12 +272,11 @@
* we want to send exceeds that size we have to split it into smaller messages */
ASSERT(push_bundle_size > extra);
const size_t safe_cap = push_bundle_size - extra;
- size_t msgs_num = (strlen(msg) / safe_cap) + ((strlen(msg) % safe_cap) != 0);
- struct buffer *msgs = gc_malloc((msgs_num + 1) * sizeof(struct buffer), true, &gc);
+ struct buffer_list *msgs = buffer_list_new();
- msgs[msgs_num].data = NULL;
if (!message_splitter(msg, msgs, &gc, safe_cap))
{
+ buffer_list_free(msgs);
gc_free(&gc);
return -EINVAL;
}
@@ -286,6 +287,7 @@
if (!mi)
{
+ buffer_list_free(msgs);
gc_free(&gc);
return -ENOENT;
}
@@ -293,6 +295,7 @@
if (!support_push_update(mi))
{
msg(M_CLIENT, "PUSH_UPDATE: not sending message to unsupported peer with ID: %u", mi->context.c2.tls_multi->peer_id);
+ buffer_list_free(msgs);
gc_free(&gc);
return 0;
}
@@ -300,11 +303,13 @@
if (!mi->halt
&& send_single_push_update(m, mi, msgs))
{
+ buffer_list_free(msgs);
gc_free(&gc);
return 1;
}
else
{
+ buffer_list_free(msgs);
gc_free(&gc);
return 0;
}
@@ -334,6 +339,7 @@
}
hash_iterator_free(&hi);
+ buffer_list_free(msgs);
gc_free(&gc);
return count;
}
diff --git a/tests/unit_tests/openvpn/test_push_update_msg.c b/tests/unit_tests/openvpn/test_push_update_msg.c
index 516e94c..e450a31 100644
--- a/tests/unit_tests/openvpn/test_push_update_msg.c
+++ b/tests/unit_tests/openvpn/test_push_update_msg.c
@@ -136,11 +136,7 @@
bool
send_control_channel_string(struct context *c, const char *str, msglvl_t msglevel)
{
- if (res && res[i] && strcmp(res[i], str))
- {
- printf("\n\nexpected: %s\n\n actual: %s\n\n", res[i], str);
- return false;
- }
+ check_expected(str);
i++;
return true;
}
@@ -301,44 +297,75 @@
#ifdef ENABLE_MANAGEMENT
char *r0[] = {
- "PUSH_UPDATE,redirect-gateway local,route 192.168.1.0 255.255.255.0"
+ "PUSH_UPDATE,redirect-gateway local,route 192.168.1.0 255.255.255.0",
+ NULL
};
char *r1[] = {
"PUSH_UPDATE,-dhcp-option,blablalalalalalalalalalalalalf, lalalalalalalalalalalalalalaf,push-continuation 2",
"PUSH_UPDATE, akakakakakakakakakakakaf, dhcp-option DNS 8.8.8.8,redirect-gateway local,push-continuation 2",
- "PUSH_UPDATE,route 192.168.1.0 255.255.255.0,push-continuation 1"
+ "PUSH_UPDATE,route 192.168.1.0 255.255.255.0,push-continuation 1",
+ NULL
};
char *r3[] = {
- "PUSH_UPDATE,,,"
+ "PUSH_UPDATE,,,",
+ NULL
};
char *r4[] = {
"PUSH_UPDATE,-dhcp-option, blablalalalalalalalalalalalalf, lalalalalalalalalalalalalalaf,push-continuation 2",
"PUSH_UPDATE, akakakakakakakakakakakaf,dhcp-option DNS 8.8.8.8, redirect-gateway local,push-continuation 2",
- "PUSH_UPDATE, route 192.168.1.0 255.255.255.0,,push-continuation 1"
+ "PUSH_UPDATE, route 192.168.1.0 255.255.255.0,,push-continuation 1",
+ NULL
};
char *r5[] = {
"PUSH_UPDATE,,-dhcp-option, blablalalalalalalalalalalalalf, lalalalalalalalalalalalalalaf,push-continuation 2",
"PUSH_UPDATE, akakakakakakakakakakakaf,dhcp-option DNS 8.8.8.8, redirect-gateway local,push-continuation 2",
- "PUSH_UPDATE, route 192.168.1.0 255.255.255.0,push-continuation 1"
+ "PUSH_UPDATE, route 192.168.1.0 255.255.255.0,push-continuation 1",
+ NULL
};
char *r6[] = {
"PUSH_UPDATE,-dhcp-option,blablalalalalalalalalalalalalf, lalalalalalalalalalalalalalaf,push-continuation 2",
"PUSH_UPDATE, akakakakakakakakakakakaf, dhcp-option DNS 8.8.8.8, redirect-gateway 10.10.10.10,,push-continuation 2",
- "PUSH_UPDATE, route 192.168.1.0 255.255.255.0,,push-continuation 1"
+ "PUSH_UPDATE, route 192.168.1.0 255.255.255.0,,push-continuation 1",
+ NULL
};
char *r7[] = {
"PUSH_UPDATE,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,push-continuation 2",
- "PUSH_UPDATE,,,,,,,,,,,,,,,,,,,push-continuation 1"
+ "PUSH_UPDATE,,,,,,,,,,,,,,,,,,,push-continuation 1",
+ NULL
};
char *r8[] = {
"PUSH_UPDATE,-dhcp-option,blablalalalalalalalalalalalalf, lalalalalalalalalalalalalalaf,push-continuation 2",
"PUSH_UPDATE, akakakakakakakakakakakaf, dhcp-option DNS 8.8.8.8,redirect-gateway\n local,push-continuation 2",
- "PUSH_UPDATE,route 192.168.1.0 255.255.255.0\n\n\n,push-continuation 1"
+ "PUSH_UPDATE,route 192.168.1.0 255.255.255.0\n\n\n,push-continuation 1",
+ NULL
};
char *r9[] = {
- "PUSH_UPDATE,,"
+ "PUSH_UPDATE,,",
+ NULL
};
-
+char *r11[] = {
+ "PUSH_UPDATE,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,push-continuation 2",
+ "PUSH_UPDATE,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,push-continuation 2",
+ "PUSH_UPDATE,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,push-continuation 2",
+ "PUSH_UPDATE,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,push-continuation 2",
+ "PUSH_UPDATE,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,push-continuation 1",
+ NULL
+};
+char *r12[] = {
+ "PUSH_UPDATE,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,,,,,,a,push-continuation 2",
+ "PUSH_UPDATE,abc,push-continuation 1",
+ NULL
+};
+char *r13[] = {
+ "PUSH_UPDATE,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,,,,,,a,",
+ NULL
+};
+char *r14[] = {
+ "PUSH_UPDATE,a,push-continuation 2",
+ "PUSH_UPDATE,aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa,push-continuation 2",
+ "PUSH_UPDATE,a,push-continuation 1",
+ NULL
+};
const char *msg0 = "redirect-gateway local,route 192.168.1.0 255.255.255.0";
const char *msg1 = "-dhcp-option,blablalalalalalalalalalalalalf, lalalalalalalalalalalalalalaf,"
@@ -365,8 +392,29 @@
"daisy damage damp dance danger daring dash daughter dawn day deal debate debris decade december decide decline"
"decorate decrease deer defense define defy degree delay deliver demand demise denial";
+const char *msg11 = "a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,"
+ "a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,"
+ "a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,"
+ "a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,"
+ "a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a";
+
+const char *msg12 = "a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,,,,,,a,abc";
+
+const char *msg13 = "a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,,,,,,a,";
+
+const char *msg14 = "a,aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa,a";
+
#define PUSH_BUNDLE_SIZE_TEST 184
+#define expect_control_channel_strings() \
+ do \
+ { \
+ for (int j = 0; res[j] != NULL; j++) \
+ { \
+ expect_string(send_control_channel_string, str, res[j]); \
+ } \
+ } while (0)
+
static void
test_send_push_msg0(void **state)
{
@@ -374,8 +422,10 @@
res = r0;
struct multi_context *m = *state;
const unsigned long cid = 0;
+ expect_control_channel_strings();
assert_int_equal(send_push_update(m, &cid, msg0, UPT_BY_CID, PUSH_BUNDLE_SIZE_TEST), 1);
}
+
static void
test_send_push_msg1(void **state)
{
@@ -383,6 +433,7 @@
res = r1;
struct multi_context *m = *state;
const unsigned long cid = 0;
+ expect_control_channel_strings();
assert_int_equal(send_push_update(m, &cid, msg1, UPT_BY_CID, PUSH_BUNDLE_SIZE_TEST), 1);
}
@@ -403,6 +454,7 @@
res = r3;
struct multi_context *m = *state;
const unsigned long cid = 0;
+ expect_control_channel_strings();
assert_int_equal(send_push_update(m, &cid, msg3, UPT_BY_CID, PUSH_BUNDLE_SIZE_TEST), 1);
}
@@ -413,6 +465,7 @@
res = r4;
struct multi_context *m = *state;
const unsigned long cid = 0;
+ expect_control_channel_strings();
assert_int_equal(send_push_update(m, &cid, msg4, UPT_BY_CID, PUSH_BUNDLE_SIZE_TEST), 1);
}
@@ -423,6 +476,7 @@
res = r5;
struct multi_context *m = *state;
const unsigned long cid = 0;
+ expect_control_channel_strings();
assert_int_equal(send_push_update(m, &cid, msg5, UPT_BY_CID, PUSH_BUNDLE_SIZE_TEST), 1);
}
@@ -433,6 +487,7 @@
res = r6;
struct multi_context *m = *state;
const unsigned long cid = 0;
+ expect_control_channel_strings();
assert_int_equal(send_push_update(m, &cid, msg6, UPT_BY_CID, PUSH_BUNDLE_SIZE_TEST), 1);
}
@@ -443,6 +498,7 @@
res = r7;
struct multi_context *m = *state;
const unsigned long cid = 0;
+ expect_control_channel_strings();
assert_int_equal(send_push_update(m, &cid, msg7, UPT_BY_CID, PUSH_BUNDLE_SIZE_TEST), 1);
}
@@ -453,6 +509,7 @@
res = r8;
struct multi_context *m = *state;
const unsigned long cid = 0;
+ expect_control_channel_strings();
assert_int_equal(send_push_update(m, &cid, msg8, UPT_BY_CID, PUSH_BUNDLE_SIZE_TEST), 1);
}
@@ -463,6 +520,7 @@
res = r9;
struct multi_context *m = *state;
const unsigned long cid = 0;
+ expect_control_channel_strings();
assert_int_equal(send_push_update(m, &cid, msg9, UPT_BY_CID, PUSH_BUNDLE_SIZE_TEST), 1);
}
@@ -476,6 +534,50 @@
assert_int_equal(send_push_update(m, &cid, msg10, UPT_BY_CID, PUSH_BUNDLE_SIZE_TEST), -EINVAL);
}
+static void
+test_send_push_msg11(void **state)
+{
+ i = 0;
+ res = r11;
+ struct multi_context *m = *state;
+ const unsigned long cid = 0;
+ expect_control_channel_strings();
+ assert_int_equal(send_push_update(m, &cid, msg11, UPT_BY_CID, PUSH_BUNDLE_SIZE_TEST), 1);
+}
+
+static void
+test_send_push_msg12(void **state)
+{
+ i = 0;
+ res = r12;
+ struct multi_context *m = *state;
+ const unsigned long cid = 0;
+ expect_control_channel_strings();
+ assert_int_equal(send_push_update(m, &cid, msg12, UPT_BY_CID, PUSH_BUNDLE_SIZE_TEST), 1);
+}
+
+static void
+test_send_push_msg13(void **state)
+{
+ i = 0;
+ res = r13;
+ struct multi_context *m = *state;
+ const unsigned long cid = 0;
+ expect_control_channel_strings();
+ assert_int_equal(send_push_update(m, &cid, msg13, UPT_BY_CID, PUSH_BUNDLE_SIZE_TEST), 1);
+}
+
+static void
+test_send_push_msg14(void **state)
+{
+ i = 0;
+ res = r14;
+ struct multi_context *m = *state;
+ const unsigned long cid = 0;
+ expect_control_channel_strings();
+ assert_int_equal(send_push_update(m, &cid, msg14, UPT_BY_CID, PUSH_BUNDLE_SIZE_TEST), 1);
+}
+
#undef PUSH_BUNDLE_SIZE_TEST
static int
@@ -535,6 +637,7 @@
cmocka_unit_test_setup_teardown(test_incoming_push_message_mix, setup, teardown),
cmocka_unit_test_setup_teardown(test_incoming_push_message_mix2, setup, teardown),
#ifdef ENABLE_MANAGEMENT
+
cmocka_unit_test_setup_teardown(test_send_push_msg0, setup2, teardown2),
cmocka_unit_test_setup_teardown(test_send_push_msg1, setup2, teardown2),
cmocka_unit_test_setup_teardown(test_send_push_msg2, setup2, teardown2),
@@ -545,7 +648,11 @@
cmocka_unit_test_setup_teardown(test_send_push_msg7, setup2, teardown2),
cmocka_unit_test_setup_teardown(test_send_push_msg8, setup2, teardown2),
cmocka_unit_test_setup_teardown(test_send_push_msg9, setup2, teardown2),
- cmocka_unit_test_setup_teardown(test_send_push_msg10, setup2, teardown2)
+ cmocka_unit_test_setup_teardown(test_send_push_msg10, setup2, teardown2),
+ cmocka_unit_test_setup_teardown(test_send_push_msg11, setup2, teardown2),
+ cmocka_unit_test_setup_teardown(test_send_push_msg12, setup2, teardown2),
+ cmocka_unit_test_setup_teardown(test_send_push_msg13, setup2, teardown2),
+ cmocka_unit_test_setup_teardown(test_send_push_msg14, setup2, teardown2)
#endif
};
--
To view, visit http://gerrit.openvpn.net/c/openvpn/+/1316?usp=email
To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings?usp=email
Gerrit-MessageType: newpatchset
Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: Idba419681fe3ccc4e6e2f6ce7592332dcff62cd9
Gerrit-Change-Number: 1316
Gerrit-PatchSet: 3
Gerrit-Owner: mrbff <ma...@ma...>
Gerrit-Reviewer: flichtenheld <fr...@li...>
Gerrit-Reviewer: plaisthos <arn...@rf...>
Gerrit-CC: openvpn-devel <ope...@li...>
Gerrit-Attention: plaisthos <arn...@rf...>
Gerrit-Attention: flichtenheld <fr...@li...>
|
|
From: flichtenheld (C. Review) <ge...@op...> - 2025-10-29 12:53:44
|
Attention is currently required from: mrbff, plaisthos. flichtenheld has posted comments on this change by mrbff. ( http://gerrit.openvpn.net/c/openvpn/+/1316?usp=email ) Change subject: PUSH_UPDATE server: invalid read bug-fix and unit-tests improvements ...................................................................... Patch Set 3: Code-Review-1 (2 comments) File tests/unit_tests/openvpn/test_push_update_msg.c: http://gerrit.openvpn.net/c/openvpn/+/1316/comment/ad0ce4ab_16f525ad?usp=email : PS3, Line 140: i++; i seems unused? http://gerrit.openvpn.net/c/openvpn/+/1316/comment/b1ef2d25_51152e2b?usp=email : PS3, Line 409: #define expect_control_channel_strings() \ Just give the macro an argument instead of using global res. -- To view, visit http://gerrit.openvpn.net/c/openvpn/+/1316?usp=email To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings?usp=email Gerrit-MessageType: comment Gerrit-Project: openvpn Gerrit-Branch: master Gerrit-Change-Id: Idba419681fe3ccc4e6e2f6ce7592332dcff62cd9 Gerrit-Change-Number: 1316 Gerrit-PatchSet: 3 Gerrit-Owner: mrbff <ma...@ma...> Gerrit-Reviewer: flichtenheld <fr...@li...> Gerrit-Reviewer: plaisthos <arn...@rf...> Gerrit-CC: openvpn-devel <ope...@li...> Gerrit-Attention: plaisthos <arn...@rf...> Gerrit-Attention: mrbff <ma...@ma...> Gerrit-Comment-Date: Wed, 29 Oct 2025 12:53:35 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes |
|
From: mrbff (C. Review) <ge...@op...> - 2025-10-29 13:18:31
|
Attention is currently required from: flichtenheld, mrbff, plaisthos.
Hello flichtenheld, plaisthos,
I'd like you to reexamine a change. Please visit
http://gerrit.openvpn.net/c/openvpn/+/1316?usp=email
to look at the new patch set (#4).
The following approvals got outdated and were removed:
Code-Review-1 by flichtenheld
Change subject: PUSH_UPDATE server: invalid read bug-fix and unit-tests improvements
......................................................................
PUSH_UPDATE server: invalid read bug-fix and unit-tests improvements
The number of messages calculated before the call to message_splitter(), used
in the memory allocation in the buffer array, could in certain cases be less
than one than the actual number of messages, thus causing an override of the
sentinel buffer in message_splitter and therefore an invalid read in
send_single_push_update(). The case in question would be, for example, a
sequence of three options "A, B, C" with the size of B equal to safe_cap - 1
and the sum of the sizes of A and C less than safe_cap - 2.
The buffer array was therefore replaced with a list of buffers to completely
avoid calculating the number of messages before it was actually computed.
The test case in question has been added to the unit tests.
The unit tests have been improved using cmocka macros.
Change-Id: Idba419681fe3ccc4e6e2f6ce7592332dcff62cd9
Signed-off-by: Marco Baffo <ma...@ma...>
---
M src/openvpn/push_util.c
M tests/unit_tests/openvpn/test_push_update_msg.c
2 files changed, 137 insertions(+), 57 deletions(-)
git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/16/1316/4
diff --git a/src/openvpn/push_util.c b/src/openvpn/push_util.c
index 552679a..51c7b5f 100644
--- a/src/openvpn/push_util.c
+++ b/src/openvpn/push_util.c
@@ -91,7 +91,7 @@
* Return `false` on failure an `true` on success.
*/
static bool
-message_splitter(const char *s, struct buffer *msgs, struct gc_arena *gc, const size_t safe_cap)
+message_splitter(const char *s, struct buffer_list *msgs, struct gc_arena *gc, const size_t safe_cap)
{
if (!s || !*s)
{
@@ -100,7 +100,6 @@
char *str = gc_strdup(s, gc);
size_t i = 0;
- int im = 0;
while (*str)
{
@@ -115,37 +114,38 @@
}
str[ci] = '\0';
/* copy from i to (ci -1) */
- msgs[im] = forge_msg(str, ",push-continuation 2", gc);
+ struct buffer tmp = forge_msg(str, ",push-continuation 2", gc);
+ buffer_list_push(msgs, BSTR(&tmp));
i = ci + 1;
}
else
{
- if (im)
+ if (msgs->head)
{
- msgs[im] = forge_msg(str, ",push-continuation 1", gc);
+ struct buffer tmp = forge_msg(str, ",push-continuation 1", gc);
+ buffer_list_push(msgs, BSTR(&tmp));
}
else
{
- msgs[im] = forge_msg(str, NULL, gc);
+ struct buffer tmp = forge_msg(str, NULL, gc);
+ buffer_list_push(msgs, BSTR(&tmp));
}
i = strlen(str);
}
str = &str[i];
- im++;
}
return true;
}
/* send the message(s) prepared to one single client */
static bool
-send_single_push_update(struct multi_context *m, struct multi_instance *mi, struct buffer *msgs)
+send_single_push_update(struct multi_context *m, struct multi_instance *mi, struct buffer_list *msgs)
{
- if (!msgs[0].data || !*(msgs[0].data))
+ if (!msgs->head)
{
return false;
}
- int i = -1;
unsigned int option_types_found = 0;
struct context *c = &mi->context;
struct options o;
@@ -160,9 +160,10 @@
o.ifconfig_local = canary;
o.ifconfig_ipv6_local = canary;
- while (msgs[++i].data && *(msgs[i].data))
+ struct buffer_entry *e = msgs->head;
+ while (e)
{
- if (!send_control_channel_string(c, BSTR(&msgs[i]), D_PUSH))
+ if (!send_control_channel_string(c, BSTR(&e->buf), D_PUSH))
{
return false;
}
@@ -182,13 +183,14 @@
* Also we need to make a temporary copy so we can buf_advance()
* without modifying original buffer.
*/
- struct buffer tmp_msg = msgs[i];
+ struct buffer tmp_msg = e->buf;
buf_string_compare_advance(&tmp_msg, push_update_cmd);
unsigned int permission_mask = pull_permission_mask(c);
if (process_push_update(c, &o, permission_mask, &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->peer_id);
}
+ e = e->next;
}
if (option_types_found & OPT_P_UP)
@@ -270,12 +272,11 @@
* we want to send exceeds that size we have to split it into smaller messages */
ASSERT(push_bundle_size > extra);
const size_t safe_cap = push_bundle_size - extra;
- size_t msgs_num = (strlen(msg) / safe_cap) + ((strlen(msg) % safe_cap) != 0);
- struct buffer *msgs = gc_malloc((msgs_num + 1) * sizeof(struct buffer), true, &gc);
+ struct buffer_list *msgs = buffer_list_new();
- msgs[msgs_num].data = NULL;
if (!message_splitter(msg, msgs, &gc, safe_cap))
{
+ buffer_list_free(msgs);
gc_free(&gc);
return -EINVAL;
}
@@ -286,6 +287,7 @@
if (!mi)
{
+ buffer_list_free(msgs);
gc_free(&gc);
return -ENOENT;
}
@@ -293,6 +295,7 @@
if (!support_push_update(mi))
{
msg(M_CLIENT, "PUSH_UPDATE: not sending message to unsupported peer with ID: %u", mi->context.c2.tls_multi->peer_id);
+ buffer_list_free(msgs);
gc_free(&gc);
return 0;
}
@@ -300,11 +303,13 @@
if (!mi->halt
&& send_single_push_update(m, mi, msgs))
{
+ buffer_list_free(msgs);
gc_free(&gc);
return 1;
}
else
{
+ buffer_list_free(msgs);
gc_free(&gc);
return 0;
}
@@ -334,6 +339,7 @@
}
hash_iterator_free(&hi);
+ buffer_list_free(msgs);
gc_free(&gc);
return count;
}
diff --git a/tests/unit_tests/openvpn/test_push_update_msg.c b/tests/unit_tests/openvpn/test_push_update_msg.c
index 516e94c..6ef1266 100644
--- a/tests/unit_tests/openvpn/test_push_update_msg.c
+++ b/tests/unit_tests/openvpn/test_push_update_msg.c
@@ -130,18 +130,11 @@
return true;
}
#else /* ifndef ENABLE_MANAGEMENT */
-char **res;
-int i;
bool
send_control_channel_string(struct context *c, const char *str, msglvl_t msglevel)
{
- if (res && res[i] && strcmp(res[i], str))
- {
- printf("\n\nexpected: %s\n\n actual: %s\n\n", res[i], str);
- return false;
- }
- i++;
+ check_expected(str);
return true;
}
@@ -301,44 +294,75 @@
#ifdef ENABLE_MANAGEMENT
char *r0[] = {
- "PUSH_UPDATE,redirect-gateway local,route 192.168.1.0 255.255.255.0"
+ "PUSH_UPDATE,redirect-gateway local,route 192.168.1.0 255.255.255.0",
+ NULL
};
char *r1[] = {
"PUSH_UPDATE,-dhcp-option,blablalalalalalalalalalalalalf, lalalalalalalalalalalalalalaf,push-continuation 2",
"PUSH_UPDATE, akakakakakakakakakakakaf, dhcp-option DNS 8.8.8.8,redirect-gateway local,push-continuation 2",
- "PUSH_UPDATE,route 192.168.1.0 255.255.255.0,push-continuation 1"
+ "PUSH_UPDATE,route 192.168.1.0 255.255.255.0,push-continuation 1",
+ NULL
};
char *r3[] = {
- "PUSH_UPDATE,,,"
+ "PUSH_UPDATE,,,",
+ NULL
};
char *r4[] = {
"PUSH_UPDATE,-dhcp-option, blablalalalalalalalalalalalalf, lalalalalalalalalalalalalalaf,push-continuation 2",
"PUSH_UPDATE, akakakakakakakakakakakaf,dhcp-option DNS 8.8.8.8, redirect-gateway local,push-continuation 2",
- "PUSH_UPDATE, route 192.168.1.0 255.255.255.0,,push-continuation 1"
+ "PUSH_UPDATE, route 192.168.1.0 255.255.255.0,,push-continuation 1",
+ NULL
};
char *r5[] = {
"PUSH_UPDATE,,-dhcp-option, blablalalalalalalalalalalalalf, lalalalalalalalalalalalalalaf,push-continuation 2",
"PUSH_UPDATE, akakakakakakakakakakakaf,dhcp-option DNS 8.8.8.8, redirect-gateway local,push-continuation 2",
- "PUSH_UPDATE, route 192.168.1.0 255.255.255.0,push-continuation 1"
+ "PUSH_UPDATE, route 192.168.1.0 255.255.255.0,push-continuation 1",
+ NULL
};
char *r6[] = {
"PUSH_UPDATE,-dhcp-option,blablalalalalalalalalalalalalf, lalalalalalalalalalalalalalaf,push-continuation 2",
"PUSH_UPDATE, akakakakakakakakakakakaf, dhcp-option DNS 8.8.8.8, redirect-gateway 10.10.10.10,,push-continuation 2",
- "PUSH_UPDATE, route 192.168.1.0 255.255.255.0,,push-continuation 1"
+ "PUSH_UPDATE, route 192.168.1.0 255.255.255.0,,push-continuation 1",
+ NULL
};
char *r7[] = {
"PUSH_UPDATE,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,push-continuation 2",
- "PUSH_UPDATE,,,,,,,,,,,,,,,,,,,push-continuation 1"
+ "PUSH_UPDATE,,,,,,,,,,,,,,,,,,,push-continuation 1",
+ NULL
};
char *r8[] = {
"PUSH_UPDATE,-dhcp-option,blablalalalalalalalalalalalalf, lalalalalalalalalalalalalalaf,push-continuation 2",
"PUSH_UPDATE, akakakakakakakakakakakaf, dhcp-option DNS 8.8.8.8,redirect-gateway\n local,push-continuation 2",
- "PUSH_UPDATE,route 192.168.1.0 255.255.255.0\n\n\n,push-continuation 1"
+ "PUSH_UPDATE,route 192.168.1.0 255.255.255.0\n\n\n,push-continuation 1",
+ NULL
};
char *r9[] = {
- "PUSH_UPDATE,,"
+ "PUSH_UPDATE,,",
+ NULL
};
-
+char *r11[] = {
+ "PUSH_UPDATE,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,push-continuation 2",
+ "PUSH_UPDATE,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,push-continuation 2",
+ "PUSH_UPDATE,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,push-continuation 2",
+ "PUSH_UPDATE,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,push-continuation 2",
+ "PUSH_UPDATE,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,push-continuation 1",
+ NULL
+};
+char *r12[] = {
+ "PUSH_UPDATE,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,,,,,,a,push-continuation 2",
+ "PUSH_UPDATE,abc,push-continuation 1",
+ NULL
+};
+char *r13[] = {
+ "PUSH_UPDATE,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,,,,,,a,",
+ NULL
+};
+char *r14[] = {
+ "PUSH_UPDATE,a,push-continuation 2",
+ "PUSH_UPDATE,aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa,push-continuation 2",
+ "PUSH_UPDATE,a,push-continuation 1",
+ NULL
+};
const char *msg0 = "redirect-gateway local,route 192.168.1.0 255.255.255.0";
const char *msg1 = "-dhcp-option,blablalalalalalalalalalalalalf, lalalalalalalalalalalalalalaf,"
@@ -365,32 +389,50 @@
"daisy damage damp dance danger daring dash daughter dawn day deal debate debris decade december decide decline"
"decorate decrease deer defense define defy degree delay deliver demand demise denial";
+const char *msg11 = "a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,"
+ "a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,"
+ "a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,"
+ "a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,"
+ "a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a";
+
+const char *msg12 = "a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,,,,,,a,abc";
+
+const char *msg13 = "a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,,,,,,a,";
+
+const char *msg14 = "a,aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa,a";
+
#define PUSH_BUNDLE_SIZE_TEST 184
+#define expect_control_channel_strings(res) \
+ do \
+ { \
+ for (int j = 0; res[j] != NULL; j++) \
+ { \
+ expect_string(send_control_channel_string, str, res[j]); \
+ } \
+ } while (0)
+
static void
test_send_push_msg0(void **state)
{
- i = 0;
- res = r0;
struct multi_context *m = *state;
const unsigned long cid = 0;
+ expect_control_channel_strings(r0);
assert_int_equal(send_push_update(m, &cid, msg0, UPT_BY_CID, PUSH_BUNDLE_SIZE_TEST), 1);
}
+
static void
test_send_push_msg1(void **state)
{
- i = 0;
- res = r1;
struct multi_context *m = *state;
const unsigned long cid = 0;
+ expect_control_channel_strings(r1);
assert_int_equal(send_push_update(m, &cid, msg1, UPT_BY_CID, PUSH_BUNDLE_SIZE_TEST), 1);
}
static void
test_send_push_msg2(void **state)
{
- i = 0;
- res = NULL;
struct multi_context *m = *state;
const unsigned long cid = 0;
assert_int_equal(send_push_update(m, &cid, msg2, UPT_BY_CID, PUSH_BUNDLE_SIZE_TEST), -EINVAL);
@@ -399,83 +441,110 @@
static void
test_send_push_msg3(void **state)
{
- i = 0;
- res = r3;
struct multi_context *m = *state;
const unsigned long cid = 0;
+ expect_control_channel_strings(r3);
assert_int_equal(send_push_update(m, &cid, msg3, UPT_BY_CID, PUSH_BUNDLE_SIZE_TEST), 1);
}
static void
test_send_push_msg4(void **state)
{
- i = 0;
- res = r4;
struct multi_context *m = *state;
const unsigned long cid = 0;
+ expect_control_channel_strings(r4);
assert_int_equal(send_push_update(m, &cid, msg4, UPT_BY_CID, PUSH_BUNDLE_SIZE_TEST), 1);
}
static void
test_send_push_msg5(void **state)
{
- i = 0;
- res = r5;
struct multi_context *m = *state;
const unsigned long cid = 0;
+ expect_control_channel_strings(r5);
assert_int_equal(send_push_update(m, &cid, msg5, UPT_BY_CID, PUSH_BUNDLE_SIZE_TEST), 1);
}
static void
test_send_push_msg6(void **state)
{
- i = 0;
- res = r6;
struct multi_context *m = *state;
const unsigned long cid = 0;
+ expect_control_channel_strings(r6);
assert_int_equal(send_push_update(m, &cid, msg6, UPT_BY_CID, PUSH_BUNDLE_SIZE_TEST), 1);
}
static void
test_send_push_msg7(void **state)
{
- i = 0;
- res = r7;
struct multi_context *m = *state;
const unsigned long cid = 0;
+ expect_control_channel_strings(r7);
assert_int_equal(send_push_update(m, &cid, msg7, UPT_BY_CID, PUSH_BUNDLE_SIZE_TEST), 1);
}
static void
test_send_push_msg8(void **state)
{
- i = 0;
- res = r8;
struct multi_context *m = *state;
const unsigned long cid = 0;
+ expect_control_channel_strings(r8);
assert_int_equal(send_push_update(m, &cid, msg8, UPT_BY_CID, PUSH_BUNDLE_SIZE_TEST), 1);
}
static void
test_send_push_msg9(void **state)
{
- i = 0;
- res = r9;
struct multi_context *m = *state;
const unsigned long cid = 0;
+ expect_control_channel_strings(r9);
assert_int_equal(send_push_update(m, &cid, msg9, UPT_BY_CID, PUSH_BUNDLE_SIZE_TEST), 1);
}
static void
test_send_push_msg10(void **state)
{
- i = 0;
- res = NULL;
struct multi_context *m = *state;
const unsigned long cid = 0;
assert_int_equal(send_push_update(m, &cid, msg10, UPT_BY_CID, PUSH_BUNDLE_SIZE_TEST), -EINVAL);
}
+static void
+test_send_push_msg11(void **state)
+{
+ struct multi_context *m = *state;
+ const unsigned long cid = 0;
+ expect_control_channel_strings(r11);
+ assert_int_equal(send_push_update(m, &cid, msg11, UPT_BY_CID, PUSH_BUNDLE_SIZE_TEST), 1);
+}
+
+static void
+test_send_push_msg12(void **state)
+{
+ struct multi_context *m = *state;
+ const unsigned long cid = 0;
+ expect_control_channel_strings(r12);
+ assert_int_equal(send_push_update(m, &cid, msg12, UPT_BY_CID, PUSH_BUNDLE_SIZE_TEST), 1);
+}
+
+static void
+test_send_push_msg13(void **state)
+{
+ struct multi_context *m = *state;
+ const unsigned long cid = 0;
+ expect_control_channel_strings(r13);
+ assert_int_equal(send_push_update(m, &cid, msg13, UPT_BY_CID, PUSH_BUNDLE_SIZE_TEST), 1);
+}
+
+static void
+test_send_push_msg14(void **state)
+{
+ struct multi_context *m = *state;
+ const unsigned long cid = 0;
+ expect_control_channel_strings(r14);
+ assert_int_equal(send_push_update(m, &cid, msg14, UPT_BY_CID, PUSH_BUNDLE_SIZE_TEST), 1);
+}
+
#undef PUSH_BUNDLE_SIZE_TEST
static int
@@ -535,6 +604,7 @@
cmocka_unit_test_setup_teardown(test_incoming_push_message_mix, setup, teardown),
cmocka_unit_test_setup_teardown(test_incoming_push_message_mix2, setup, teardown),
#ifdef ENABLE_MANAGEMENT
+
cmocka_unit_test_setup_teardown(test_send_push_msg0, setup2, teardown2),
cmocka_unit_test_setup_teardown(test_send_push_msg1, setup2, teardown2),
cmocka_unit_test_setup_teardown(test_send_push_msg2, setup2, teardown2),
@@ -545,7 +615,11 @@
cmocka_unit_test_setup_teardown(test_send_push_msg7, setup2, teardown2),
cmocka_unit_test_setup_teardown(test_send_push_msg8, setup2, teardown2),
cmocka_unit_test_setup_teardown(test_send_push_msg9, setup2, teardown2),
- cmocka_unit_test_setup_teardown(test_send_push_msg10, setup2, teardown2)
+ cmocka_unit_test_setup_teardown(test_send_push_msg10, setup2, teardown2),
+ cmocka_unit_test_setup_teardown(test_send_push_msg11, setup2, teardown2),
+ cmocka_unit_test_setup_teardown(test_send_push_msg12, setup2, teardown2),
+ cmocka_unit_test_setup_teardown(test_send_push_msg13, setup2, teardown2),
+ cmocka_unit_test_setup_teardown(test_send_push_msg14, setup2, teardown2)
#endif
};
--
To view, visit http://gerrit.openvpn.net/c/openvpn/+/1316?usp=email
To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings?usp=email
Gerrit-MessageType: newpatchset
Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: Idba419681fe3ccc4e6e2f6ce7592332dcff62cd9
Gerrit-Change-Number: 1316
Gerrit-PatchSet: 4
Gerrit-Owner: mrbff <ma...@ma...>
Gerrit-Reviewer: flichtenheld <fr...@li...>
Gerrit-Reviewer: plaisthos <arn...@rf...>
Gerrit-CC: openvpn-devel <ope...@li...>
Gerrit-Attention: plaisthos <arn...@rf...>
Gerrit-Attention: flichtenheld <fr...@li...>
Gerrit-Attention: mrbff <ma...@ma...>
|
|
From: mrbff (C. Review) <ge...@op...> - 2025-10-29 13:19:10
|
Attention is currently required from: flichtenheld, plaisthos. mrbff has posted comments on this change by mrbff. ( http://gerrit.openvpn.net/c/openvpn/+/1316?usp=email ) Change subject: PUSH_UPDATE server: invalid read bug-fix and unit-tests improvements ...................................................................... Patch Set 4: (2 comments) File tests/unit_tests/openvpn/test_push_update_msg.c: http://gerrit.openvpn.net/c/openvpn/+/1316/comment/56b80860_c9cc6bd8?usp=email : PS3, Line 140: i++; > i seems unused? yes, you are right, not needed anymore http://gerrit.openvpn.net/c/openvpn/+/1316/comment/30b59b99_cd44b706?usp=email : PS3, Line 409: #define expect_control_channel_strings() \ > Just give the macro an argument instead of using global res. Done -- To view, visit http://gerrit.openvpn.net/c/openvpn/+/1316?usp=email To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings?usp=email Gerrit-MessageType: comment Gerrit-Project: openvpn Gerrit-Branch: master Gerrit-Change-Id: Idba419681fe3ccc4e6e2f6ce7592332dcff62cd9 Gerrit-Change-Number: 1316 Gerrit-PatchSet: 4 Gerrit-Owner: mrbff <ma...@ma...> Gerrit-Reviewer: flichtenheld <fr...@li...> Gerrit-Reviewer: plaisthos <arn...@rf...> Gerrit-CC: openvpn-devel <ope...@li...> Gerrit-Attention: plaisthos <arn...@rf...> Gerrit-Attention: flichtenheld <fr...@li...> Gerrit-Comment-Date: Wed, 29 Oct 2025 13:18:55 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: flichtenheld <fr...@li...> |
|
From: mrbff (C. Review) <ge...@op...> - 2025-10-29 14:01:06
|
Attention is currently required from: flichtenheld, plaisthos.
Hello flichtenheld, plaisthos,
I'd like you to reexamine a change. Please visit
http://gerrit.openvpn.net/c/openvpn/+/1316?usp=email
to look at the new patch set (#5).
Change subject: PUSH_UPDATE server: invalid read bug-fix and unit-tests improvements
......................................................................
PUSH_UPDATE server: invalid read bug-fix and unit-tests improvements
The number of messages calculated before the call to message_splitter(), used
in the memory allocation in the buffer array, could in certain cases be less
than one than the actual number of messages, thus causing an override of the
sentinel buffer in message_splitter and therefore an invalid read in
send_single_push_update(). The case in question would be, for example, a
sequence of three options "A,B,C" with the size of B equal to safe_cap - 1
and the sum of the sizes of A and C less than safe_cap - 2.
The buffer array was therefore replaced with a list of buffers to completely
avoid calculating the number of messages before it was actually computed.
The test case in question has been added to the unit tests.
The unit tests have been improved using cmocka macros.
Change-Id: Idba419681fe3ccc4e6e2f6ce7592332dcff62cd9
Signed-off-by: Marco Baffo <ma...@ma...>
---
M src/openvpn/push_util.c
M tests/unit_tests/openvpn/test_push_update_msg.c
2 files changed, 137 insertions(+), 57 deletions(-)
git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/16/1316/5
diff --git a/src/openvpn/push_util.c b/src/openvpn/push_util.c
index 552679a..51c7b5f 100644
--- a/src/openvpn/push_util.c
+++ b/src/openvpn/push_util.c
@@ -91,7 +91,7 @@
* Return `false` on failure an `true` on success.
*/
static bool
-message_splitter(const char *s, struct buffer *msgs, struct gc_arena *gc, const size_t safe_cap)
+message_splitter(const char *s, struct buffer_list *msgs, struct gc_arena *gc, const size_t safe_cap)
{
if (!s || !*s)
{
@@ -100,7 +100,6 @@
char *str = gc_strdup(s, gc);
size_t i = 0;
- int im = 0;
while (*str)
{
@@ -115,37 +114,38 @@
}
str[ci] = '\0';
/* copy from i to (ci -1) */
- msgs[im] = forge_msg(str, ",push-continuation 2", gc);
+ struct buffer tmp = forge_msg(str, ",push-continuation 2", gc);
+ buffer_list_push(msgs, BSTR(&tmp));
i = ci + 1;
}
else
{
- if (im)
+ if (msgs->head)
{
- msgs[im] = forge_msg(str, ",push-continuation 1", gc);
+ struct buffer tmp = forge_msg(str, ",push-continuation 1", gc);
+ buffer_list_push(msgs, BSTR(&tmp));
}
else
{
- msgs[im] = forge_msg(str, NULL, gc);
+ struct buffer tmp = forge_msg(str, NULL, gc);
+ buffer_list_push(msgs, BSTR(&tmp));
}
i = strlen(str);
}
str = &str[i];
- im++;
}
return true;
}
/* send the message(s) prepared to one single client */
static bool
-send_single_push_update(struct multi_context *m, struct multi_instance *mi, struct buffer *msgs)
+send_single_push_update(struct multi_context *m, struct multi_instance *mi, struct buffer_list *msgs)
{
- if (!msgs[0].data || !*(msgs[0].data))
+ if (!msgs->head)
{
return false;
}
- int i = -1;
unsigned int option_types_found = 0;
struct context *c = &mi->context;
struct options o;
@@ -160,9 +160,10 @@
o.ifconfig_local = canary;
o.ifconfig_ipv6_local = canary;
- while (msgs[++i].data && *(msgs[i].data))
+ struct buffer_entry *e = msgs->head;
+ while (e)
{
- if (!send_control_channel_string(c, BSTR(&msgs[i]), D_PUSH))
+ if (!send_control_channel_string(c, BSTR(&e->buf), D_PUSH))
{
return false;
}
@@ -182,13 +183,14 @@
* Also we need to make a temporary copy so we can buf_advance()
* without modifying original buffer.
*/
- struct buffer tmp_msg = msgs[i];
+ struct buffer tmp_msg = e->buf;
buf_string_compare_advance(&tmp_msg, push_update_cmd);
unsigned int permission_mask = pull_permission_mask(c);
if (process_push_update(c, &o, permission_mask, &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->peer_id);
}
+ e = e->next;
}
if (option_types_found & OPT_P_UP)
@@ -270,12 +272,11 @@
* we want to send exceeds that size we have to split it into smaller messages */
ASSERT(push_bundle_size > extra);
const size_t safe_cap = push_bundle_size - extra;
- size_t msgs_num = (strlen(msg) / safe_cap) + ((strlen(msg) % safe_cap) != 0);
- struct buffer *msgs = gc_malloc((msgs_num + 1) * sizeof(struct buffer), true, &gc);
+ struct buffer_list *msgs = buffer_list_new();
- msgs[msgs_num].data = NULL;
if (!message_splitter(msg, msgs, &gc, safe_cap))
{
+ buffer_list_free(msgs);
gc_free(&gc);
return -EINVAL;
}
@@ -286,6 +287,7 @@
if (!mi)
{
+ buffer_list_free(msgs);
gc_free(&gc);
return -ENOENT;
}
@@ -293,6 +295,7 @@
if (!support_push_update(mi))
{
msg(M_CLIENT, "PUSH_UPDATE: not sending message to unsupported peer with ID: %u", mi->context.c2.tls_multi->peer_id);
+ buffer_list_free(msgs);
gc_free(&gc);
return 0;
}
@@ -300,11 +303,13 @@
if (!mi->halt
&& send_single_push_update(m, mi, msgs))
{
+ buffer_list_free(msgs);
gc_free(&gc);
return 1;
}
else
{
+ buffer_list_free(msgs);
gc_free(&gc);
return 0;
}
@@ -334,6 +339,7 @@
}
hash_iterator_free(&hi);
+ buffer_list_free(msgs);
gc_free(&gc);
return count;
}
diff --git a/tests/unit_tests/openvpn/test_push_update_msg.c b/tests/unit_tests/openvpn/test_push_update_msg.c
index 516e94c..6ef1266 100644
--- a/tests/unit_tests/openvpn/test_push_update_msg.c
+++ b/tests/unit_tests/openvpn/test_push_update_msg.c
@@ -130,18 +130,11 @@
return true;
}
#else /* ifndef ENABLE_MANAGEMENT */
-char **res;
-int i;
bool
send_control_channel_string(struct context *c, const char *str, msglvl_t msglevel)
{
- if (res && res[i] && strcmp(res[i], str))
- {
- printf("\n\nexpected: %s\n\n actual: %s\n\n", res[i], str);
- return false;
- }
- i++;
+ check_expected(str);
return true;
}
@@ -301,44 +294,75 @@
#ifdef ENABLE_MANAGEMENT
char *r0[] = {
- "PUSH_UPDATE,redirect-gateway local,route 192.168.1.0 255.255.255.0"
+ "PUSH_UPDATE,redirect-gateway local,route 192.168.1.0 255.255.255.0",
+ NULL
};
char *r1[] = {
"PUSH_UPDATE,-dhcp-option,blablalalalalalalalalalalalalf, lalalalalalalalalalalalalalaf,push-continuation 2",
"PUSH_UPDATE, akakakakakakakakakakakaf, dhcp-option DNS 8.8.8.8,redirect-gateway local,push-continuation 2",
- "PUSH_UPDATE,route 192.168.1.0 255.255.255.0,push-continuation 1"
+ "PUSH_UPDATE,route 192.168.1.0 255.255.255.0,push-continuation 1",
+ NULL
};
char *r3[] = {
- "PUSH_UPDATE,,,"
+ "PUSH_UPDATE,,,",
+ NULL
};
char *r4[] = {
"PUSH_UPDATE,-dhcp-option, blablalalalalalalalalalalalalf, lalalalalalalalalalalalalalaf,push-continuation 2",
"PUSH_UPDATE, akakakakakakakakakakakaf,dhcp-option DNS 8.8.8.8, redirect-gateway local,push-continuation 2",
- "PUSH_UPDATE, route 192.168.1.0 255.255.255.0,,push-continuation 1"
+ "PUSH_UPDATE, route 192.168.1.0 255.255.255.0,,push-continuation 1",
+ NULL
};
char *r5[] = {
"PUSH_UPDATE,,-dhcp-option, blablalalalalalalalalalalalalf, lalalalalalalalalalalalalalaf,push-continuation 2",
"PUSH_UPDATE, akakakakakakakakakakakaf,dhcp-option DNS 8.8.8.8, redirect-gateway local,push-continuation 2",
- "PUSH_UPDATE, route 192.168.1.0 255.255.255.0,push-continuation 1"
+ "PUSH_UPDATE, route 192.168.1.0 255.255.255.0,push-continuation 1",
+ NULL
};
char *r6[] = {
"PUSH_UPDATE,-dhcp-option,blablalalalalalalalalalalalalf, lalalalalalalalalalalalalalaf,push-continuation 2",
"PUSH_UPDATE, akakakakakakakakakakakaf, dhcp-option DNS 8.8.8.8, redirect-gateway 10.10.10.10,,push-continuation 2",
- "PUSH_UPDATE, route 192.168.1.0 255.255.255.0,,push-continuation 1"
+ "PUSH_UPDATE, route 192.168.1.0 255.255.255.0,,push-continuation 1",
+ NULL
};
char *r7[] = {
"PUSH_UPDATE,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,push-continuation 2",
- "PUSH_UPDATE,,,,,,,,,,,,,,,,,,,push-continuation 1"
+ "PUSH_UPDATE,,,,,,,,,,,,,,,,,,,push-continuation 1",
+ NULL
};
char *r8[] = {
"PUSH_UPDATE,-dhcp-option,blablalalalalalalalalalalalalf, lalalalalalalalalalalalalalaf,push-continuation 2",
"PUSH_UPDATE, akakakakakakakakakakakaf, dhcp-option DNS 8.8.8.8,redirect-gateway\n local,push-continuation 2",
- "PUSH_UPDATE,route 192.168.1.0 255.255.255.0\n\n\n,push-continuation 1"
+ "PUSH_UPDATE,route 192.168.1.0 255.255.255.0\n\n\n,push-continuation 1",
+ NULL
};
char *r9[] = {
- "PUSH_UPDATE,,"
+ "PUSH_UPDATE,,",
+ NULL
};
-
+char *r11[] = {
+ "PUSH_UPDATE,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,push-continuation 2",
+ "PUSH_UPDATE,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,push-continuation 2",
+ "PUSH_UPDATE,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,push-continuation 2",
+ "PUSH_UPDATE,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,push-continuation 2",
+ "PUSH_UPDATE,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,push-continuation 1",
+ NULL
+};
+char *r12[] = {
+ "PUSH_UPDATE,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,,,,,,a,push-continuation 2",
+ "PUSH_UPDATE,abc,push-continuation 1",
+ NULL
+};
+char *r13[] = {
+ "PUSH_UPDATE,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,,,,,,a,",
+ NULL
+};
+char *r14[] = {
+ "PUSH_UPDATE,a,push-continuation 2",
+ "PUSH_UPDATE,aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa,push-continuation 2",
+ "PUSH_UPDATE,a,push-continuation 1",
+ NULL
+};
const char *msg0 = "redirect-gateway local,route 192.168.1.0 255.255.255.0";
const char *msg1 = "-dhcp-option,blablalalalalalalalalalalalalf, lalalalalalalalalalalalalalaf,"
@@ -365,32 +389,50 @@
"daisy damage damp dance danger daring dash daughter dawn day deal debate debris decade december decide decline"
"decorate decrease deer defense define defy degree delay deliver demand demise denial";
+const char *msg11 = "a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,"
+ "a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,"
+ "a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,"
+ "a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,"
+ "a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a";
+
+const char *msg12 = "a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,,,,,,a,abc";
+
+const char *msg13 = "a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,,,,,,a,";
+
+const char *msg14 = "a,aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa,a";
+
#define PUSH_BUNDLE_SIZE_TEST 184
+#define expect_control_channel_strings(res) \
+ do \
+ { \
+ for (int j = 0; res[j] != NULL; j++) \
+ { \
+ expect_string(send_control_channel_string, str, res[j]); \
+ } \
+ } while (0)
+
static void
test_send_push_msg0(void **state)
{
- i = 0;
- res = r0;
struct multi_context *m = *state;
const unsigned long cid = 0;
+ expect_control_channel_strings(r0);
assert_int_equal(send_push_update(m, &cid, msg0, UPT_BY_CID, PUSH_BUNDLE_SIZE_TEST), 1);
}
+
static void
test_send_push_msg1(void **state)
{
- i = 0;
- res = r1;
struct multi_context *m = *state;
const unsigned long cid = 0;
+ expect_control_channel_strings(r1);
assert_int_equal(send_push_update(m, &cid, msg1, UPT_BY_CID, PUSH_BUNDLE_SIZE_TEST), 1);
}
static void
test_send_push_msg2(void **state)
{
- i = 0;
- res = NULL;
struct multi_context *m = *state;
const unsigned long cid = 0;
assert_int_equal(send_push_update(m, &cid, msg2, UPT_BY_CID, PUSH_BUNDLE_SIZE_TEST), -EINVAL);
@@ -399,83 +441,110 @@
static void
test_send_push_msg3(void **state)
{
- i = 0;
- res = r3;
struct multi_context *m = *state;
const unsigned long cid = 0;
+ expect_control_channel_strings(r3);
assert_int_equal(send_push_update(m, &cid, msg3, UPT_BY_CID, PUSH_BUNDLE_SIZE_TEST), 1);
}
static void
test_send_push_msg4(void **state)
{
- i = 0;
- res = r4;
struct multi_context *m = *state;
const unsigned long cid = 0;
+ expect_control_channel_strings(r4);
assert_int_equal(send_push_update(m, &cid, msg4, UPT_BY_CID, PUSH_BUNDLE_SIZE_TEST), 1);
}
static void
test_send_push_msg5(void **state)
{
- i = 0;
- res = r5;
struct multi_context *m = *state;
const unsigned long cid = 0;
+ expect_control_channel_strings(r5);
assert_int_equal(send_push_update(m, &cid, msg5, UPT_BY_CID, PUSH_BUNDLE_SIZE_TEST), 1);
}
static void
test_send_push_msg6(void **state)
{
- i = 0;
- res = r6;
struct multi_context *m = *state;
const unsigned long cid = 0;
+ expect_control_channel_strings(r6);
assert_int_equal(send_push_update(m, &cid, msg6, UPT_BY_CID, PUSH_BUNDLE_SIZE_TEST), 1);
}
static void
test_send_push_msg7(void **state)
{
- i = 0;
- res = r7;
struct multi_context *m = *state;
const unsigned long cid = 0;
+ expect_control_channel_strings(r7);
assert_int_equal(send_push_update(m, &cid, msg7, UPT_BY_CID, PUSH_BUNDLE_SIZE_TEST), 1);
}
static void
test_send_push_msg8(void **state)
{
- i = 0;
- res = r8;
struct multi_context *m = *state;
const unsigned long cid = 0;
+ expect_control_channel_strings(r8);
assert_int_equal(send_push_update(m, &cid, msg8, UPT_BY_CID, PUSH_BUNDLE_SIZE_TEST), 1);
}
static void
test_send_push_msg9(void **state)
{
- i = 0;
- res = r9;
struct multi_context *m = *state;
const unsigned long cid = 0;
+ expect_control_channel_strings(r9);
assert_int_equal(send_push_update(m, &cid, msg9, UPT_BY_CID, PUSH_BUNDLE_SIZE_TEST), 1);
}
static void
test_send_push_msg10(void **state)
{
- i = 0;
- res = NULL;
struct multi_context *m = *state;
const unsigned long cid = 0;
assert_int_equal(send_push_update(m, &cid, msg10, UPT_BY_CID, PUSH_BUNDLE_SIZE_TEST), -EINVAL);
}
+static void
+test_send_push_msg11(void **state)
+{
+ struct multi_context *m = *state;
+ const unsigned long cid = 0;
+ expect_control_channel_strings(r11);
+ assert_int_equal(send_push_update(m, &cid, msg11, UPT_BY_CID, PUSH_BUNDLE_SIZE_TEST), 1);
+}
+
+static void
+test_send_push_msg12(void **state)
+{
+ struct multi_context *m = *state;
+ const unsigned long cid = 0;
+ expect_control_channel_strings(r12);
+ assert_int_equal(send_push_update(m, &cid, msg12, UPT_BY_CID, PUSH_BUNDLE_SIZE_TEST), 1);
+}
+
+static void
+test_send_push_msg13(void **state)
+{
+ struct multi_context *m = *state;
+ const unsigned long cid = 0;
+ expect_control_channel_strings(r13);
+ assert_int_equal(send_push_update(m, &cid, msg13, UPT_BY_CID, PUSH_BUNDLE_SIZE_TEST), 1);
+}
+
+static void
+test_send_push_msg14(void **state)
+{
+ struct multi_context *m = *state;
+ const unsigned long cid = 0;
+ expect_control_channel_strings(r14);
+ assert_int_equal(send_push_update(m, &cid, msg14, UPT_BY_CID, PUSH_BUNDLE_SIZE_TEST), 1);
+}
+
#undef PUSH_BUNDLE_SIZE_TEST
static int
@@ -535,6 +604,7 @@
cmocka_unit_test_setup_teardown(test_incoming_push_message_mix, setup, teardown),
cmocka_unit_test_setup_teardown(test_incoming_push_message_mix2, setup, teardown),
#ifdef ENABLE_MANAGEMENT
+
cmocka_unit_test_setup_teardown(test_send_push_msg0, setup2, teardown2),
cmocka_unit_test_setup_teardown(test_send_push_msg1, setup2, teardown2),
cmocka_unit_test_setup_teardown(test_send_push_msg2, setup2, teardown2),
@@ -545,7 +615,11 @@
cmocka_unit_test_setup_teardown(test_send_push_msg7, setup2, teardown2),
cmocka_unit_test_setup_teardown(test_send_push_msg8, setup2, teardown2),
cmocka_unit_test_setup_teardown(test_send_push_msg9, setup2, teardown2),
- cmocka_unit_test_setup_teardown(test_send_push_msg10, setup2, teardown2)
+ cmocka_unit_test_setup_teardown(test_send_push_msg10, setup2, teardown2),
+ cmocka_unit_test_setup_teardown(test_send_push_msg11, setup2, teardown2),
+ cmocka_unit_test_setup_teardown(test_send_push_msg12, setup2, teardown2),
+ cmocka_unit_test_setup_teardown(test_send_push_msg13, setup2, teardown2),
+ cmocka_unit_test_setup_teardown(test_send_push_msg14, setup2, teardown2)
#endif
};
--
To view, visit http://gerrit.openvpn.net/c/openvpn/+/1316?usp=email
To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings?usp=email
Gerrit-MessageType: newpatchset
Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: Idba419681fe3ccc4e6e2f6ce7592332dcff62cd9
Gerrit-Change-Number: 1316
Gerrit-PatchSet: 5
Gerrit-Owner: mrbff <ma...@ma...>
Gerrit-Reviewer: flichtenheld <fr...@li...>
Gerrit-Reviewer: plaisthos <arn...@rf...>
Gerrit-CC: openvpn-devel <ope...@li...>
Gerrit-Attention: plaisthos <arn...@rf...>
Gerrit-Attention: flichtenheld <fr...@li...>
|
|
From: mrbff (C. Review) <ge...@op...> - 2025-10-29 14:03:50
|
Attention is currently required from: flichtenheld, plaisthos.
Hello flichtenheld, plaisthos,
I'd like you to reexamine a change. Please visit
http://gerrit.openvpn.net/c/openvpn/+/1316?usp=email
to look at the new patch set (#6).
Change subject: PUSH_UPDATE server: invalid read bug-fix and unit-tests improvements
......................................................................
PUSH_UPDATE server: invalid read bug-fix and unit-tests improvements
The number of messages calculated before the call to message_splitter(),
used in the memory allocation in the buffer array, could in certain
cases be less than one than the actual number of messages, thus causing
an override of the sentinel buffer in message_splitter() and therefore
an invalid read in send_single_push_update().
The case in question would be, for example, a sequence of three options
"A,B,C" with the size of B equal to safe_cap - 1 and the sum of the
sizes of A and C less than safe_cap - 2.
The buffer array was therefore replaced with a list of buffers to
completely avoid calculating the number of messages before it was
actually computed.
The test case in question has been added to the unit tests.
The unit tests have been improved using cmocka macros.
Change-Id: Idba419681fe3ccc4e6e2f6ce7592332dcff62cd9
Signed-off-by: Marco Baffo <ma...@ma...>
---
M src/openvpn/push_util.c
M tests/unit_tests/openvpn/test_push_update_msg.c
2 files changed, 137 insertions(+), 57 deletions(-)
git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/16/1316/6
diff --git a/src/openvpn/push_util.c b/src/openvpn/push_util.c
index 552679a..51c7b5f 100644
--- a/src/openvpn/push_util.c
+++ b/src/openvpn/push_util.c
@@ -91,7 +91,7 @@
* Return `false` on failure an `true` on success.
*/
static bool
-message_splitter(const char *s, struct buffer *msgs, struct gc_arena *gc, const size_t safe_cap)
+message_splitter(const char *s, struct buffer_list *msgs, struct gc_arena *gc, const size_t safe_cap)
{
if (!s || !*s)
{
@@ -100,7 +100,6 @@
char *str = gc_strdup(s, gc);
size_t i = 0;
- int im = 0;
while (*str)
{
@@ -115,37 +114,38 @@
}
str[ci] = '\0';
/* copy from i to (ci -1) */
- msgs[im] = forge_msg(str, ",push-continuation 2", gc);
+ struct buffer tmp = forge_msg(str, ",push-continuation 2", gc);
+ buffer_list_push(msgs, BSTR(&tmp));
i = ci + 1;
}
else
{
- if (im)
+ if (msgs->head)
{
- msgs[im] = forge_msg(str, ",push-continuation 1", gc);
+ struct buffer tmp = forge_msg(str, ",push-continuation 1", gc);
+ buffer_list_push(msgs, BSTR(&tmp));
}
else
{
- msgs[im] = forge_msg(str, NULL, gc);
+ struct buffer tmp = forge_msg(str, NULL, gc);
+ buffer_list_push(msgs, BSTR(&tmp));
}
i = strlen(str);
}
str = &str[i];
- im++;
}
return true;
}
/* send the message(s) prepared to one single client */
static bool
-send_single_push_update(struct multi_context *m, struct multi_instance *mi, struct buffer *msgs)
+send_single_push_update(struct multi_context *m, struct multi_instance *mi, struct buffer_list *msgs)
{
- if (!msgs[0].data || !*(msgs[0].data))
+ if (!msgs->head)
{
return false;
}
- int i = -1;
unsigned int option_types_found = 0;
struct context *c = &mi->context;
struct options o;
@@ -160,9 +160,10 @@
o.ifconfig_local = canary;
o.ifconfig_ipv6_local = canary;
- while (msgs[++i].data && *(msgs[i].data))
+ struct buffer_entry *e = msgs->head;
+ while (e)
{
- if (!send_control_channel_string(c, BSTR(&msgs[i]), D_PUSH))
+ if (!send_control_channel_string(c, BSTR(&e->buf), D_PUSH))
{
return false;
}
@@ -182,13 +183,14 @@
* Also we need to make a temporary copy so we can buf_advance()
* without modifying original buffer.
*/
- struct buffer tmp_msg = msgs[i];
+ struct buffer tmp_msg = e->buf;
buf_string_compare_advance(&tmp_msg, push_update_cmd);
unsigned int permission_mask = pull_permission_mask(c);
if (process_push_update(c, &o, permission_mask, &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->peer_id);
}
+ e = e->next;
}
if (option_types_found & OPT_P_UP)
@@ -270,12 +272,11 @@
* we want to send exceeds that size we have to split it into smaller messages */
ASSERT(push_bundle_size > extra);
const size_t safe_cap = push_bundle_size - extra;
- size_t msgs_num = (strlen(msg) / safe_cap) + ((strlen(msg) % safe_cap) != 0);
- struct buffer *msgs = gc_malloc((msgs_num + 1) * sizeof(struct buffer), true, &gc);
+ struct buffer_list *msgs = buffer_list_new();
- msgs[msgs_num].data = NULL;
if (!message_splitter(msg, msgs, &gc, safe_cap))
{
+ buffer_list_free(msgs);
gc_free(&gc);
return -EINVAL;
}
@@ -286,6 +287,7 @@
if (!mi)
{
+ buffer_list_free(msgs);
gc_free(&gc);
return -ENOENT;
}
@@ -293,6 +295,7 @@
if (!support_push_update(mi))
{
msg(M_CLIENT, "PUSH_UPDATE: not sending message to unsupported peer with ID: %u", mi->context.c2.tls_multi->peer_id);
+ buffer_list_free(msgs);
gc_free(&gc);
return 0;
}
@@ -300,11 +303,13 @@
if (!mi->halt
&& send_single_push_update(m, mi, msgs))
{
+ buffer_list_free(msgs);
gc_free(&gc);
return 1;
}
else
{
+ buffer_list_free(msgs);
gc_free(&gc);
return 0;
}
@@ -334,6 +339,7 @@
}
hash_iterator_free(&hi);
+ buffer_list_free(msgs);
gc_free(&gc);
return count;
}
diff --git a/tests/unit_tests/openvpn/test_push_update_msg.c b/tests/unit_tests/openvpn/test_push_update_msg.c
index 516e94c..6ef1266 100644
--- a/tests/unit_tests/openvpn/test_push_update_msg.c
+++ b/tests/unit_tests/openvpn/test_push_update_msg.c
@@ -130,18 +130,11 @@
return true;
}
#else /* ifndef ENABLE_MANAGEMENT */
-char **res;
-int i;
bool
send_control_channel_string(struct context *c, const char *str, msglvl_t msglevel)
{
- if (res && res[i] && strcmp(res[i], str))
- {
- printf("\n\nexpected: %s\n\n actual: %s\n\n", res[i], str);
- return false;
- }
- i++;
+ check_expected(str);
return true;
}
@@ -301,44 +294,75 @@
#ifdef ENABLE_MANAGEMENT
char *r0[] = {
- "PUSH_UPDATE,redirect-gateway local,route 192.168.1.0 255.255.255.0"
+ "PUSH_UPDATE,redirect-gateway local,route 192.168.1.0 255.255.255.0",
+ NULL
};
char *r1[] = {
"PUSH_UPDATE,-dhcp-option,blablalalalalalalalalalalalalf, lalalalalalalalalalalalalalaf,push-continuation 2",
"PUSH_UPDATE, akakakakakakakakakakakaf, dhcp-option DNS 8.8.8.8,redirect-gateway local,push-continuation 2",
- "PUSH_UPDATE,route 192.168.1.0 255.255.255.0,push-continuation 1"
+ "PUSH_UPDATE,route 192.168.1.0 255.255.255.0,push-continuation 1",
+ NULL
};
char *r3[] = {
- "PUSH_UPDATE,,,"
+ "PUSH_UPDATE,,,",
+ NULL
};
char *r4[] = {
"PUSH_UPDATE,-dhcp-option, blablalalalalalalalalalalalalf, lalalalalalalalalalalalalalaf,push-continuation 2",
"PUSH_UPDATE, akakakakakakakakakakakaf,dhcp-option DNS 8.8.8.8, redirect-gateway local,push-continuation 2",
- "PUSH_UPDATE, route 192.168.1.0 255.255.255.0,,push-continuation 1"
+ "PUSH_UPDATE, route 192.168.1.0 255.255.255.0,,push-continuation 1",
+ NULL
};
char *r5[] = {
"PUSH_UPDATE,,-dhcp-option, blablalalalalalalalalalalalalf, lalalalalalalalalalalalalalaf,push-continuation 2",
"PUSH_UPDATE, akakakakakakakakakakakaf,dhcp-option DNS 8.8.8.8, redirect-gateway local,push-continuation 2",
- "PUSH_UPDATE, route 192.168.1.0 255.255.255.0,push-continuation 1"
+ "PUSH_UPDATE, route 192.168.1.0 255.255.255.0,push-continuation 1",
+ NULL
};
char *r6[] = {
"PUSH_UPDATE,-dhcp-option,blablalalalalalalalalalalalalf, lalalalalalalalalalalalalalaf,push-continuation 2",
"PUSH_UPDATE, akakakakakakakakakakakaf, dhcp-option DNS 8.8.8.8, redirect-gateway 10.10.10.10,,push-continuation 2",
- "PUSH_UPDATE, route 192.168.1.0 255.255.255.0,,push-continuation 1"
+ "PUSH_UPDATE, route 192.168.1.0 255.255.255.0,,push-continuation 1",
+ NULL
};
char *r7[] = {
"PUSH_UPDATE,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,push-continuation 2",
- "PUSH_UPDATE,,,,,,,,,,,,,,,,,,,push-continuation 1"
+ "PUSH_UPDATE,,,,,,,,,,,,,,,,,,,push-continuation 1",
+ NULL
};
char *r8[] = {
"PUSH_UPDATE,-dhcp-option,blablalalalalalalalalalalalalf, lalalalalalalalalalalalalalaf,push-continuation 2",
"PUSH_UPDATE, akakakakakakakakakakakaf, dhcp-option DNS 8.8.8.8,redirect-gateway\n local,push-continuation 2",
- "PUSH_UPDATE,route 192.168.1.0 255.255.255.0\n\n\n,push-continuation 1"
+ "PUSH_UPDATE,route 192.168.1.0 255.255.255.0\n\n\n,push-continuation 1",
+ NULL
};
char *r9[] = {
- "PUSH_UPDATE,,"
+ "PUSH_UPDATE,,",
+ NULL
};
-
+char *r11[] = {
+ "PUSH_UPDATE,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,push-continuation 2",
+ "PUSH_UPDATE,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,push-continuation 2",
+ "PUSH_UPDATE,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,push-continuation 2",
+ "PUSH_UPDATE,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,push-continuation 2",
+ "PUSH_UPDATE,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,push-continuation 1",
+ NULL
+};
+char *r12[] = {
+ "PUSH_UPDATE,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,,,,,,a,push-continuation 2",
+ "PUSH_UPDATE,abc,push-continuation 1",
+ NULL
+};
+char *r13[] = {
+ "PUSH_UPDATE,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,,,,,,a,",
+ NULL
+};
+char *r14[] = {
+ "PUSH_UPDATE,a,push-continuation 2",
+ "PUSH_UPDATE,aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa,push-continuation 2",
+ "PUSH_UPDATE,a,push-continuation 1",
+ NULL
+};
const char *msg0 = "redirect-gateway local,route 192.168.1.0 255.255.255.0";
const char *msg1 = "-dhcp-option,blablalalalalalalalalalalalalf, lalalalalalalalalalalalalalaf,"
@@ -365,32 +389,50 @@
"daisy damage damp dance danger daring dash daughter dawn day deal debate debris decade december decide decline"
"decorate decrease deer defense define defy degree delay deliver demand demise denial";
+const char *msg11 = "a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,"
+ "a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,"
+ "a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,"
+ "a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,"
+ "a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a";
+
+const char *msg12 = "a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,,,,,,a,abc";
+
+const char *msg13 = "a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,,,,,,a,";
+
+const char *msg14 = "a,aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa,a";
+
#define PUSH_BUNDLE_SIZE_TEST 184
+#define expect_control_channel_strings(res) \
+ do \
+ { \
+ for (int j = 0; res[j] != NULL; j++) \
+ { \
+ expect_string(send_control_channel_string, str, res[j]); \
+ } \
+ } while (0)
+
static void
test_send_push_msg0(void **state)
{
- i = 0;
- res = r0;
struct multi_context *m = *state;
const unsigned long cid = 0;
+ expect_control_channel_strings(r0);
assert_int_equal(send_push_update(m, &cid, msg0, UPT_BY_CID, PUSH_BUNDLE_SIZE_TEST), 1);
}
+
static void
test_send_push_msg1(void **state)
{
- i = 0;
- res = r1;
struct multi_context *m = *state;
const unsigned long cid = 0;
+ expect_control_channel_strings(r1);
assert_int_equal(send_push_update(m, &cid, msg1, UPT_BY_CID, PUSH_BUNDLE_SIZE_TEST), 1);
}
static void
test_send_push_msg2(void **state)
{
- i = 0;
- res = NULL;
struct multi_context *m = *state;
const unsigned long cid = 0;
assert_int_equal(send_push_update(m, &cid, msg2, UPT_BY_CID, PUSH_BUNDLE_SIZE_TEST), -EINVAL);
@@ -399,83 +441,110 @@
static void
test_send_push_msg3(void **state)
{
- i = 0;
- res = r3;
struct multi_context *m = *state;
const unsigned long cid = 0;
+ expect_control_channel_strings(r3);
assert_int_equal(send_push_update(m, &cid, msg3, UPT_BY_CID, PUSH_BUNDLE_SIZE_TEST), 1);
}
static void
test_send_push_msg4(void **state)
{
- i = 0;
- res = r4;
struct multi_context *m = *state;
const unsigned long cid = 0;
+ expect_control_channel_strings(r4);
assert_int_equal(send_push_update(m, &cid, msg4, UPT_BY_CID, PUSH_BUNDLE_SIZE_TEST), 1);
}
static void
test_send_push_msg5(void **state)
{
- i = 0;
- res = r5;
struct multi_context *m = *state;
const unsigned long cid = 0;
+ expect_control_channel_strings(r5);
assert_int_equal(send_push_update(m, &cid, msg5, UPT_BY_CID, PUSH_BUNDLE_SIZE_TEST), 1);
}
static void
test_send_push_msg6(void **state)
{
- i = 0;
- res = r6;
struct multi_context *m = *state;
const unsigned long cid = 0;
+ expect_control_channel_strings(r6);
assert_int_equal(send_push_update(m, &cid, msg6, UPT_BY_CID, PUSH_BUNDLE_SIZE_TEST), 1);
}
static void
test_send_push_msg7(void **state)
{
- i = 0;
- res = r7;
struct multi_context *m = *state;
const unsigned long cid = 0;
+ expect_control_channel_strings(r7);
assert_int_equal(send_push_update(m, &cid, msg7, UPT_BY_CID, PUSH_BUNDLE_SIZE_TEST), 1);
}
static void
test_send_push_msg8(void **state)
{
- i = 0;
- res = r8;
struct multi_context *m = *state;
const unsigned long cid = 0;
+ expect_control_channel_strings(r8);
assert_int_equal(send_push_update(m, &cid, msg8, UPT_BY_CID, PUSH_BUNDLE_SIZE_TEST), 1);
}
static void
test_send_push_msg9(void **state)
{
- i = 0;
- res = r9;
struct multi_context *m = *state;
const unsigned long cid = 0;
+ expect_control_channel_strings(r9);
assert_int_equal(send_push_update(m, &cid, msg9, UPT_BY_CID, PUSH_BUNDLE_SIZE_TEST), 1);
}
static void
test_send_push_msg10(void **state)
{
- i = 0;
- res = NULL;
struct multi_context *m = *state;
const unsigned long cid = 0;
assert_int_equal(send_push_update(m, &cid, msg10, UPT_BY_CID, PUSH_BUNDLE_SIZE_TEST), -EINVAL);
}
+static void
+test_send_push_msg11(void **state)
+{
+ struct multi_context *m = *state;
+ const unsigned long cid = 0;
+ expect_control_channel_strings(r11);
+ assert_int_equal(send_push_update(m, &cid, msg11, UPT_BY_CID, PUSH_BUNDLE_SIZE_TEST), 1);
+}
+
+static void
+test_send_push_msg12(void **state)
+{
+ struct multi_context *m = *state;
+ const unsigned long cid = 0;
+ expect_control_channel_strings(r12);
+ assert_int_equal(send_push_update(m, &cid, msg12, UPT_BY_CID, PUSH_BUNDLE_SIZE_TEST), 1);
+}
+
+static void
+test_send_push_msg13(void **state)
+{
+ struct multi_context *m = *state;
+ const unsigned long cid = 0;
+ expect_control_channel_strings(r13);
+ assert_int_equal(send_push_update(m, &cid, msg13, UPT_BY_CID, PUSH_BUNDLE_SIZE_TEST), 1);
+}
+
+static void
+test_send_push_msg14(void **state)
+{
+ struct multi_context *m = *state;
+ const unsigned long cid = 0;
+ expect_control_channel_strings(r14);
+ assert_int_equal(send_push_update(m, &cid, msg14, UPT_BY_CID, PUSH_BUNDLE_SIZE_TEST), 1);
+}
+
#undef PUSH_BUNDLE_SIZE_TEST
static int
@@ -535,6 +604,7 @@
cmocka_unit_test_setup_teardown(test_incoming_push_message_mix, setup, teardown),
cmocka_unit_test_setup_teardown(test_incoming_push_message_mix2, setup, teardown),
#ifdef ENABLE_MANAGEMENT
+
cmocka_unit_test_setup_teardown(test_send_push_msg0, setup2, teardown2),
cmocka_unit_test_setup_teardown(test_send_push_msg1, setup2, teardown2),
cmocka_unit_test_setup_teardown(test_send_push_msg2, setup2, teardown2),
@@ -545,7 +615,11 @@
cmocka_unit_test_setup_teardown(test_send_push_msg7, setup2, teardown2),
cmocka_unit_test_setup_teardown(test_send_push_msg8, setup2, teardown2),
cmocka_unit_test_setup_teardown(test_send_push_msg9, setup2, teardown2),
- cmocka_unit_test_setup_teardown(test_send_push_msg10, setup2, teardown2)
+ cmocka_unit_test_setup_teardown(test_send_push_msg10, setup2, teardown2),
+ cmocka_unit_test_setup_teardown(test_send_push_msg11, setup2, teardown2),
+ cmocka_unit_test_setup_teardown(test_send_push_msg12, setup2, teardown2),
+ cmocka_unit_test_setup_teardown(test_send_push_msg13, setup2, teardown2),
+ cmocka_unit_test_setup_teardown(test_send_push_msg14, setup2, teardown2)
#endif
};
--
To view, visit http://gerrit.openvpn.net/c/openvpn/+/1316?usp=email
To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings?usp=email
Gerrit-MessageType: newpatchset
Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: Idba419681fe3ccc4e6e2f6ce7592332dcff62cd9
Gerrit-Change-Number: 1316
Gerrit-PatchSet: 6
Gerrit-Owner: mrbff <ma...@ma...>
Gerrit-Reviewer: flichtenheld <fr...@li...>
Gerrit-Reviewer: plaisthos <arn...@rf...>
Gerrit-CC: openvpn-devel <ope...@li...>
Gerrit-Attention: plaisthos <arn...@rf...>
Gerrit-Attention: flichtenheld <fr...@li...>
|