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

Message ID 1510236133-6926-1-git-send-email-steffan.karger@fox-it.com
State Superseded
Headers show
Series None | expand

Commit Message

Steffan Karger Nov. 9, 2017, 3:02 a.m. UTC
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 <steffan.karger@fox-it.com>
---
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(-)

Comments

Antonio Quartulli Dec. 28, 2017, 11:51 a.m. UTC | #1
Hi,

On 09/11/17 22:02, Steffan Karger wrote:
> 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 <steffan.karger@fox-it.com>
> ---
> 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(-)
> 
> diff --git a/src/openvpn/buffer.c b/src/openvpn/buffer.c
> index 583c5c7..f72d525 100644
> --- a/src/openvpn/buffer.c
> +++ b/src/openvpn/buffer.c
> @@ -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;
> +            }

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.

>          }
>  
>          if (count >= 2)
> diff --git a/src/openvpn/buffer.h b/src/openvpn/buffer.h
> index 1ed5631..499c75e 100644
> --- a/src/openvpn/buffer.h
> +++ b/src/openvpn/buffer.h
> @@ -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)

>  
>  struct buffer_list *buffer_list_file(const char *fn, int max_line_len);
>  
> diff --git a/tests/unit_tests/openvpn/test_buffer.c b/tests/unit_tests/openvpn/test_buffer.c
> index f5830a1..99e6530 100644
> --- a/tests/unit_tests/openvpn/test_buffer.c
> +++ b/tests/unit_tests/openvpn/test_buffer.c
> @@ -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);

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

>      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 <a@unstable.cc>


Cheers,

Patch

diff --git a/src/openvpn/buffer.c b/src/openvpn/buffer.c
index 583c5c7..f72d525 100644
--- a/src/openvpn/buffer.c
+++ b/src/openvpn/buffer.c
@@ -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)
diff --git a/src/openvpn/buffer.h b/src/openvpn/buffer.h
index 1ed5631..499c75e 100644
--- a/src/openvpn/buffer.h
+++ b/src/openvpn/buffer.h
@@ -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);
 
diff --git a/tests/unit_tests/openvpn/test_buffer.c b/tests/unit_tests/openvpn/test_buffer.c
index f5830a1..99e6530 100644
--- a/tests/unit_tests/openvpn/test_buffer.c
+++ b/tests/unit_tests/openvpn/test_buffer.c
@@ -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