buffer_list_aggregate_separator(): add unit tests

PATCH In review Steffan Karger 5 participants 27 messages Jul 1, 2017 – Oct 16, 2018

lore

Share & tools

lore

Steffan Karger

author

2017-07-01 12:53 UTC #0 lore raw

PATCH 1/5 buffer_list_aggregate_separator(): add unit tests

Before refactoring this function, add tests to verify the new implementation.

While writing the tests, it became clear that this function is not behaving very well. We'll fix that in follow-up commits.

Signed-off-by Steffan Karger <[email protected]...>

---
 tests/unit_tests/openvpn/test_buffer.c | 181 ++++++++++++++++++++++++++++++++-
 1 file changed, 179 insertions(+), 2 deletions(-)

tests/unit_tests/openvpn/test_buffer.c +179 -2

index 69bb2e5..acfeb1a 100644
@@ -35,7 +35,7 @@
 #include "buffer.h"
 
 static void
-buffer_strprefix(void **state)
+test_buffer_strprefix(void **state)
 {
     assert_true(strprefix("123456", "123456"));
     assert_true(strprefix("123456", "123"));

@@ -44,11 +44,188 @@ buffer_strprefix(void **state)
     assert_false(strprefix("12", "123"));
 }
 
+#define testsep ","
+#define testnosep ""
+#define teststr1 "one"
+#define teststr2 "two"
+#define teststr3 "three"
+#define teststr4 "four"
+
+struct test_buffer_list_aggregate_ctx {
+    struct buffer_list *empty;
+    struct buffer_list *one_two_three;
+    struct buffer_list *zero_length_strings;
+    struct buffer_list *empty_buffers;
+};
+
+static int test_buffer_list_setup(void **state)
+{
+    struct test_buffer_list_aggregate_ctx *ctx  = calloc(1, sizeof(*ctx));
+    ctx->empty = buffer_list_new(0);
+
+    ctx->one_two_three = buffer_list_new(3);
+    buffer_list_push(ctx->one_two_three, teststr1);
+    buffer_list_push(ctx->one_two_three, teststr2);
+    buffer_list_push(ctx->one_two_three, teststr3);
+
+    ctx->zero_length_strings = buffer_list_new(2);
+    buffer_list_push(ctx->zero_length_strings, "");
+    buffer_list_push(ctx->zero_length_strings, "");
+
+    ctx->empty_buffers = buffer_list_new(2);
+    uint8_t *data1 = malloc(1);
+    uint8_t *data2 = malloc(1);
+    buffer_list_push_data(ctx->empty_buffers, data1, 0);
+    buffer_list_push_data(ctx->empty_buffers, data2, 0);
+
+    *state = ctx;
+    return 0;
+}
+
+static int test_buffer_list_teardown(void **state)
+{
+    struct test_buffer_list_aggregate_ctx *ctx = *state;
+
+    buffer_list_free(ctx->empty);
+    buffer_list_free(ctx->one_two_three);
+    buffer_list_free(ctx->zero_length_strings);
+    buffer_list_free(ctx->empty_buffers);
+    free(ctx);
+    return 0;
+}
+
+static void
+test_buffer_list_full(void **state)
+{
+    struct test_buffer_list_aggregate_ctx *ctx = *state;
+
+    /* list full */
+    assert_int_equal(ctx->one_two_three->size, 3);
+    buffer_list_push(ctx->one_two_three, teststr4);
+    assert_int_equal(ctx->one_two_three->size, 3);
+}
+
+static void
+test_buffer_list_aggregate_separator_empty(void **state)
+{
+    struct test_buffer_list_aggregate_ctx *ctx = *state;
+
+    /* aggregating an empty buffer list results in an empty buffer list */
+    buffer_list_aggregate_separator(ctx->empty, 3, testsep);
+    assert_null(ctx->empty->head);
+}
+
+static void
+test_buffer_list_aggregate_separator_noop(void **state)
+{
+    struct test_buffer_list_aggregate_ctx *ctx = *state;
+
+    /* With a max length of 2, no aggregation should take place */
+    buffer_list_aggregate_separator(ctx->one_two_three, 2, testsep);
+    assert_int_equal(ctx->one_two_three->size, 3);
+    struct buffer *buf = buffer_list_peek(ctx->one_two_three);
+    assert_string_equal((const char *)buf->data, teststr1);
+}
+
+static void
+test_buffer_list_aggregate_separator_two(void **state)
+{
+    struct test_buffer_list_aggregate_ctx *ctx = *state;
+
+    /* Aggregate the first two elements */
+    /* FIXME this exceeds the supplied max */
+    buffer_list_aggregate_separator(ctx->one_two_three, 4, testsep);
+    /* FIXME size does not get adjusted after aggregating */
+    assert_int_equal(ctx->one_two_three->size, 3);
+    struct buffer *buf = buffer_list_peek(ctx->one_two_three);
+    assert_string_equal((const char *)buf->data,
+                        teststr1 testsep teststr2 testsep);
+}
+
+static void
+test_buffer_list_aggregate_separator_all(void **state)
+{
+    struct test_buffer_list_aggregate_ctx *ctx = *state;
+
+    /* Aggregate all */
+    buffer_list_aggregate_separator(ctx->one_two_three, 1<<16, testsep);
+    /* FIXME size does not get adjusted after aggregating */
+    assert_int_equal(ctx->one_two_three->size, 3);
+    struct buffer *buf = buffer_list_peek(ctx->one_two_three);
+    assert_string_equal((const char *)buf->data,
+                        teststr1 testsep teststr2 testsep teststr3 testsep);
+}
+
+static void
+test_buffer_list_aggregate_separator_nosep(void **state)
+{
+    struct test_buffer_list_aggregate_ctx *ctx = *state;
+
+    /* Aggregate all */
+    buffer_list_aggregate_separator(ctx->one_two_three, 1<<16, testnosep);
+    /* FIXME size does not get adjusted after aggregating */
+    assert_int_equal(ctx->one_two_three->size, 3);
+    struct buffer *buf = buffer_list_peek(ctx->one_two_three);
+    assert_string_equal((const char *)buf->data, teststr1 teststr2 teststr3);
+}
+
+static void
+test_buffer_list_aggregate_separator_zerolen(void **state)
+{
+    struct test_buffer_list_aggregate_ctx *ctx = *state;
+    struct buffer_list *bl_zerolen = ctx->zero_length_strings;
+
+    /* Aggregate all */
+    buffer_list_aggregate_separator(bl_zerolen, 1<<16, testnosep);
+    /* FIXME size does not get adjusted after aggregating */
+    assert_int_equal(bl_zerolen->size, 2);
+    struct buffer *buf = buffer_list_peek(bl_zerolen);
+    assert_string_equal((const char *)buf->data, "");
+}
+
+static void
+test_buffer_list_aggregate_separator_emptybuffers(void **state)
+{
+    struct test_buffer_list_aggregate_ctx *ctx = *state;
+    struct buffer_list *bl_emptybuffers = ctx->empty_buffers;
+
+    /* Aggregate all */
+    buffer_list_aggregate_separator(bl_emptybuffers, 1<<16, testnosep);
+    /* FIXME size does not get adjusted after aggregating */
+    assert_int_equal(bl_emptybuffers->size, 2);
+    struct buffer *buf = buffer_list_peek(bl_emptybuffers);
+    assert_int_equal(BLEN(buf), 0);
+}
+
 int
 main(void)
 {
     const struct CMUnitTest tests[] = {
-        cmocka_unit_test(buffer_strprefix),
+        cmocka_unit_test(test_buffer_strprefix),
+        cmocka_unit_test_setup_teardown(test_buffer_list_full,
+                                        test_buffer_list_setup,
+                                        test_buffer_list_teardown),
+        cmocka_unit_test_setup_teardown(test_buffer_list_aggregate_separator_empty,
+                                        test_buffer_list_setup,
+                                        test_buffer_list_teardown),
+        cmocka_unit_test_setup_teardown(test_buffer_list_aggregate_separator_noop,
+                                        test_buffer_list_setup,
+                                        test_buffer_list_teardown),
+        cmocka_unit_test_setup_teardown(test_buffer_list_aggregate_separator_two,
+                                        test_buffer_list_setup,
+                                        test_buffer_list_teardown),
+        cmocka_unit_test_setup_teardown(test_buffer_list_aggregate_separator_all,
+                                        test_buffer_list_setup,
+                                        test_buffer_list_teardown),
+        cmocka_unit_test_setup_teardown(test_buffer_list_aggregate_separator_nosep,
+                                        test_buffer_list_setup,
+                                        test_buffer_list_teardown),
+        cmocka_unit_test_setup_teardown(test_buffer_list_aggregate_separator_zerolen,
+                                        test_buffer_list_setup,
+                                        test_buffer_list_teardown),
+        cmocka_unit_test_setup_teardown(test_buffer_list_aggregate_separator_emptybuffers,
+                                        test_buffer_list_setup,
+                                        test_buffer_list_teardown),
     };
 
     return cmocka_run_group_tests_name("buffer", tests, NULL, NULL);

Signature · 1 line

2.7.4

Steffan Karger

author

2017-07-01 12:53 UTC #1 lore raw

PATCH 2/5 buffer_list_aggregate_separator(): update list size after aggregating

After aggregating a buffer_list, the size should be adjusted accordingly.

Signed-off-by Steffan Karger <[email protected]...>

---
 src/openvpn/buffer.c                   |  1 +
 tests/unit_tests/openvpn/test_buffer.c | 15 +++++----------
 2 files changed, 6 insertions(+), 10 deletions(-)

src/openvpn/buffer.c +1 -0

index 87e27ec..1cc2957 100644
@@ -1265,6 +1265,7 @@ buffer_list_aggregate_separator(struct buffer_list *bl, const size_t max, const
                 e = next;
             }
             bl->head = f;
+            bl->size -= count-1;
             f->next = more;
             if (!more)
             {

tests/unit_tests/openvpn/test_buffer.c +5 ~~-10~~

index acfeb1a..6842114 100644
@@ -135,8 +135,7 @@ test_buffer_list_aggregate_separator_two(void **state)
     /* Aggregate the first two elements */
     /* FIXME this exceeds the supplied max */
     buffer_list_aggregate_separator(ctx->one_two_three, 4, testsep);
-    /* FIXME size does not get adjusted after aggregating */
-    assert_int_equal(ctx->one_two_three->size, 3);
+    assert_int_equal(ctx->one_two_three->size, 2);
     struct buffer *buf = buffer_list_peek(ctx->one_two_three);
     assert_string_equal((const char *)buf->data,
                         teststr1 testsep teststr2 testsep);

@@ -149,8 +148,7 @@ test_buffer_list_aggregate_separator_all(void **state)
 
     /* Aggregate all */
     buffer_list_aggregate_separator(ctx->one_two_three, 1<<16, testsep);
-    /* FIXME size does not get adjusted after aggregating */
-    assert_int_equal(ctx->one_two_three->size, 3);
+    assert_int_equal(ctx->one_two_three->size, 1);
     struct buffer *buf = buffer_list_peek(ctx->one_two_three);
     assert_string_equal((const char *)buf->data,
                         teststr1 testsep teststr2 testsep teststr3 testsep);

@@ -163,8 +161,7 @@ test_buffer_list_aggregate_separator_nosep(void **state)
 
     /* Aggregate all */
     buffer_list_aggregate_separator(ctx->one_two_three, 1<<16, testnosep);
-    /* FIXME size does not get adjusted after aggregating */
-    assert_int_equal(ctx->one_two_three->size, 3);
+    assert_int_equal(ctx->one_two_three->size, 1);
     struct buffer *buf = buffer_list_peek(ctx->one_two_three);
     assert_string_equal((const char *)buf->data, teststr1 teststr2 teststr3);
 }

@@ -177,8 +174,7 @@ test_buffer_list_aggregate_separator_zerolen(void **state)
 
     /* Aggregate all */
     buffer_list_aggregate_separator(bl_zerolen, 1<<16, testnosep);
-    /* FIXME size does not get adjusted after aggregating */
-    assert_int_equal(bl_zerolen->size, 2);
+    assert_int_equal(bl_zerolen->size, 1);
     struct buffer *buf = buffer_list_peek(bl_zerolen);
     assert_string_equal((const char *)buf->data, "");
 }

@@ -191,8 +187,7 @@ test_buffer_list_aggregate_separator_emptybuffers(void **state)
 
     /* Aggregate all */
     buffer_list_aggregate_separator(bl_emptybuffers, 1<<16, testnosep);
-    /* FIXME size does not get adjusted after aggregating */
-    assert_int_equal(bl_emptybuffers->size, 2);
+    assert_int_equal(bl_emptybuffers->size, 1);
     struct buffer *buf = buffer_list_peek(bl_emptybuffers);
     assert_int_equal(BLEN(buf), 0);
 }

Signature · 1 line

2.7.4

Steffan Karger

author

2017-12-11 11:41 UTC #2 lore raw

PATCH v2 2/5 buffer_list_aggregate_separator(): update list size after aggregating

After aggregating a buffer_list, the size should be adjusted accordingly.

Signed-off-by Steffan Karger <[email protected]...>

---
v2: rebase on master

 src/openvpn/buffer.c                   |  1 +
 tests/unit_tests/openvpn/test_buffer.c | 15 +++++----------
 2 files changed, 6 insertions(+), 10 deletions(-)

src/openvpn/buffer.c +1 -0

index ecd3b17..28ac508 100644
@@ -1257,6 +1257,7 @@ buffer_list_aggregate_separator(struct buffer_list *bl, const size_t max, const
                 e = next;
             }
             bl->head = f;
+            bl->size -= count-1;
             f->next = more;
             if (!more)
             {

tests/unit_tests/openvpn/test_buffer.c +5 ~~-10~~

index ba5aa67..f5830a1 100644
@@ -139,8 +139,7 @@ test_buffer_list_aggregate_separator_two(void **state)
     /* Aggregate the first two elements */
     /* FIXME this exceeds the supplied max */
     buffer_list_aggregate_separator(ctx->one_two_three, 4, testsep);
-    /* FIXME size does not get adjusted after aggregating */
-    assert_int_equal(ctx->one_two_three->size, 3);
+    assert_int_equal(ctx->one_two_three->size, 2);
     struct buffer *buf = buffer_list_peek(ctx->one_two_three);
     assert_buf_equals_str(buf, teststr1 testsep teststr2 testsep);
 }

@@ -152,8 +151,7 @@ test_buffer_list_aggregate_separator_all(void **state)
 
     /* Aggregate all */
     buffer_list_aggregate_separator(ctx->one_two_three, 1<<16, testsep);
-    /* FIXME size does not get adjusted after aggregating */
-    assert_int_equal(ctx->one_two_three->size, 3);
+    assert_int_equal(ctx->one_two_three->size, 1);
     struct buffer *buf = buffer_list_peek(ctx->one_two_three);
     assert_buf_equals_str(buf,
                           teststr1 testsep teststr2 testsep teststr3 testsep);

@@ -166,8 +164,7 @@ test_buffer_list_aggregate_separator_nosep(void **state)
 
     /* Aggregate all */
     buffer_list_aggregate_separator(ctx->one_two_three, 1<<16, testnosep);
-    /* FIXME size does not get adjusted after aggregating */
-    assert_int_equal(ctx->one_two_three->size, 3);
+    assert_int_equal(ctx->one_two_three->size, 1);
     struct buffer *buf = buffer_list_peek(ctx->one_two_three);
     assert_buf_equals_str(buf, teststr1 teststr2 teststr3);
 }

@@ -180,8 +177,7 @@ test_buffer_list_aggregate_separator_zerolen(void **state)
 
     /* Aggregate all */
     buffer_list_aggregate_separator(bl_zerolen, 1<<16, testnosep);
-    /* FIXME size does not get adjusted after aggregating */
-    assert_int_equal(bl_zerolen->size, 2);
+    assert_int_equal(bl_zerolen->size, 1);
     struct buffer *buf = buffer_list_peek(bl_zerolen);
     assert_buf_equals_str(buf, "");
 }

@@ -194,8 +190,7 @@ test_buffer_list_aggregate_separator_emptybuffers(void **state)
 
     /* Aggregate all */
     buffer_list_aggregate_separator(bl_emptybuffers, 1<<16, testnosep);
-    /* FIXME size does not get adjusted after aggregating */
-    assert_int_equal(bl_emptybuffers->size, 2);
+    assert_int_equal(bl_emptybuffers->size, 1);
     struct buffer *buf = buffer_list_peek(bl_emptybuffers);
     assert_int_equal(BLEN(buf), 0);
 }

Signature · 1 line

2.7.4

Antonio Quartulli

Acked-by on v2 2/5

2017-12-28 22:50 UTC #3 lore raw

Re: [Openvpn-devel] [PATCH 2/5 v2] buffer_list_aggregate_separator(): update list size after aggregating

Hi,

the patch looks good and finally gets rid of some FIXMEs in the buffer unit-test.

However, the field fixed by this patch is not really used anywhere, thus no real logic flaw is being corrected (Steffan, correct me if I am wrong).

For this reason, I think that backporting this patch to older releases is entirely up to the maintainers.

Quoted 3 lines · Steffan Karger

After aggregating a buffer_list, the size should be adjusted accordingly.
Signed-off-by Steffan Karger <[email protected]...>

Acked-by Antonio Quartulli <[email protected]...>

except for a little style issue remarked below:

Quoted 12 lines

---
v2: rebase on master

 src/openvpn/buffer.c                   |  1 +
 tests/unit_tests/openvpn/test_buffer.c | 15 +++++----------
 2 files changed, 6 insertions(+), 10 deletions(-)

src/openvpn/buffer.c +0 -0

index ecd3b17..28ac508 100644

src/openvpn/buffer.c buffer_list_aggregate_separator(struct buffer_list *bl, const size_t max, const · line 1260

Quoted 4 lines

src/openvpn/buffer.c +1 -0

@@ -1257,6 +1257,7 @@ buffer_list_aggregate_separator(struct buffer_list *bl, const size_t max, const
                 e = next;
             }
             bl->head = f;
+            bl->size -= count-1;

the '-' sign should be surrounded by spaces.

Cheers,

Signature · 1 line

Antonio Quartulli

Steffan Karger

author

2017-12-29 09:52 UTC #4 lore raw

PATCH v3 2/5 buffer_list_aggregate_separator(): update list size after aggregating

After aggregating a buffer_list, the size should be adjusted accordingly.

Signed-off-by Steffan Karger <[email protected]...>

---
v2: rebase on master
v3: add spaces around '-'

 src/openvpn/buffer.c                   |  1 +
 tests/unit_tests/openvpn/test_buffer.c | 15 +++++----------
 2 files changed, 6 insertions(+), 10 deletions(-)

src/openvpn/buffer.c +1 -0

index e601a31..28ce8ca 100644
@@ -1257,6 +1257,7 @@ buffer_list_aggregate_separator(struct buffer_list *bl, const size_t max, const
                 e = next;
             }
             bl->head = f;
+            bl->size -= count - 1;
             f->next = more;
             if (!more)
             {

tests/unit_tests/openvpn/test_buffer.c +5 ~~-10~~

index b4625bb..bddb30a 100644
@@ -138,8 +138,7 @@ test_buffer_list_aggregate_separator_two(void **state)
     /* Aggregate the first two elements */
     /* FIXME this exceeds the supplied max */
     buffer_list_aggregate_separator(ctx->one_two_three, 4, testsep);
-    /* FIXME size does not get adjusted after aggregating */
-    assert_int_equal(ctx->one_two_three->size, 3);
+    assert_int_equal(ctx->one_two_three->size, 2);
     struct buffer *buf = buffer_list_peek(ctx->one_two_three);
     assert_buf_equals_str(buf, teststr1 testsep teststr2 testsep);
 }

@@ -151,8 +150,7 @@ test_buffer_list_aggregate_separator_all(void **state)
 
     /* Aggregate all */
     buffer_list_aggregate_separator(ctx->one_two_three, 1<<16, testsep);
-    /* FIXME size does not get adjusted after aggregating */
-    assert_int_equal(ctx->one_two_three->size, 3);
+    assert_int_equal(ctx->one_two_three->size, 1);
     struct buffer *buf = buffer_list_peek(ctx->one_two_three);
     assert_buf_equals_str(buf,
                           teststr1 testsep teststr2 testsep teststr3 testsep);

@@ -165,8 +163,7 @@ test_buffer_list_aggregate_separator_nosep(void **state)
 
     /* Aggregate all */
     buffer_list_aggregate_separator(ctx->one_two_three, 1<<16, testnosep);
-    /* FIXME size does not get adjusted after aggregating */
-    assert_int_equal(ctx->one_two_three->size, 3);
+    assert_int_equal(ctx->one_two_three->size, 1);
     struct buffer *buf = buffer_list_peek(ctx->one_two_three);
     assert_buf_equals_str(buf, teststr1 teststr2 teststr3);
 }

@@ -179,8 +176,7 @@ test_buffer_list_aggregate_separator_zerolen(void **state)
 
     /* Aggregate all */
     buffer_list_aggregate_separator(bl_zerolen, 1<<16, testnosep);
-    /* FIXME size does not get adjusted after aggregating */
-    assert_int_equal(bl_zerolen->size, 2);
+    assert_int_equal(bl_zerolen->size, 1);
     struct buffer *buf = buffer_list_peek(bl_zerolen);
     assert_buf_equals_str(buf, "");
 }

@@ -193,8 +189,7 @@ test_buffer_list_aggregate_separator_emptybuffers(void **state)
 
     /* Aggregate all */
     buffer_list_aggregate_separator(bl_emptybuffers, 1<<16, testnosep);
-    /* FIXME size does not get adjusted after aggregating */
-    assert_int_equal(bl_emptybuffers->size, 2);
+    assert_int_equal(bl_emptybuffers->size, 1);
     struct buffer *buf = buffer_list_peek(bl_emptybuffers);
     assert_int_equal(BLEN(buf), 0);
 }

Signature · 1 line

2.7.4

Antonio Quartulli

Acked-by on v3 2/5

2017-12-29 09:56 UTC #5 lore raw

Re: [Openvpn-devel] [PATCH 2/5 v3] buffer_list_aggregate_separator(): update list size after aggregating

Hi,

Quoted 3 lines · Steffan Karger

After aggregating a buffer_list, the size should be adjusted accordingly.
Signed-off-by Steffan Karger <[email protected]...>

Acked-by Antonio Quartulli <[email protected]...>

Signature · 1 line

Antonio Quartulli

Steffan Karger

author

2017-07-01 12:54 UTC #7 lore raw

PATCH 3/5 buffer_list_aggregate_separator(): don't exceed max_len

In reply to Steffan Karger

buffer_list_aggregate_separator() would merge buffer_list entries until it had exceeded the provided max_len, instead of stopping *before* exceeding the max value.

Signed-off-by Steffan Karger <[email protected]...>

---
 src/openvpn/buffer.c                   | 17 +++++++++++++----
 src/openvpn/buffer.h                   |  2 +-
 tests/unit_tests/openvpn/test_buffer.c | 12 +++++++-----
 3 files changed, 21 insertions(+), 10 deletions(-)

src/openvpn/buffer.c +13 -4

index 1cc2957..2700ad1 100644
@@ -1231,7 +1231,8 @@ buffer_list_peek(struct buffer_list *ol)
 }
 
 void
-buffer_list_aggregate_separator(struct buffer_list *bl, const size_t max, const char *sep)
+buffer_list_aggregate_separator(struct buffer_list *bl, const size_t max_len,
+                                const char *sep)
 {
     int sep_len = strlen(sep);

@@ -1240,10 +1241,18 @@ buffer_list_aggregate_separator(struct buffer_list *bl, const size_t max, const
         struct buffer_entry *more = bl->head;
         size_t size = 0;
         int count = 0;
-        for (count = 0; more && size <= max; ++count)
+        for (count = 0; more; ++count)
         {
-            size += BLEN(&more->buf) + sep_len;
-            more = more->next;
+            size_t extra_len = BLEN(&more->buf) + sep_len;
+            if (size + extra_len <= max_len)
+            {
+                size += extra_len;
+                more = more->next;
+            }
+            else
+            {
+                break;
+            }
         }
 
         if (count >= 2)

src/openvpn/buffer.h +1 -1

index 8bc4428..019e606 100644
@@ -1102,7 +1102,7 @@ void buffer_list_pop(struct buffer_list *ol);
 
 void buffer_list_aggregate(struct buffer_list *bl, const size_t max);
 
-void buffer_list_aggregate_separator(struct buffer_list *bl, const size_t max, const char *sep);
+void buffer_list_aggregate_separator(struct buffer_list *bl, const size_t max_len, const char *sep);
 
 struct buffer_list *buffer_list_file(const char *fn, int max_line_len);

tests/unit_tests/openvpn/test_buffer.c +7 -5

index 6842114..8b2575f 100644
@@ -131,14 +131,16 @@ static void
 test_buffer_list_aggregate_separator_two(void **state)
 {
     struct test_buffer_list_aggregate_ctx *ctx = *state;
+    const char *expected = teststr1 testsep teststr2 testsep;
 
-    /* Aggregate the first two elements */
-    /* FIXME this exceeds the supplied max */
-    buffer_list_aggregate_separator(ctx->one_two_three, 4, testsep);
+    /* Aggregate the first two elements
+     * (add 1 to max_len to test if "three" is not sneaked in too)
+     */
+    buffer_list_aggregate_separator(ctx->one_two_three, strlen(expected)+1,
+                                    testsep);
     assert_int_equal(ctx->one_two_three->size, 2);
     struct buffer *buf = buffer_list_peek(ctx->one_two_three);
-    assert_string_equal((const char *)buf->data,
-                        teststr1 testsep teststr2 testsep);
+    assert_string_equal((const char *)buf->data, expected);
 }
 
 static void

Signature · 1 line

2.7.4

Steffan Karger

author

2017-11-09 14:02 UTC #8 lore raw

PATCH v2 3/5 buffer_list_aggregate_separator(): don't exceed max_len

buffer_list_aggregate_separator() would merge buffer_list entries until it had exceeded the provided max_len, instead of stopping *before* exceeding the max value.

Signed-off-by Steffan Karger <[email protected]...>

---
v2: rebase on 1/5 v2 (other patches should still apply)

 src/openvpn/buffer.c                   | 17 +++++++++++++----
 src/openvpn/buffer.h                   |  2 +-
 tests/unit_tests/openvpn/test_buffer.c | 11 +++++++----
 3 files changed, 21 insertions(+), 9 deletions(-)

src/openvpn/buffer.c +13 -4

index 583c5c7..f72d525 100644
@@ -1231,7 +1231,8 @@ buffer_list_peek(struct buffer_list *ol)
 }
 
 void
-buffer_list_aggregate_separator(struct buffer_list *bl, const size_t max, const char *sep)
+buffer_list_aggregate_separator(struct buffer_list *bl, const size_t max_len,
+                                const char *sep)
 {
     int sep_len = strlen(sep);

@@ -1240,10 +1241,18 @@ buffer_list_aggregate_separator(struct buffer_list *bl, const size_t max, const
         struct buffer_entry *more = bl->head;
         size_t size = 0;
         int count = 0;
-        for (count = 0; more && size <= max; ++count)
+        for (count = 0; more; ++count)
         {
-            size += BLEN(&more->buf) + sep_len;
-            more = more->next;
+            size_t extra_len = BLEN(&more->buf) + sep_len;
+            if (size + extra_len <= max_len)
+            {
+                size += extra_len;
+                more = more->next;
+            }
+            else
+            {
+                break;
+            }
         }
 
         if (count >= 2)

src/openvpn/buffer.h +1 -1

index 1ed5631..499c75e 100644
@@ -1102,7 +1102,7 @@ void buffer_list_pop(struct buffer_list *ol);
 
 void buffer_list_aggregate(struct buffer_list *bl, const size_t max);
 
-void buffer_list_aggregate_separator(struct buffer_list *bl, const size_t max, const char *sep);
+void buffer_list_aggregate_separator(struct buffer_list *bl, const size_t max_len, const char *sep);
 
 struct buffer_list *buffer_list_file(const char *fn, int max_line_len);

tests/unit_tests/openvpn/test_buffer.c +7 -4

index f5830a1..99e6530 100644
@@ -135,13 +135,16 @@ static void
 test_buffer_list_aggregate_separator_two(void **state)
 {
     struct test_buffer_list_aggregate_ctx *ctx = *state;
+    const char *expected = teststr1 testsep teststr2 testsep;
 
-    /* Aggregate the first two elements */
-    /* FIXME this exceeds the supplied max */
-    buffer_list_aggregate_separator(ctx->one_two_three, 4, testsep);
+    /* Aggregate the first two elements
+     * (add 1 to max_len to test if "three" is not sneaked in too)
+     */
+    buffer_list_aggregate_separator(ctx->one_two_three, strlen(expected)+1,
+                                    testsep);
     assert_int_equal(ctx->one_two_three->size, 2);
     struct buffer *buf = buffer_list_peek(ctx->one_two_three);
-    assert_buf_equals_str(buf, teststr1 testsep teststr2 testsep);
+    assert_buf_equals_str(buf, expected);
 }
 
 static void

Signature · 1 line

2.7.4

Antonio Quartulli

Acked-by on v2 3/5

2017-12-28 22:51 UTC #9 lore raw

Re: [Openvpn-devel] [PATCH 3/5 v2] buffer_list_aggregate_separator(): don't exceed max_len

Hi,

Quoted 36 lines · Steffan Karger

buffer_list_aggregate_separator() would merge buffer_list entries until it had exceeded the provided max_len, instead of stopping *before* exceeding the max value.

Signed-off-by Steffan Karger <[email protected]...>

---
v2: rebase on 1/5 v2 (other patches should still apply)

 src/openvpn/buffer.c                   | 17 +++++++++++++----
 src/openvpn/buffer.h                   |  2 +-
 tests/unit_tests/openvpn/test_buffer.c | 11 +++++++----
 3 files changed, 21 insertions(+), 9 deletions(-)

src/openvpn/buffer.c +3 -4

index 583c5c7..f72d525 100644
@@ -1231,7 +1231,8 @@ buffer_list_peek(struct buffer_list *ol)
 }
 
 void
-buffer_list_aggregate_separator(struct buffer_list *bl, const size_t max, const char *sep)
+buffer_list_aggregate_separator(struct buffer_list *bl, const size_t max_len,
+                                const char *sep)
 {
     int sep_len = strlen(sep);

@@ -1240,10 +1241,18 @@ buffer_list_aggregate_separator(struct buffer_list *bl, const size_t max, const
         struct buffer_entry *more = bl->head;
         size_t size = 0;
         int count = 0;
-        for (count = 0; more && size <= max; ++count)
+        for (count = 0; more; ++count)
         {
-            size += BLEN(&more->buf) + sep_len;
-            more = more->next;

src/openvpn/buffer.c buffer_list_aggregate_separator(struct buffer_list *bl, const size_t max, const · line 1255

Quoted 10 lines

src/openvpn/buffer.c +10 -0

+            size_t extra_len = BLEN(&more->buf) + sep_len;
+            if (size + extra_len <= max_len)
+            {
+                size += extra_len;
+                more = more->next;
+            }
+            else
+            {
+                break;
+            }

The code looks correct as it simply changes the looping condition and moves it inside the loop to stay below the 80chars.

However, my taste would suggest to change the inner if-else block as follows:

if (condition)

{
    break;
}

rest;

simply to avoid non-needed indentation and follow the usual "this is bad, we can't continue, thus go away" logic.

But again, this is just my taste. Other maintainers can decide what is better.

src/openvpn/buffer.h void buffer_list_pop(struct buffer_list *ol); · line 1105

Quoted 13 lines

src/openvpn/buffer.c +0 -0

         }
 
         if (count >= 2)

src/openvpn/buffer.h +1 -1

index 1ed5631..499c75e 100644
@@ -1102,7 +1102,7 @@ void buffer_list_pop(struct buffer_list *ol);
 
 void buffer_list_aggregate(struct buffer_list *bl, const size_t max);
 
-void buffer_list_aggregate_separator(struct buffer_list *bl, const size_t max, const char *sep);
+void buffer_list_aggregate_separator(struct buffer_list *bl, const size_t max_len, const char *sep);

why not going on a new line now that you are touching it? (I know it exceeded 80 chars even before your change)

Quoted 11 lines

src/openvpn/buffer.h +0 -0

 struct buffer_list *buffer_list_file(const char *fn, int max_line_len);

tests/unit_tests/openvpn/test_buffer.c +0 -0

index f5830a1..99e6530 100644
@@ -135,13 +135,16 @@ static void
 test_buffer_list_aggregate_separator_two(void **state)
 {
     struct test_buffer_list_aggregate_ctx *ctx = *state;

tests/unit_tests/openvpn/test_buffer.c static void · line 144

Quoted 10 lines

tests/unit_tests/openvpn/test_buffer.c +6 -3

+    const char *expected = teststr1 testsep teststr2 testsep;
 
-    /* Aggregate the first two elements */
-    /* FIXME this exceeds the supplied max */
-    buffer_list_aggregate_separator(ctx->one_two_three, 4, testsep);
+    /* Aggregate the first two elements
+     * (add 1 to max_len to test if "three" is not sneaked in too)
+     */
+    buffer_list_aggregate_separator(ctx->one_two_three, strlen(expected)+1,
+                                    testsep);

Like in the previous patch, the '+' should be surrounded by spaces.

tests/unit_tests/openvpn/test_buffer.c static void · line 150

Quoted 8 lines

tests/unit_tests/openvpn/test_buffer.c +1 -1

     assert_int_equal(ctx->one_two_three->size, 2);
     struct buffer *buf = buffer_list_peek(ctx->one_two_three);
-    assert_buf_equals_str(buf, teststr1 testsep teststr2 testsep);
+    assert_buf_equals_str(buf, expected);
 }
 
 static void

Another FIXME thrown away :-)

For the code correctness:

Acked-by Antonio Quartulli <[email protected]...>

Cheers,

Signature · 1 line

Antonio Quartulli

Steffan Karger

author

2017-12-29 09:53 UTC #10 lore raw

PATCH v3 3/5 buffer_list_aggregate_separator(): don't exceed max_len

buffer_list_aggregate_separator() would merge buffer_list entries until it had exceeded the provided max_len, instead of stopping *before* exceeding the max value.

Signed-off-by Steffan Karger <[email protected]...>

---
v2: rebase on 1/5 v2 (other patches should still apply)
v3: as suggested by ordex, reorder if contitions and add spaces around '+'

 src/openvpn/buffer.c                   | 13 ++++++++++---
 src/openvpn/buffer.h                   |  3 ++-
 tests/unit_tests/openvpn/test_buffer.c | 11 +++++++----
 3 files changed, 19 insertions(+), 8 deletions(-)

src/openvpn/buffer.c +10 -3

index 28ce8ca..2656702 100644
@@ -1223,7 +1223,8 @@ buffer_list_peek(struct buffer_list *ol)
 }
 
 void
-buffer_list_aggregate_separator(struct buffer_list *bl, const size_t max, const char *sep)
+buffer_list_aggregate_separator(struct buffer_list *bl, const size_t max_len,
+                                const char *sep)
 {
     int sep_len = strlen(sep);

@@ -1232,9 +1233,15 @@ buffer_list_aggregate_separator(struct buffer_list *bl, const size_t max, const
         struct buffer_entry *more = bl->head;
         size_t size = 0;
         int count = 0;
-        for (count = 0; more && size <= max; ++count)
+        for (count = 0; more; ++count)
         {
-            size += BLEN(&more->buf) + sep_len;
+            size_t extra_len = BLEN(&more->buf) + sep_len;
+            if (size + extra_len > max_len)
+            {
+                break;
+            }
+
+            size += extra_len;
             more = more->next;
         }

src/openvpn/buffer.h +2 -1

index b73f6c8..e588b80 100644
@@ -1107,7 +1107,8 @@ void buffer_list_pop(struct buffer_list *ol);
 
 void buffer_list_aggregate(struct buffer_list *bl, const size_t max);
 
-void buffer_list_aggregate_separator(struct buffer_list *bl, const size_t max, const char *sep);
+void buffer_list_aggregate_separator(struct buffer_list *bl,
+                                     const size_t max_len, const char *sep);
 
 struct buffer_list *buffer_list_file(const char *fn, int max_line_len);

tests/unit_tests/openvpn/test_buffer.c +7 -4

index bddb30a..5af9332 100644
@@ -134,13 +134,16 @@ static void
 test_buffer_list_aggregate_separator_two(void **state)
 {
     struct test_buffer_list_aggregate_ctx *ctx = *state;
+    const char *expected = teststr1 testsep teststr2 testsep;
 
-    /* Aggregate the first two elements */
-    /* FIXME this exceeds the supplied max */
-    buffer_list_aggregate_separator(ctx->one_two_three, 4, testsep);
+    /* Aggregate the first two elements
+     * (add 1 to max_len to test if "three" is not sneaked in too)
+     */
+    buffer_list_aggregate_separator(ctx->one_two_three, strlen(expected) + 1,
+                                    testsep);
     assert_int_equal(ctx->one_two_three->size, 2);
     struct buffer *buf = buffer_list_peek(ctx->one_two_three);
-    assert_buf_equals_str(buf, teststr1 testsep teststr2 testsep);
+    assert_buf_equals_str(buf, expected);
 }
 
 static void

Signature · 1 line

2.7.4

Antonio Quartulli

Acked-by on v3 3/5

2017-12-29 09:58 UTC #11 lore raw

Re: [Openvpn-devel] [PATCH 3/5 v3] buffer_list_aggregate_separator(): don't exceed max_len

Hi,

Quoted 8 lines · Steffan Karger

buffer_list_aggregate_separator() would merge buffer_list entries until it had exceeded the provided max_len, instead of stopping *before* exceeding the max value.
Signed-off-by Steffan Karger <[email protected]...>
---
v2: rebase on 1/5 v2 (other patches should still apply)
v3: as suggested by ordex, reorder if contitions and add spaces around '+'

Thanks for that!

Acked-by Antonio Quartulli <[email protected]...>

Signature · 1 line

Antonio Quartulli

Steffan Karger

author

2017-07-01 12:54 UTC #13 lore raw

PATCH 4/5 buffer_list_aggregate_separator(): prevent 0-byte malloc

In reply to Steffan Karger

As pointed out in finding OVPN-05 of the cryptograpy engineering audit (funded by Private Internet Access), buffer_list_aggregate_separator() could perform a 0-byte malloc when called with a list of 0-length buffers and a "" separator. If other could would later try to access that buffer memory, this would result in undefined behaviour. To prevent this, always malloc() 1 byte.

To simplify as we go, use alloc_buf() to allocate the buffer. This has the additional benefit that the actual buffer data (not the contents) is zero-terminated, because alloc_buf() calls calloc() and we have 1 extra byte of data.

Signed-off-by Steffan Karger <[email protected]...>

---
 src/openvpn/buffer.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

src/openvpn/buffer.c +1 -2

index 2700ad1..ecbdc1b 100644
@@ -1261,8 +1261,7 @@ buffer_list_aggregate_separator(struct buffer_list *bl, const size_t max_len,
             struct buffer_entry *e = bl->head, *f;
 
             ALLOC_OBJ_CLEAR(f, struct buffer_entry);
-            f->buf.data = malloc(size);
-            check_malloc_return(f->buf.data);
+            f->buf = alloc_buf(size+1); /* prevent 0-byte malloc */
             f->buf.capacity = size;
             for (i = 0; e && i < count; ++i)
             {

Signature · 1 line

2.7.4

Antonio Quartulli

2017-12-28 22:51 UTC #14 lore raw

Re: [Openvpn-devel] [PATCH 4/5] buffer_list_aggregate_separator(): prevent 0-byte malloc

Hi,

Quoted 7 lines · Steffan Karger

As pointed out in finding OVPN-05 of the cryptograpy engineering audit (funded by Private Internet Access), buffer_list_aggregate_separator() could perform a 0-byte malloc when called with a list of 0-length buffers and a "" separator. If other could would later try to access that buffer memory, this would result in undefined behaviour. To prevent this, always malloc() 1 byte.

Quoted 6 lines

To simplify as we go, use alloc_buf() to allocate the buffer. This has the additional benefit that the actual buffer data (not the contents) is zero-terminated, because alloc_buf() calls calloc() and we have 1 extra byte of data.
Signed-off-by Steffan Karger <[email protected]...>

Does it really make sense to continue the execution if we receive a list of empty buffers? Shouldn't we report this as an error?

Quoted 9 lines

---
 src/openvpn/buffer.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

src/openvpn/buffer.c +0 -0

index 2700ad1..ecbdc1b 100644

src/openvpn/buffer.c buffer_list_aggregate_separator(struct buffer_list *bl, const size_t max_len, · line 1264

Quoted 6 lines

src/openvpn/buffer.c +1 -2

@@ -1261,8 +1261,7 @@ buffer_list_aggregate_separator(struct buffer_list *bl, const size_t max_len,
             struct buffer_entry *e = bl->head, *f;
 
             ALLOC_OBJ_CLEAR(f, struct buffer_entry);
-            f->buf.data = malloc(size);
-            check_malloc_return(f->buf.data);
+            f->buf = alloc_buf(size+1); /* prevent 0-byte malloc */

the '+' sign should be surrounded by spaces.

Cheers,

Quoted 4 lines

src/openvpn/buffer.c +0 -0

             f->buf.capacity = size;
             for (i = 0; e && i < count; ++i)
             {

Signature · 1 line

Antonio Quartulli

Steffan Karger

author

2017-12-29 09:26 UTC #15 lore raw

Re: [Openvpn-devel] [PATCH 4/5] buffer_list_aggregate_separator(): prevent 0-byte malloc

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

Hi,

Quoted 14 lines · Antonio Quartulli

Quoted 13 lines · Steffan Karger
As pointed out in finding OVPN-05 of the cryptograpy engineering audit (funded by Private Internet Access), buffer_list_aggregate_separator() could perform a 0-byte malloc when called with a list of 0-length buffers and a "" separator. If other could would later try to access that buffer memory, this would result in undefined behaviour. To prevent this, always malloc() 1 byte.
To simplify as we go, use alloc_buf() to allocate the buffer. This has the additional benefit that the actual buffer data (not the contents) is zero-terminated, because alloc_buf() calls calloc() and we have 1 extra byte of data.

Quoted 5 lines

Quoted 1 line
Signed-off-by Steffan Karger <[email protected]...>
Does it really make sense to continue the execution if we receive a list of empty buffers? Shouldn't we report this as an error?

Data is pushed to buffer lists from e.g. tls_send_payload(), where I'm not sure that 'sending' 0-byte payload is an invalid operation. So instead of trying to figure out if that's possible, I prefer to just make this function deal with it.

(Consider e.g. 0/n record splitting, which might want to 'send' 0-length data.)

I'll send follow-up patches to take care of your other remarks.

- -Steffan
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2

iQEcBAEBCAAGBQJaRgo0AAoJEEEwndWOY1w5ajsH/AlPjZL21m1J+hFuhau30D6/ RkXQiR6bKSPhG1zW9IZ6voTz+/XTJK09t16ob4z0HiltRpZ35QXJ9QYKbMmgoOEe IwOG2sDr1FZ8TZYlVOqYqZ9V2WISXsjuubhz6WCTEhkq0pXtBuMSEDqFZWdqjuVa H7eBBd6Uw7VvyU4pPusQpPt5w1T39pQZkdLXj7XHSdOFJHYFKzuNwtGPCUrg67al hObWWKGrbxczgMEZtLt/gqt5TWKsyBB4Uf+G1Qayb8TJuccyvYXUajfmjXufVHjd e5U50FSp1QFS21aCU5ctjKPY/XjAxrx4BnUJUAg++aJw1a7+Z7utvPIDxin2TMU= =7r2E
-----END PGP SIGNATURE-----

Antonio Quartulli

2017-12-29 09:27 UTC #16 lore raw

Re: [Openvpn-devel] [PATCH 4/5] buffer_list_aggregate_separator(): prevent 0-byte malloc

Hi,

Quoted 20 lines · Steffan Karger

Hi,
Quoted 15 lines · Antonio Quartulli
Quoted 14 lines · Steffan Karger
As pointed out in finding OVPN-05 of the cryptograpy engineering audit (funded by Private Internet Access), buffer_list_aggregate_separator() could perform a 0-byte malloc when called with a list of 0-length buffers and a "" separator. If other could would later try to access that buffer memory, this would result in undefined behaviour. To prevent this, always malloc() 1 byte.
To simplify as we go, use alloc_buf() to allocate the buffer. This has the additional benefit that the actual buffer data (not the contents) is zero-terminated, because alloc_buf() calls calloc() and we have 1 extra byte of data.
Signed-off-by Steffan Karger <[email protected]...>

Quoted 7 lines

Quoted 2 lines
Does it really make sense to continue the execution if we receive a list of empty buffers? Shouldn't we report this as an error?
Data is pushed to buffer lists from e.g. tls_send_payload(), where I'm not sure that 'sending' 0-byte payload is an invalid operation. So instead of trying to figure out if that's possible, I prefer to just make this function deal with it.

Yeah, makes sense: better let the external layer deal with the logic.

Quoted 6 lines

(Consider e.g. 0/n record splitting, which might want to 'send' 0-length data.)
I'll send follow-up patches to take care of your other remarks.

Thanks!

Cheers,

Signature · 1 line

Antonio Quartulli

Steffan Karger

author

2017-12-29 09:54 UTC #17 lore raw

PATCH v2 4/5 buffer_list_aggregate_separator(): prevent 0-byte malloc

In reply to Antonio Quartulli

Signed-off-by Steffan Karger <[email protected]...>

---
v2: add spaces around '+'

 src/openvpn/buffer.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

src/openvpn/buffer.c +1 -2

index 2656702..cfe6f2c 100644
@@ -1251,8 +1251,7 @@ buffer_list_aggregate_separator(struct buffer_list *bl, const size_t max_len,
             struct buffer_entry *e = bl->head, *f;
 
             ALLOC_OBJ_CLEAR(f, struct buffer_entry);
-            f->buf.data = malloc(size);
-            check_malloc_return(f->buf.data);
+            f->buf = alloc_buf(size + 1); /* prevent 0-byte malloc */
             f->buf.capacity = size;
             for (i = 0; e && i < count; ++i)
             {

Signature · 1 line

2.7.4

Antonio Quartulli

Acked-by on v2 4/5

2017-12-29 10:01 UTC #18 lore raw

Re: [Openvpn-devel] [PATCH 4/5 v2] buffer_list_aggregate_separator(): prevent 0-byte malloc

Hi,

Quoted 7 lines · Steffan Karger

As pointed out in finding OVPN-05 of the cryptograpy engineering audit (funded by Private Internet Access), buffer_list_aggregate_separator() could perform a 0-byte malloc when called with a list of 0-length buffers and a "" separator. If other could would later try to access that buffer memory, this would result in undefined behaviour. To prevent this, always malloc() 1 byte.

Quoted 6 lines

To simplify as we go, use alloc_buf() to allocate the buffer. This has the additional benefit that the actual buffer data (not the contents) is zero-terminated, because alloc_buf() calls calloc() and we have 1 extra byte of data.
Signed-off-by Steffan Karger <[email protected]...>

Acked-by Antonio Quartulli <[email protected]...>

Signature · 1 line

Antonio Quartulli

Gert Doering

2018-01-11 11:38 UTC #19 lore raw

[Openvpn-devel] [PATCH applied] Re: buffer_list_aggregate_separator(): prevent 0-byte malloc

In reply to Steffan Karger

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

commit 748902f46260fe11cb25726d2bf93bb06ad338f2 (master) commit ef4b827d68d8fef75b04f23e6322aa9d2d4ad8a8 (release/2.4)

Author: Steffan Karger
Date:   Fri Dec 29 10:54:00 2017 +0100

     buffer_list_aggregate_separator(): prevent 0-byte malloc

     Signed-off-by: Steffan Karger <[email protected]...>
     Acked-by: Antonio Quartulli <[email protected]...>
     Message-Id: <[email protected]...>
     URL: https://www.mail-archive.com/[email protected]/msg16106.html
     Signed-off-by: Gert Doering <[email protected]...>

--
kind regards,

Gert Doering

Steffan Karger

author

2017-07-01 12:54 UTC #20 lore raw

PATCH 5/5 buffer_list_aggregate_separator(): simplify code

In reply to Steffan Karger

Clean up the function by slightly simplifying the logic.

Mostly witespace changes, so best reviewed using 'git diff -w'.

Signed-off-by Steffan Karger <[email protected]...>

---
 src/openvpn/buffer.c | 75 ++++++++++++++++++++++++----------------------------
 1 file changed, 35 insertions(+), 40 deletions(-)

src/openvpn/buffer.c +35 ~~-40~~

index ecbdc1b..abb2342 100644
@@ -1234,51 +1234,46 @@ void
 buffer_list_aggregate_separator(struct buffer_list *bl, const size_t max_len,
                                 const char *sep)
 {
-    int sep_len = strlen(sep);
+    const int sep_len = strlen(sep);
+    struct buffer_entry *more = bl->head;
+    size_t size = 0;
+    int count = 0;
+    for (count = 0; more; ++count)
+    {
+        size_t extra_len = BLEN(&more->buf) + sep_len;
+        if (size + extra_len <= max_len)
+        {
+            size += extra_len;
+            more = more->next;
+        }
+        else
+        {
+            break;
+        }
+    }
 
-    if (bl->head)
+    if (count >= 2)
     {
-        struct buffer_entry *more = bl->head;
-        size_t size = 0;
-        int count = 0;
-        for (count = 0; more; ++count)
+        struct buffer_entry *f;
+        ALLOC_OBJ_CLEAR(f, struct buffer_entry);
+        f->buf = alloc_buf(size+1); /* prevent 0-byte malloc */
+
+        struct buffer_entry *e = bl->head;
+        for (size_t i = 0; e && i < count; ++i)
         {
-            size_t extra_len = BLEN(&more->buf) + sep_len;
-            if (size + extra_len <= max_len)
-            {
-                size += extra_len;
-                more = more->next;
-            }
-            else
-            {
-                break;
-            }
+            struct buffer_entry *next = e->next;
+            buf_copy(&f->buf, &e->buf);
+            buf_write(&f->buf, sep, sep_len);
+            free_buf(&e->buf);
+            free(e);
+            e = next;
         }
-
-        if (count >= 2)
+        bl->head = f;
+        bl->size -= count-1;
+        f->next = more;
+        if (!more)
         {
-            int i;
-            struct buffer_entry *e = bl->head, *f;
-
-            ALLOC_OBJ_CLEAR(f, struct buffer_entry);
-            f->buf = alloc_buf(size+1); /* prevent 0-byte malloc */
-            f->buf.capacity = size;
-            for (i = 0; e && i < count; ++i)
-            {
-                struct buffer_entry *next = e->next;
-                buf_copy(&f->buf, &e->buf);
-                buf_write(&f->buf, sep, sep_len);
-                free_buf(&e->buf);
-                free(e);
-                e = next;
-            }
-            bl->head = f;
-            bl->size -= count-1;
-            f->next = more;
-            if (!more)
-            {
-                bl->tail = f;
-            }
+            bl->tail = f;
         }
     }
 }

Signature · 1 line

2.7.4

Steffan Karger

author

2017-12-29 09:54 UTC #21 lore raw

PATCH v2 5/5 buffer_list_aggregate_separator(): simplify code

Clean up the function by slightly simplifying the logic.

Mostly witespace changes, so best reviewed using 'git diff -w'.

Signed-off-by Steffan Karger <[email protected]...>

---
v2: rebase on new version of preceding patches

 src/openvpn/buffer.c | 71 ++++++++++++++++++++++++----------------------------
 1 file changed, 33 insertions(+), 38 deletions(-)

src/openvpn/buffer.c +33 ~~-38~~

index cfe6f2c..858bb08 100644
@@ -1226,49 +1226,44 @@ void
 buffer_list_aggregate_separator(struct buffer_list *bl, const size_t max_len,
                                 const char *sep)
 {
-    int sep_len = strlen(sep);
+    const int sep_len = strlen(sep);
+    struct buffer_entry *more = bl->head;
+    size_t size = 0;
+    int count = 0;
+    for (count = 0; more; ++count)
+    {
+        size_t extra_len = BLEN(&more->buf) + sep_len;
+        if (size + extra_len > max_len)
+        {
+            break;
+        }
+
+        size += extra_len;
+        more = more->next;
+    }
 
-    if (bl->head)
+    if (count >= 2)
     {
-        struct buffer_entry *more = bl->head;
-        size_t size = 0;
-        int count = 0;
-        for (count = 0; more; ++count)
-        {
-            size_t extra_len = BLEN(&more->buf) + sep_len;
-            if (size + extra_len > max_len)
-            {
-                break;
-            }
+        struct buffer_entry *f;
+        ALLOC_OBJ_CLEAR(f, struct buffer_entry);
+        f->buf = alloc_buf(size + 1); /* prevent 0-byte malloc */
 
-            size += extra_len;
-            more = more->next;
+        struct buffer_entry *e = bl->head;
+        for (size_t i = 0; e && i < count; ++i)
+        {
+            struct buffer_entry *next = e->next;
+            buf_copy(&f->buf, &e->buf);
+            buf_write(&f->buf, sep, sep_len);
+            free_buf(&e->buf);
+            free(e);
+            e = next;
         }
-
-        if (count >= 2)
+        bl->head = f;
+        bl->size -= count - 1;
+        f->next = more;
+        if (!more)
         {
-            int i;
-            struct buffer_entry *e = bl->head, *f;
-
-            ALLOC_OBJ_CLEAR(f, struct buffer_entry);
-            f->buf = alloc_buf(size + 1); /* prevent 0-byte malloc */
-            f->buf.capacity = size;
-            for (i = 0; e && i < count; ++i)
-            {
-                struct buffer_entry *next = e->next;
-                buf_copy(&f->buf, &e->buf);
-                buf_write(&f->buf, sep, sep_len);
-                free_buf(&e->buf);
-                free(e);
-                e = next;
-            }
-            bl->head = f;
-            bl->size -= count - 1;
-            f->next = more;
-            if (!more)
-            {
-                bl->tail = f;
-            }
+            bl->tail = f;
         }
     }
 }

Signature · 1 line

2.7.4

Antonio Quartulli

Acked-by on v2 5/5

2018-10-10 08:27 UTC #22 lore raw

Re: [Openvpn-devel] [PATCH 5/5 v2] buffer_list_aggregate_separator(): simplify code

Hi,

Quoted 3 lines · Steffan Karger

Clean up the function by slightly simplifying the logic.
Mostly witespace changes, so best reviewed using 'git diff -w'.

^ missing 'h' here

Quoted 5 lines

Signed-off-by Steffan Karger <[email protected]...>
---
v2: rebase on new version of preceding patches

Other than the typ0 above, the patch does what it says and does not introduce any functional change.

Even though it was "easier" to review with "git show -w", I still had to dig into the original function to make sure I understood the original logic and I was later able to guarantee it was not changed by the patch.

For this reason I wrote some doc for the buffer_list_* functions which I will send as a new patch soon.

Acked-by Antonio Quartulli <[email protected]...>

Signature · 1 line

Antonio Quartulli

David Sommerseth

2018-10-16 20:35 UTC #23 lore raw

Re: [Openvpn-devel] [PATCH applied] buffer_list_aggregate_separator(): simplify code

In reply to Steffan Karger

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

Commit message type fixed during commit.

Your patch has been applied to the following branches

commit e883c66b9da390f56ef4a596c9eb6b237a185a50 (master) commit be47e4292459aabdf89e53d9f5747b113fc2cd5d (release/2.4)

Author: Steffan Karger
Date:   Fri Dec 29 10:54:31 2017 +0100

     buffer_list_aggregate_separator(): simplify code

     Signed-off-by: Steffan Karger <[email protected]...>
     Acked-by: Antonio Quartulli <[email protected]...>
     Message-Id: <[email protected]...>
     URL: https://www.mail-archive.com/[email protected]/msg16105.html
     Signed-off-by: David Sommerseth <[email protected]...>

- --
kind regards,

David Sommerseth

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.22 (GNU/Linux)

iQIcBAEBCgAGBQJbxkuoAAoJEIbPlEyWcf3ypgoQAIpeS0Ick75HV61SgxyM5PCb v3EbcoHCV+ihWKVRg+Q4RhkPz7VTzoMVfPJEj0seZ4RFOJni8XLZLSb62MbWhTqv qFVvYiGHXxBeAYYlcN9qR3QX1efs4kpWMip9AFDmvVzBIiInr1i6nDA5C5obwmnH 2Z8M3lPZLZweAtcL9dHxsSYY+6k/+k5ITmTyrGbPqfK8wniI9ESLdTq5Ga/LD7yx dOFBL0pw83d6umCVFX2LWtq9Hmdhj/sc1IYtpRFIiDdzYZX2KVqrCDsnOSlaJsnP 6a6oQqy47MUJmmX9XrG7R/q/IzGuj5TA6c6I2IpZU/4ojEiYXhKs8YD7D4sLAA1w OElKQHPwCt/YnRXl+cnqs2QJjMaVTXS/X4wg5BPbehJrZvovwK6VsbvExPpJQ/fu qRbCzJqGZ4S3rJjBL3GjVYs50mmgtSJa5tLHzUuDdSYI7FMdkZ9FwH3Spmive39g 2lQ/LQrPqTtg/yOl1wGigKEYKyiIPzY3TorIpokgmZWXI+4t1rRH/0fw4ZfSrM21 hFCKNdFY8blrbUOyryWnWhR30KV/PsFwmc6W7Ht1EfvNWonSzBp9PxPg3ijBz6YQ c6UfkVBteLiVTB5k0jRxgNY7yoJ6B4VodIc9CC7+VBxR4oy/pp14PaX27Oy3/jZM u2XA4B8wbVcuyj8Ytre3
=S2oM
-----END PGP SIGNATURE-----

Gert Doering

2017-11-04 17:47 UTC #24 lore raw

In reply to Steffan Karger

Hi,

Quoted 9 lines · Steffan Karger

Before refactoring this function, add tests to verify the new implementation.
While writing the tests, it became clear that this function is not behaving very well. We'll fix that in follow-up commits.
Signed-off-by Steffan Karger <[email protected]...>
---
 tests/unit_tests/openvpn/test_buffer.c | 181 ++++++++++++++++++++++++++++++++-

Working on my heap of old stuff, I've decided to ACK this ("test code is good!") but it doesn't pass - with just 1/5 applied to master, the test code fails:

[==========] Running 9 test(s).
[ RUN ] test_buffer_strprefix
[ OK ] test_buffer_strprefix
[ RUN ] test_buffer_list_full
[ OK ] test_buffer_list_full
[ RUN ] test_buffer_list_aggregate_separator_empty
[ OK ] test_buffer_list_aggregate_separator_empty
[ RUN ] test_buffer_list_aggregate_separator_noop
[ OK ] test_buffer_list_aggregate_separator_noop
[ RUN ] test_buffer_list_aggregate_separator_two
[ OK ] test_buffer_list_aggregate_separator_two
[ RUN ] test_buffer_list_aggregate_separator_all
[ OK ] test_buffer_list_aggregate_separator_all

[ RUN      ] test_buffer_list_aggregate_separator_nosep
[  ERROR   ] --- "onetwothreeee," != "onetwothree"
[   LINE   ] --- test_buffer.c:169: error: Failure!
[  FAILED  ] test_buffer_list_aggregate_separator_nosep

... so, waiting for a v2...

gert

Signature · 4 lines

USENET is *not* the non-clickable part of WWW!
                                                           //www.muc.de/~gert/
Gert Doering - Munich, Germany                             [email protected]...
fax: +49-89-35655025                        [email protected]...

Steffan Karger

2017-11-04 22:45 UTC #25 lore raw

PATCH v2 1/1 buffer_list_aggregate_separator(): add unit tests

From: Steffan Karger <[email protected]...>

Before refactoring this function, add tests to verify the new implementation.

While writing the tests, it became clear that this function is not behaving very well. We'll fix that in follow-up commits.

Signed-off-by Steffan Karger <[email protected]...>

---
v2: add assert_buf_equals_str macro, and use that to compare (not zero-
terminated) buffer contents with expected strings.

 tests/unit_tests/openvpn/test_buffer.c | 184 ++++++++++++++++++++++++++++++++-
 1 file changed, 182 insertions(+), 2 deletions(-)

tests/unit_tests/openvpn/test_buffer.c +182 -2

index 69bb2e58..ba5aa670 100644
@@ -35,7 +35,7 @@
 #include "buffer.h"
 
 static void
-buffer_strprefix(void **state)
+test_buffer_strprefix(void **state)
 {
     assert_true(strprefix("123456", "123456"));
     assert_true(strprefix("123456", "123"));

@@ -44,11 +44,191 @@ buffer_strprefix(void **state)
     assert_false(strprefix("12", "123"));
 }
 
+#define testsep ","
+#define testnosep ""
+#define teststr1 "one"
+#define teststr2 "two"
+#define teststr3 "three"
+#define teststr4 "four"
+
+#define assert_buf_equals_str(buf, str) \
+    assert_int_equal(BLEN(buf), strlen(str)); \
+    assert_memory_equal(BPTR(buf), str, BLEN(buf));
+
+struct test_buffer_list_aggregate_ctx {
+    struct buffer_list *empty;
+    struct buffer_list *one_two_three;
+    struct buffer_list *zero_length_strings;
+    struct buffer_list *empty_buffers;
+};
+
+static int test_buffer_list_setup(void **state)
+{
+    struct test_buffer_list_aggregate_ctx *ctx  = calloc(1, sizeof(*ctx));
+    ctx->empty = buffer_list_new(0);
+
+    ctx->one_two_three = buffer_list_new(3);
+    buffer_list_push(ctx->one_two_three, teststr1);
+    buffer_list_push(ctx->one_two_three, teststr2);
+    buffer_list_push(ctx->one_two_three, teststr3);
+
+    ctx->zero_length_strings = buffer_list_new(2);
+    buffer_list_push(ctx->zero_length_strings, "");
+    buffer_list_push(ctx->zero_length_strings, "");
+
+    ctx->empty_buffers = buffer_list_new(2);
+    uint8_t *data1 = malloc(1);
+    uint8_t *data2 = malloc(1);
+    buffer_list_push_data(ctx->empty_buffers, data1, 0);
+    buffer_list_push_data(ctx->empty_buffers, data2, 0);
+
+    *state = ctx;
+    return 0;
+}
+
+static int test_buffer_list_teardown(void **state)
+{
+    struct test_buffer_list_aggregate_ctx *ctx = *state;
+
+    buffer_list_free(ctx->empty);
+    buffer_list_free(ctx->one_two_three);
+    buffer_list_free(ctx->zero_length_strings);
+    buffer_list_free(ctx->empty_buffers);
+    free(ctx);
+    return 0;
+}
+
+static void
+test_buffer_list_full(void **state)
+{
+    struct test_buffer_list_aggregate_ctx *ctx = *state;
+
+    /* list full */
+    assert_int_equal(ctx->one_two_three->size, 3);
+    buffer_list_push(ctx->one_two_three, teststr4);
+    assert_int_equal(ctx->one_two_three->size, 3);
+}
+
+static void
+test_buffer_list_aggregate_separator_empty(void **state)
+{
+    struct test_buffer_list_aggregate_ctx *ctx = *state;
+
+    /* aggregating an empty buffer list results in an empty buffer list */
+    buffer_list_aggregate_separator(ctx->empty, 3, testsep);
+    assert_null(ctx->empty->head);
+}
+
+static void
+test_buffer_list_aggregate_separator_noop(void **state)
+{
+    struct test_buffer_list_aggregate_ctx *ctx = *state;
+
+    /* With a max length of 2, no aggregation should take place */
+    buffer_list_aggregate_separator(ctx->one_two_three, 2, testsep);
+    assert_int_equal(ctx->one_two_three->size, 3);
+    struct buffer *buf = buffer_list_peek(ctx->one_two_three);
+    assert_buf_equals_str(buf, teststr1);
+}
+
+static void
+test_buffer_list_aggregate_separator_two(void **state)
+{
+    struct test_buffer_list_aggregate_ctx *ctx = *state;
+
+    /* Aggregate the first two elements */
+    /* FIXME this exceeds the supplied max */
+    buffer_list_aggregate_separator(ctx->one_two_three, 4, testsep);
+    /* FIXME size does not get adjusted after aggregating */
+    assert_int_equal(ctx->one_two_three->size, 3);
+    struct buffer *buf = buffer_list_peek(ctx->one_two_three);
+    assert_buf_equals_str(buf, teststr1 testsep teststr2 testsep);
+}
+
+static void
+test_buffer_list_aggregate_separator_all(void **state)
+{
+    struct test_buffer_list_aggregate_ctx *ctx = *state;
+
+    /* Aggregate all */
+    buffer_list_aggregate_separator(ctx->one_two_three, 1<<16, testsep);
+    /* FIXME size does not get adjusted after aggregating */
+    assert_int_equal(ctx->one_two_three->size, 3);
+    struct buffer *buf = buffer_list_peek(ctx->one_two_three);
+    assert_buf_equals_str(buf,
+                          teststr1 testsep teststr2 testsep teststr3 testsep);
+}
+
+static void
+test_buffer_list_aggregate_separator_nosep(void **state)
+{
+    struct test_buffer_list_aggregate_ctx *ctx = *state;
+
+    /* Aggregate all */
+    buffer_list_aggregate_separator(ctx->one_two_three, 1<<16, testnosep);
+    /* FIXME size does not get adjusted after aggregating */
+    assert_int_equal(ctx->one_two_three->size, 3);
+    struct buffer *buf = buffer_list_peek(ctx->one_two_three);
+    assert_buf_equals_str(buf, teststr1 teststr2 teststr3);
+}
+
+static void
+test_buffer_list_aggregate_separator_zerolen(void **state)
+{
+    struct test_buffer_list_aggregate_ctx *ctx = *state;
+    struct buffer_list *bl_zerolen = ctx->zero_length_strings;
+
+    /* Aggregate all */
+    buffer_list_aggregate_separator(bl_zerolen, 1<<16, testnosep);
+    /* FIXME size does not get adjusted after aggregating */
+    assert_int_equal(bl_zerolen->size, 2);
+    struct buffer *buf = buffer_list_peek(bl_zerolen);
+    assert_buf_equals_str(buf, "");
+}
+
+static void
+test_buffer_list_aggregate_separator_emptybuffers(void **state)
+{
+    struct test_buffer_list_aggregate_ctx *ctx = *state;
+    struct buffer_list *bl_emptybuffers = ctx->empty_buffers;
+
+    /* Aggregate all */
+    buffer_list_aggregate_separator(bl_emptybuffers, 1<<16, testnosep);
+    /* FIXME size does not get adjusted after aggregating */
+    assert_int_equal(bl_emptybuffers->size, 2);
+    struct buffer *buf = buffer_list_peek(bl_emptybuffers);
+    assert_int_equal(BLEN(buf), 0);
+}
+
 int
 main(void)
 {
     const struct CMUnitTest tests[] = {
-        cmocka_unit_test(buffer_strprefix),
+        cmocka_unit_test(test_buffer_strprefix),
+        cmocka_unit_test_setup_teardown(test_buffer_list_full,
+                                        test_buffer_list_setup,
+                                        test_buffer_list_teardown),
+        cmocka_unit_test_setup_teardown(test_buffer_list_aggregate_separator_empty,
+                                        test_buffer_list_setup,
+                                        test_buffer_list_teardown),
+        cmocka_unit_test_setup_teardown(test_buffer_list_aggregate_separator_noop,
+                                        test_buffer_list_setup,
+                                        test_buffer_list_teardown),
+        cmocka_unit_test_setup_teardown(test_buffer_list_aggregate_separator_two,
+                                        test_buffer_list_setup,
+                                        test_buffer_list_teardown),
+        cmocka_unit_test_setup_teardown(test_buffer_list_aggregate_separator_all,
+                                        test_buffer_list_setup,
+                                        test_buffer_list_teardown),
+        cmocka_unit_test_setup_teardown(test_buffer_list_aggregate_separator_nosep,
+                                        test_buffer_list_setup,
+                                        test_buffer_list_teardown),
+        cmocka_unit_test_setup_teardown(test_buffer_list_aggregate_separator_zerolen,
+                                        test_buffer_list_setup,
+                                        test_buffer_list_teardown),
+        cmocka_unit_test_setup_teardown(test_buffer_list_aggregate_separator_emptybuffers,
+                                        test_buffer_list_setup,
+                                        test_buffer_list_teardown),
     };
 
     return cmocka_run_group_tests_name("buffer", tests, NULL, NULL);

Signature · 1 line

2.14.1

Gert Doering

2017-11-05 10:12 UTC #26 lore raw

ACK, test code is good :-) (it passes the tests now, and does not change anything else). Applying to 2.4 as well, since "test good is good", even if we might not apply the refactoring bits there.

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

commit 2ddb527abe38f5866ff01e91f8ee89d0f9700762 (master) commit a202f6e74b32beabad764ee4226781a7f7e9c1c6 (release/2.4)

Author: Steffan Karger
Date:   Sat Nov 4 23:45:50 2017 +0100

     buffer_list_aggregate_separator(): add unit tests

     Signed-off-by: Steffan Karger <[email protected]...>
     Acked-by: Gert Doering <[email protected]...>
     Message-Id: <[email protected]...>
     URL: https://www.mail-archive.com/[email protected]/msg15748.html
     Signed-off-by: Gert Doering <[email protected]...>

--
kind regards,

Gert Doering