[Tarantool-patches] [PATCH v3 0/2] base64: fix decoder, improve its performance

[Tarantool-patches] [PATCH v3 0/2] base64: fix decoder, improve its performance

[Sergey Nikiforov]

First patch fixes #3069, second one improves base64 decoder performance

Branch: https://github.com/tarantool/tarantool/tree/void234/gh-3069-fix-base64-memory-overrun-v3
Issue: https://github.com/tarantool/tarantool/issues/3069

Sergey Nikiforov (2):
  base64: fix decoder output buffer overrun (reads)
  base64: improve decoder performance

 test/unit/base64.c      |  7 +++-
 test/unit/base64.result | 84 +++++++++++++++++++++++++++--------------
 third_party/base64.c    | 58 +++++++++++++++++++---------
 3 files changed, 103 insertions(+), 46 deletions(-)

-- 
2.25.1

[Tarantool-patches] [PATCH v3 1/2] base64: fix decoder output buffer overrun (reads)

[Sergey Nikiforov]

Was caught by base64 test with enabled ASAN.

It also caused data corruption - garbage instead of "extra bits" was
saved into state->result if there was no space in output buffer.

Fixes: #3069
---

Branch: https://github.com/tarantool/tarantool/tree/void234/gh-3069-fix-base64-memory-overrun-v3
Issue: https://github.com/tarantool/tarantool/issues/3069

 third_party/base64.c | 28 +++++++++++++++++-----------
 1 file changed, 17 insertions(+), 11 deletions(-)

diff --git a/third_party/base64.c b/third_party/base64.c
index 8ecab23eb..3350a98ff 100644
--- a/third_party/base64.c
+++ b/third_party/base64.c
@@ -247,8 +247,9 @@ base64_decode_block(const char *in_base64, int in_len,
 	char *out_pos = out_bin;
 	char *out_end = out_bin + out_len;
 	int fragment;
+	char curr_byte;
 
-	*out_pos = state->result;
+	curr_byte = state->result;
 
 	switch (state->step)
 	{
@@ -259,49 +260,54 @@ base64_decode_block(const char *in_base64, int in_len,
 				if (in_pos == in_end || out_pos >= out_end)
 				{
 					state->step = step_a;
-					state->result = *out_pos;
+					state->result = curr_byte;
 					return out_pos - out_bin;
 				}
 				fragment = base64_decode_value(*in_pos++);
 			} while (fragment < 0);
-			*out_pos    = (fragment & 0x03f) << 2;
+			curr_byte = (fragment & 0x03f) << 2;
 	case step_b:
 			do {
 				if (in_pos == in_end || out_pos >= out_end)
 				{
 					state->step = step_b;
-					state->result = *out_pos;
+					state->result = curr_byte;
 					return out_pos - out_bin;
 				}
 				fragment = base64_decode_value(*in_pos++);
 			} while (fragment < 0);
-			*out_pos++ |= (fragment & 0x030) >> 4;
+			curr_byte |= (fragment & 0x030) >> 4;
+			*out_pos++ = curr_byte;
+			curr_byte = (fragment & 0x00f) << 4;
 			if (out_pos < out_end)
-				*out_pos = (fragment & 0x00f) << 4;
+				*out_pos = curr_byte;
 	case step_c:
 			do {
 				if (in_pos == in_end || out_pos >= out_end)
 				{
 					state->step = step_c;
-					state->result = *out_pos;
+					state->result = curr_byte;
 					return out_pos - out_bin;
 				}
 				fragment = base64_decode_value(*in_pos++);
 			} while (fragment < 0);
-			*out_pos++ |= (fragment & 0x03c) >> 2;
+			curr_byte |= (fragment & 0x03c) >> 2;
+			*out_pos++ = curr_byte;
+			curr_byte = (fragment & 0x003) << 6;
 			if (out_pos < out_end)
-				*out_pos = (fragment & 0x003) << 6;
+				*out_pos = curr_byte;
 	case step_d:
 			do {
 				if (in_pos == in_end || out_pos >= out_end)
 				{
 					state->step = step_d;
-					state->result = *out_pos;
+					state->result = curr_byte;
 					return out_pos - out_bin;
 				}
 				fragment = base64_decode_value(*in_pos++);
 			} while (fragment < 0);
-			*out_pos++   |= (fragment & 0x03f);
+			curr_byte |= (fragment & 0x03f);
+			*out_pos++ = curr_byte;
 		}
 	}
 	/* control should not reach here */
-- 
2.25.1

[Tarantool-patches] [PATCH v3 2/2] base64: improve decoder performance

[Sergey Nikiforov]

Unnecessary checks were removed from internal loops.
Benchmark shows that performance is now ~1.19 times higher
(release build, Intel Core I7-9700K, only one thread).
---

Branch: https://github.com/tarantool/tarantool/tree/void234/gh-3069-fix-base64-memory-overrun-v3

 test/unit/base64.c      |  7 +++-
 test/unit/base64.result | 84 +++++++++++++++++++++++++++--------------
 third_party/base64.c    | 36 +++++++++++++-----
 3 files changed, 89 insertions(+), 38 deletions(-)

diff --git a/test/unit/base64.c b/test/unit/base64.c
index ada497adf..76db7d782 100644
--- a/test/unit/base64.c
+++ b/test/unit/base64.c
@@ -7,7 +7,7 @@ static void
 base64_test(const char *str, int options, const char *no_symbols,
 	    int no_symbols_len)
 {
-	plan(3 + no_symbols_len);
+	plan(4 + no_symbols_len);
 
 	int len = strlen(str);
 	int base64_buflen = base64_bufsize(len + 1, options);
@@ -34,6 +34,11 @@ base64_test(const char *str, int options, const char *no_symbols,
 	free(base64_buf);
 	free(strbuf);
 
+	const char *in = "sIIpHw==";
+	int in_len = strlen(in);
+	rc = base64_decode(in, in_len, NULL, 0);
+	is(rc, 0, "no space in out buffer");
+
 	check_plan();
 }
 
diff --git a/test/unit/base64.result b/test/unit/base64.result
index cd1f2b3f6..d606772ea 100644
--- a/test/unit/base64.result
+++ b/test/unit/base64.result
@@ -1,178 +1,206 @@
 1..28
 	*** main ***
-    1..3
+    1..4
     ok 1 - length
     ok 2 - decode length ok
     ok 3 - encode/decode
+    ok 4 - no space in out buffer
 ok 1 - subtests
-    1..6
+    1..7
     ok 1 - length
     ok 2 - no \n symbols
     ok 3 - no + symbols
     ok 4 - no = symbols
     ok 5 - decode length ok
     ok 6 - encode/decode
+    ok 7 - no space in out buffer
 ok 2 - subtests
-    1..4
+    1..5
     ok 1 - length
     ok 2 - no = symbols
     ok 3 - decode length ok
     ok 4 - encode/decode
+    ok 5 - no space in out buffer
 ok 3 - subtests
-    1..4
+    1..5
     ok 1 - length
     ok 2 - no \n symbols
     ok 3 - decode length ok
     ok 4 - encode/decode
+    ok 5 - no space in out buffer
 ok 4 - subtests
-    1..3
+    1..4
     ok 1 - length
     ok 2 - decode length ok
     ok 3 - encode/decode
+    ok 4 - no space in out buffer
 ok 5 - subtests
-    1..6
+    1..7
     ok 1 - length
     ok 2 - no \n symbols
     ok 3 - no + symbols
     ok 4 - no = symbols
     ok 5 - decode length ok
     ok 6 - encode/decode
+    ok 7 - no space in out buffer
 ok 6 - subtests
-    1..4
+    1..5
     ok 1 - length
     ok 2 - no = symbols
     ok 3 - decode length ok
     ok 4 - encode/decode
+    ok 5 - no space in out buffer
 ok 7 - subtests
-    1..4
+    1..5
     ok 1 - length
     ok 2 - no \n symbols
     ok 3 - decode length ok
     ok 4 - encode/decode
+    ok 5 - no space in out buffer
 ok 8 - subtests
-    1..3
+    1..4
     ok 1 - length
     ok 2 - decode length ok
     ok 3 - encode/decode
+    ok 4 - no space in out buffer
 ok 9 - subtests
-    1..6
+    1..7
     ok 1 - length
     ok 2 - no \n symbols
     ok 3 - no + symbols
     ok 4 - no = symbols
     ok 5 - decode length ok
     ok 6 - encode/decode
+    ok 7 - no space in out buffer
 ok 10 - subtests
-    1..4
+    1..5
     ok 1 - length
     ok 2 - no = symbols
     ok 3 - decode length ok
     ok 4 - encode/decode
+    ok 5 - no space in out buffer
 ok 11 - subtests
-    1..4
+    1..5
     ok 1 - length
     ok 2 - no \n symbols
     ok 3 - decode length ok
     ok 4 - encode/decode
+    ok 5 - no space in out buffer
 ok 12 - subtests
-    1..3
+    1..4
     ok 1 - length
     ok 2 - decode length ok
     ok 3 - encode/decode
+    ok 4 - no space in out buffer
 ok 13 - subtests
-    1..6
+    1..7
     ok 1 - length
     ok 2 - no \n symbols
     ok 3 - no + symbols
     ok 4 - no = symbols
     ok 5 - decode length ok
     ok 6 - encode/decode
+    ok 7 - no space in out buffer
 ok 14 - subtests
-    1..4
+    1..5
     ok 1 - length
     ok 2 - no = symbols
     ok 3 - decode length ok
     ok 4 - encode/decode
+    ok 5 - no space in out buffer
 ok 15 - subtests
-    1..4
+    1..5
     ok 1 - length
     ok 2 - no \n symbols
     ok 3 - decode length ok
     ok 4 - encode/decode
+    ok 5 - no space in out buffer
 ok 16 - subtests
-    1..3
+    1..4
     ok 1 - length
     ok 2 - decode length ok
     ok 3 - encode/decode
+    ok 4 - no space in out buffer
 ok 17 - subtests
-    1..6
+    1..7
     ok 1 - length
     ok 2 - no \n symbols
     ok 3 - no + symbols
     ok 4 - no = symbols
     ok 5 - decode length ok
     ok 6 - encode/decode
+    ok 7 - no space in out buffer
 ok 18 - subtests
-    1..4
+    1..5
     ok 1 - length
     ok 2 - no = symbols
     ok 3 - decode length ok
     ok 4 - encode/decode
+    ok 5 - no space in out buffer
 ok 19 - subtests
-    1..4
+    1..5
     ok 1 - length
     ok 2 - no \n symbols
     ok 3 - decode length ok
     ok 4 - encode/decode
+    ok 5 - no space in out buffer
 ok 20 - subtests
-    1..3
+    1..4
     ok 1 - length
     ok 2 - decode length ok
     ok 3 - encode/decode
+    ok 4 - no space in out buffer
 ok 21 - subtests
-    1..6
+    1..7
     ok 1 - length
     ok 2 - no \n symbols
     ok 3 - no + symbols
     ok 4 - no = symbols
     ok 5 - decode length ok
     ok 6 - encode/decode
+    ok 7 - no space in out buffer
 ok 22 - subtests
-    1..4
+    1..5
     ok 1 - length
     ok 2 - no = symbols
     ok 3 - decode length ok
     ok 4 - encode/decode
+    ok 5 - no space in out buffer
 ok 23 - subtests
-    1..4
+    1..5
     ok 1 - length
     ok 2 - no \n symbols
     ok 3 - decode length ok
     ok 4 - encode/decode
+    ok 5 - no space in out buffer
 ok 24 - subtests
-    1..3
+    1..4
     ok 1 - length
     ok 2 - decode length ok
     ok 3 - encode/decode
+    ok 4 - no space in out buffer
 ok 25 - subtests
-    1..6
+    1..7
     ok 1 - length
     ok 2 - no \n symbols
     ok 3 - no + symbols
     ok 4 - no = symbols
     ok 5 - decode length ok
     ok 6 - encode/decode
+    ok 7 - no space in out buffer
 ok 26 - subtests
-    1..4
+    1..5
     ok 1 - length
     ok 2 - no = symbols
     ok 3 - decode length ok
     ok 4 - encode/decode
+    ok 5 - no space in out buffer
 ok 27 - subtests
-    1..4
+    1..5
     ok 1 - length
     ok 2 - no \n symbols
     ok 3 - decode length ok
     ok 4 - encode/decode
+    ok 5 - no space in out buffer
 ok 28 - subtests
 	*** main: done ***
diff --git a/third_party/base64.c b/third_party/base64.c
index 3350a98ff..93442c04b 100644
--- a/third_party/base64.c
+++ b/third_party/base64.c
@@ -257,10 +257,11 @@ base64_decode_block(const char *in_base64, int in_len,
 		{
 	case step_a:
 			do {
-				if (in_pos == in_end || out_pos >= out_end)
+				if (in_pos >= in_end)
 				{
 					state->step = step_a;
-					state->result = curr_byte;
+					/* curr_byte is useless now */
+					/* state->result = curr_byte; */
 					return out_pos - out_bin;
 				}
 				fragment = base64_decode_value(*in_pos++);
@@ -268,7 +269,7 @@ base64_decode_block(const char *in_base64, int in_len,
 			curr_byte = (fragment & 0x03f) << 2;
 	case step_b:
 			do {
-				if (in_pos == in_end || out_pos >= out_end)
+				if (in_pos >= in_end)
 				{
 					state->step = step_b;
 					state->result = curr_byte;
@@ -276,14 +277,19 @@ base64_decode_block(const char *in_base64, int in_len,
 				}
 				fragment = base64_decode_value(*in_pos++);
 			} while (fragment < 0);
+			if (out_pos >= out_end)
+			{
+				/* We are losing some data */
+				state->step = step_b;
+				state->result = curr_byte;
+				return out_pos - out_bin;
+			}
 			curr_byte |= (fragment & 0x030) >> 4;
 			*out_pos++ = curr_byte;
 			curr_byte = (fragment & 0x00f) << 4;
-			if (out_pos < out_end)
-				*out_pos = curr_byte;
 	case step_c:
 			do {
-				if (in_pos == in_end || out_pos >= out_end)
+				if (in_pos >= in_end)
 				{
 					state->step = step_c;
 					state->result = curr_byte;
@@ -291,14 +297,19 @@ base64_decode_block(const char *in_base64, int in_len,
 				}
 				fragment = base64_decode_value(*in_pos++);
 			} while (fragment < 0);
+			if (out_pos >= out_end)
+			{
+				/* We are losing some data */
+				state->step = step_c;
+				state->result = curr_byte;
+				return out_pos - out_bin;
+			}
 			curr_byte |= (fragment & 0x03c) >> 2;
 			*out_pos++ = curr_byte;
 			curr_byte = (fragment & 0x003) << 6;
-			if (out_pos < out_end)
-				*out_pos = curr_byte;
 	case step_d:
 			do {
-				if (in_pos == in_end || out_pos >= out_end)
+				if (in_pos >= in_end)
 				{
 					state->step = step_d;
 					state->result = curr_byte;
@@ -306,6 +317,13 @@ base64_decode_block(const char *in_base64, int in_len,
 				}
 				fragment = base64_decode_value(*in_pos++);
 			} while (fragment < 0);
+			if (out_pos >= out_end)
+			{
+				/* We are losing some data */
+				state->step = step_d;
+				state->result = curr_byte;
+				return out_pos - out_bin;
+			}
 			curr_byte |= (fragment & 0x03f);
 			*out_pos++ = curr_byte;
 		}
-- 
2.25.1

Re: [Tarantool-patches] [PATCH v3 2/2] base64: improve decoder performance

[Vladislav Shpilevoy]

Thanks for the patch!

> diff --git a/test/unit/base64.c b/test/unit/base64.c
> index ada497adf..76db7d782 100644
> --- a/test/unit/base64.c
> +++ b/test/unit/base64.c
> @@ -34,6 +34,11 @@ base64_test(const char *str, int options, const char *no_symbols,
>  	free(base64_buf);
>  	free(strbuf);
>  
> +	const char *in = "sIIpHw==";
> +	int in_len = strlen(in);
> +	rc = base64_decode(in, in_len, NULL, 0);
> +	is(rc, 0, "no space in out buffer");

From the .result file it is clear this test is done multiple
times. And it does not depend on base64_test() arguments. So why
do you run exactly the same test again and again? Maybe move it
to a separate test function which is run only once?

> +
>  	check_plan();
>  }
>  
> diff --git a/test/unit/base64.result b/test/unit/base64.result
> index cd1f2b3f6..d606772ea 100644
> --- a/test/unit/base64.result
> +++ b/test/unit/base64.result
> @@ -1,178 +1,206 @@
>  1..28
>  	*** main ***
> -    1..3
> +    1..4
>      ok 1 - length
>      ok 2 - decode length ok
>      ok 3 - encode/decode
> +    ok 4 - no space in out buffer
>  ok 1 - subtests
> -    1..6
> +    1..7
>      ok 1 - length
>      ok 2 - no \n symbols
>      ok 3 - no + symbols
>      ok 4 - no = symbols
>      ok 5 - decode length ok
>      ok 6 - encode/decode
> +    ok 7 - no space in out buffer
>  ok 2 - subtests
> -    1..4
> +    1..5
>      ok 1 - length
>      ok 2 - no = symbols
>      ok 3 - decode length ok
>      ok 4 - encode/decode
> +    ok 5 - no space in out buffer
>  ok 3 - subtests
> -    1..4
> +    1..5
>      ok 1 - length
>      ok 2 - no \n symbols
>      ok 3 - decode length ok
>      ok 4 - encode/decode
> +    ok 5 - no space in out buffer
>  ok 4 - subtests
> -    1..3
> +    1..4
>      ok 1 - length
>      ok 2 - decode length ok
>      ok 3 - encode/decode
> +    ok 4 - no space in out buffer
>  ok 5 - subtests
> -    1..6
> +    1..7
>      ok 1 - length
>      ok 2 - no \n symbols
>      ok 3 - no + symbols
>      ok 4 - no = symbols
>      ok 5 - decode length ok
>      ok 6 - encode/decode
> +    ok 7 - no space in out buffer
>  ok 6 - subtests
> -    1..4
> +    1..5
>      ok 1 - length
>      ok 2 - no = symbols
>      ok 3 - decode length ok
>      ok 4 - encode/decode
> +    ok 5 - no space in out buffer
>  ok 7 - subtests
> -    1..4
> +    1..5
>      ok 1 - length
>      ok 2 - no \n symbols
>      ok 3 - decode length ok
>      ok 4 - encode/decode
> +    ok 5 - no space in out buffer
>  ok 8 - subtests
> -    1..3
> +    1..4
>      ok 1 - length
>      ok 2 - decode length ok
>      ok 3 - encode/decode
> +    ok 4 - no space in out buffer
>  ok 9 - subtests
> -    1..6
> +    1..7
>      ok 1 - length
>      ok 2 - no \n symbols
>      ok 3 - no + symbols
>      ok 4 - no = symbols
>      ok 5 - decode length ok
>      ok 6 - encode/decode
> +    ok 7 - no space in out buffer
>  ok 10 - subtests
> -    1..4
> +    1..5
>      ok 1 - length
>      ok 2 - no = symbols
>      ok 3 - decode length ok
>      ok 4 - encode/decode
> +    ok 5 - no space in out buffer
>  ok 11 - subtests
> -    1..4
> +    1..5
>      ok 1 - length
>      ok 2 - no \n symbols
>      ok 3 - decode length ok
>      ok 4 - encode/decode
> +    ok 5 - no space in out buffer
>  ok 12 - subtests
> -    1..3
> +    1..4
>      ok 1 - length
>      ok 2 - decode length ok
>      ok 3 - encode/decode
> +    ok 4 - no space in out buffer
>  ok 13 - subtests
> -    1..6
> +    1..7
>      ok 1 - length
>      ok 2 - no \n symbols
>      ok 3 - no + symbols
>      ok 4 - no = symbols
>      ok 5 - decode length ok
>      ok 6 - encode/decode
> +    ok 7 - no space in out buffer
>  ok 14 - subtests
> -    1..4
> +    1..5
>      ok 1 - length
>      ok 2 - no = symbols
>      ok 3 - decode length ok
>      ok 4 - encode/decode
> +    ok 5 - no space in out buffer
>  ok 15 - subtests
> -    1..4
> +    1..5
>      ok 1 - length
>      ok 2 - no \n symbols
>      ok 3 - decode length ok
>      ok 4 - encode/decode
> +    ok 5 - no space in out buffer
>  ok 16 - subtests
> -    1..3
> +    1..4
>      ok 1 - length
>      ok 2 - decode length ok
>      ok 3 - encode/decode
> +    ok 4 - no space in out buffer
>  ok 17 - subtests
> -    1..6
> +    1..7
>      ok 1 - length
>      ok 2 - no \n symbols
>      ok 3 - no + symbols
>      ok 4 - no = symbols
>      ok 5 - decode length ok
>      ok 6 - encode/decode
> +    ok 7 - no space in out buffer
>  ok 18 - subtests
> -    1..4
> +    1..5
>      ok 1 - length
>      ok 2 - no = symbols
>      ok 3 - decode length ok
>      ok 4 - encode/decode
> +    ok 5 - no space in out buffer
>  ok 19 - subtests
> -    1..4
> +    1..5
>      ok 1 - length
>      ok 2 - no \n symbols
>      ok 3 - decode length ok
>      ok 4 - encode/decode
> +    ok 5 - no space in out buffer
>  ok 20 - subtests
> -    1..3
> +    1..4
>      ok 1 - length
>      ok 2 - decode length ok
>      ok 3 - encode/decode
> +    ok 4 - no space in out buffer
>  ok 21 - subtests
> -    1..6
> +    1..7
>      ok 1 - length
>      ok 2 - no \n symbols
>      ok 3 - no + symbols
>      ok 4 - no = symbols
>      ok 5 - decode length ok
>      ok 6 - encode/decode
> +    ok 7 - no space in out buffer
>  ok 22 - subtests
> -    1..4
> +    1..5
>      ok 1 - length
>      ok 2 - no = symbols
>      ok 3 - decode length ok
>      ok 4 - encode/decode
> +    ok 5 - no space in out buffer
>  ok 23 - subtests
> -    1..4
> +    1..5
>      ok 1 - length
>      ok 2 - no \n symbols
>      ok 3 - decode length ok
>      ok 4 - encode/decode
> +    ok 5 - no space in out buffer
>  ok 24 - subtests
> -    1..3
> +    1..4
>      ok 1 - length
>      ok 2 - decode length ok
>      ok 3 - encode/decode
> +    ok 4 - no space in out buffer
>  ok 25 - subtests
> -    1..6
> +    1..7
>      ok 1 - length
>      ok 2 - no \n symbols
>      ok 3 - no + symbols
>      ok 4 - no = symbols
>      ok 5 - decode length ok
>      ok 6 - encode/decode
> +    ok 7 - no space in out buffer
>  ok 26 - subtests
> -    1..4
> +    1..5
>      ok 1 - length
>      ok 2 - no = symbols
>      ok 3 - decode length ok
>      ok 4 - encode/decode
> +    ok 5 - no space in out buffer
>  ok 27 - subtests
> -    1..4
> +    1..5
>      ok 1 - length
>      ok 2 - no \n symbols
>      ok 3 - decode length ok
>      ok 4 - encode/decode
> +    ok 5 - no space in out buffer
>  ok 28 - subtests

Re: [Tarantool-patches] [PATCH v3 2/2] base64: improve decoder performance

[Sergey Nikiforov]

Hi!

On 22.12.2020 18:05, Vladislav Shpilevoy wrote:
> Thanks for the patch!
> 
>> diff --git a/test/unit/base64.c b/test/unit/base64.c
>> index ada497adf..76db7d782 100644
>> --- a/test/unit/base64.c
>> +++ b/test/unit/base64.c
>> @@ -34,6 +34,11 @@ base64_test(const char *str, int options, const char *no_symbols,
>>   	free(base64_buf);
>>   	free(strbuf);
>>   
>> +	const char *in = "sIIpHw==";
>> +	int in_len = strlen(in);
>> +	rc = base64_decode(in, in_len, NULL, 0);
>> +	is(rc, 0, "no space in out buffer");
> 
>  From the .result file it is clear this test is done multiple
> times. And it does not depend on base64_test() arguments. So why
> do you run exactly the same test again and again? Maybe move it
> to a separate test function which is run only once?

Ok, I will do this if there will be another patch revision.
You are, however, reviewing your own code - I have just used your patch 
from earlier review iteration (17 Dec 2020 00:22:08 +0100)

Re: [Tarantool-patches] [PATCH v3 2/2] base64: improve decoder performance

[Vladislav Shpilevoy]

>>> diff --git a/test/unit/base64.c b/test/unit/base64.c
>>> index ada497adf..76db7d782 100644
>>> --- a/test/unit/base64.c
>>> +++ b/test/unit/base64.c
>>> @@ -34,6 +34,11 @@ base64_test(const char *str, int options, const char *no_symbols,
>>>       free(base64_buf);
>>>       free(strbuf);
>>>   +    const char *in = "sIIpHw==";
>>> +    int in_len = strlen(in);
>>> +    rc = base64_decode(in, in_len, NULL, 0);
>>> +    is(rc, 0, "no space in out buffer");
>>
>>  From the .result file it is clear this test is done multiple
>> times. And it does not depend on base64_test() arguments. So why
>> do you run exactly the same test again and again? Maybe move it
>> to a separate test function which is run only once?
> 
> Ok, I will do this if there will be another patch revision.
> You are, however, reviewing your own code - I have just used your patch from earlier review iteration (17 Dec 2020 00:22:08 +0100)

I used it as an example. You don't need to blindly merge
everything I post here. Because the patch is yours, not mine.
And I may be wrong quite often.

Re: [Tarantool-patches] [PATCH v3 1/2] base64: fix decoder output buffer overrun (reads)

[Leonid Vasiliev]

Hi!
Thank you for the patch.

I have some comments:

- Why are you adding a test in the next commit? Seems like this test is
validating this fix.
- I agree with Vlad that the test should be moved to a separate test
function.

The changes in third_party/base64.c - LGTM.

On 22.12.2020 13:41, Sergey Nikiforov wrote:
> Was caught by base64 test with enabled ASAN.
> 
> It also caused data corruption - garbage instead of "extra bits" was
> saved into state->result if there was no space in output buffer.
> 
> Fixes: #3069
> ---
> 
> Branch: https://github.com/tarantool/tarantool/tree/void234/gh-3069-fix-base64-memory-overrun-v3
> Issue: https://github.com/tarantool/tarantool/issues/3069
> 
>   third_party/base64.c | 28 +++++++++++++++++-----------
>   1 file changed, 17 insertions(+), 11 deletions(-)
> 
> diff --git a/third_party/base64.c b/third_party/base64.c
> index 8ecab23eb..3350a98ff 100644
> --- a/third_party/base64.c
> +++ b/third_party/base64.c
> @@ -247,8 +247,9 @@ base64_decode_block(const char *in_base64, int in_len,
>   	char *out_pos = out_bin;
>   	char *out_end = out_bin + out_len;
>   	int fragment;
> +	char curr_byte;
>   
> -	*out_pos = state->result;
> +	curr_byte = state->result;
>   
>   	switch (state->step)
>   	{
> @@ -259,49 +260,54 @@ base64_decode_block(const char *in_base64, int in_len,
>   				if (in_pos == in_end || out_pos >= out_end)
>   				{
>   					state->step = step_a;
> -					state->result = *out_pos;
> +					state->result = curr_byte;
>   					return out_pos - out_bin;
>   				}
>   				fragment = base64_decode_value(*in_pos++);
>   			} while (fragment < 0);
> -			*out_pos    = (fragment & 0x03f) << 2;
> +			curr_byte = (fragment & 0x03f) << 2;
>   	case step_b:
>   			do {
>   				if (in_pos == in_end || out_pos >= out_end)
>   				{
>   					state->step = step_b;
> -					state->result = *out_pos;
> +					state->result = curr_byte;
>   					return out_pos - out_bin;
>   				}
>   				fragment = base64_decode_value(*in_pos++);
>   			} while (fragment < 0);
> -			*out_pos++ |= (fragment & 0x030) >> 4;
> +			curr_byte |= (fragment & 0x030) >> 4;
> +			*out_pos++ = curr_byte;
> +			curr_byte = (fragment & 0x00f) << 4;
>   			if (out_pos < out_end)
> -				*out_pos = (fragment & 0x00f) << 4;
> +				*out_pos = curr_byte;
>   	case step_c:
>   			do {
>   				if (in_pos == in_end || out_pos >= out_end)
>   				{
>   					state->step = step_c;
> -					state->result = *out_pos;
> +					state->result = curr_byte;
>   					return out_pos - out_bin;
>   				}
>   				fragment = base64_decode_value(*in_pos++);
>   			} while (fragment < 0);
> -			*out_pos++ |= (fragment & 0x03c) >> 2;
> +			curr_byte |= (fragment & 0x03c) >> 2;
> +			*out_pos++ = curr_byte;
> +			curr_byte = (fragment & 0x003) << 6;
>   			if (out_pos < out_end)
> -				*out_pos = (fragment & 0x003) << 6;
> +				*out_pos = curr_byte;
>   	case step_d:
>   			do {
>   				if (in_pos == in_end || out_pos >= out_end)
>   				{
>   					state->step = step_d;
> -					state->result = *out_pos;
> +					state->result = curr_byte;
>   					return out_pos - out_bin;
>   				}
>   				fragment = base64_decode_value(*in_pos++);
>   			} while (fragment < 0);
> -			*out_pos++   |= (fragment & 0x03f);
> +			curr_byte |= (fragment & 0x03f);
> +			*out_pos++ = curr_byte;
>   		}
>   	}
>   	/* control should not reach here */
> 

Re: [Tarantool-patches] [PATCH v3 2/2] base64: improve decoder performance

[Leonid Vasiliev]

Hi! Thank you for the patch.
AFAIU the status of the patch is follows:

 > But I see we are not going anywhere here. You don't really need LGTM
 > from me on this patch, if you don't want to finish it. I am not
 > strictly against these changes, because *probably* they don't add new
 > bugs, and seem to be a tiny bit better for perf. I only don't like it
 > being not finished.

I think the changes are ok, because they are good for perf (and we have
confirmation) and don't add degradation (our tests should guarantee
this).

See some comments below:

What about a benchmark. AFAIK A. Lyapunov propose saving all benchmarks
that we used. Did you have a conversation with him?

On 22.12.2020 13:41, Sergey Nikiforov wrote:
> Unnecessary checks were removed from internal loops.
> Benchmark shows that performance is now ~1.19 times higher
> (release build, Intel Core I7-9700K, only one thread).
> ---
> 
> Branch: https://github.com/tarantool/tarantool/tree/void234/gh-3069-fix-base64-memory-overrun-v3
> 
>   test/unit/base64.c      |  7 +++-
>   test/unit/base64.result | 84 +++++++++++++++++++++++++++--------------
>   third_party/base64.c    | 36 +++++++++++++-----
>   3 files changed, 89 insertions(+), 38 deletions(-)
> 

I left my questions about the test in the review of the previous patch.

> diff --git a/test/unit/base64.c b/test/unit/base64.c
> index ada497adf..76db7d782 100644
> --- a/test/unit/base64.c
> +++ b/test/unit/base64.c
> @@ -7,7 +7,7 @@ static void
>   base64_test(const char *str, int options, const char *no_symbols,
>   	    int no_symbols_len)
>   {
> -	plan(3 + no_symbols_len);
> +	plan(4 + no_symbols_len);
>   
>   	int len = strlen(str);
>   	int base64_buflen = base64_bufsize(len + 1, options);
> @@ -34,6 +34,11 @@ base64_test(const char *str, int options, const char *no_symbols,
>   	free(base64_buf);
>   	free(strbuf);
>   
> +	const char *in = "sIIpHw==";
> +	int in_len = strlen(in);
> +	rc = base64_decode(in, in_len, NULL, 0);
> +	is(rc, 0, "no space in out buffer");
> +
>   	check_plan();
>   }
>   
> diff --git a/test/unit/base64.result b/test/unit/base64.result
> index cd1f2b3f6..d606772ea 100644
> --- a/test/unit/base64.result
> +++ b/test/unit/base64.result
> @@ -1,178 +1,206 @@
>   1..28
>   	*** main ***
> -    1..3
> +    1..4
>       ok 1 - length
>       ok 2 - decode length ok
>       ok 3 - encode/decode
> +    ok 4 - no space in out buffer
>   ok 1 - subtests
> -    1..6
> +    1..7
>       ok 1 - length
>       ok 2 - no \n symbols
>       ok 3 - no + symbols
>       ok 4 - no = symbols
>       ok 5 - decode length ok
>       ok 6 - encode/decode
> +    ok 7 - no space in out buffer
>   ok 2 - subtests
> -    1..4
> +    1..5
>       ok 1 - length
>       ok 2 - no = symbols
>       ok 3 - decode length ok
>       ok 4 - encode/decode
> +    ok 5 - no space in out buffer
>   ok 3 - subtests
> -    1..4
> +    1..5
>       ok 1 - length
>       ok 2 - no \n symbols
>       ok 3 - decode length ok
>       ok 4 - encode/decode
> +    ok 5 - no space in out buffer
>   ok 4 - subtests
> -    1..3
> +    1..4
>       ok 1 - length
>       ok 2 - decode length ok
>       ok 3 - encode/decode
> +    ok 4 - no space in out buffer
>   ok 5 - subtests
> -    1..6
> +    1..7
>       ok 1 - length
>       ok 2 - no \n symbols
>       ok 3 - no + symbols
>       ok 4 - no = symbols
>       ok 5 - decode length ok
>       ok 6 - encode/decode
> +    ok 7 - no space in out buffer
>   ok 6 - subtests
> -    1..4
> +    1..5
>       ok 1 - length
>       ok 2 - no = symbols
>       ok 3 - decode length ok
>       ok 4 - encode/decode
> +    ok 5 - no space in out buffer
>   ok 7 - subtests
> -    1..4
> +    1..5
>       ok 1 - length
>       ok 2 - no \n symbols
>       ok 3 - decode length ok
>       ok 4 - encode/decode
> +    ok 5 - no space in out buffer
>   ok 8 - subtests
> -    1..3
> +    1..4
>       ok 1 - length
>       ok 2 - decode length ok
>       ok 3 - encode/decode
> +    ok 4 - no space in out buffer
>   ok 9 - subtests
> -    1..6
> +    1..7
>       ok 1 - length
>       ok 2 - no \n symbols
>       ok 3 - no + symbols
>       ok 4 - no = symbols
>       ok 5 - decode length ok
>       ok 6 - encode/decode
> +    ok 7 - no space in out buffer
>   ok 10 - subtests
> -    1..4
> +    1..5
>       ok 1 - length
>       ok 2 - no = symbols
>       ok 3 - decode length ok
>       ok 4 - encode/decode
> +    ok 5 - no space in out buffer
>   ok 11 - subtests
> -    1..4
> +    1..5
>       ok 1 - length
>       ok 2 - no \n symbols
>       ok 3 - decode length ok
>       ok 4 - encode/decode
> +    ok 5 - no space in out buffer
>   ok 12 - subtests
> -    1..3
> +    1..4
>       ok 1 - length
>       ok 2 - decode length ok
>       ok 3 - encode/decode
> +    ok 4 - no space in out buffer
>   ok 13 - subtests
> -    1..6
> +    1..7
>       ok 1 - length
>       ok 2 - no \n symbols
>       ok 3 - no + symbols
>       ok 4 - no = symbols
>       ok 5 - decode length ok
>       ok 6 - encode/decode
> +    ok 7 - no space in out buffer
>   ok 14 - subtests
> -    1..4
> +    1..5
>       ok 1 - length
>       ok 2 - no = symbols
>       ok 3 - decode length ok
>       ok 4 - encode/decode
> +    ok 5 - no space in out buffer
>   ok 15 - subtests
> -    1..4
> +    1..5
>       ok 1 - length
>       ok 2 - no \n symbols
>       ok 3 - decode length ok
>       ok 4 - encode/decode
> +    ok 5 - no space in out buffer
>   ok 16 - subtests
> -    1..3
> +    1..4
>       ok 1 - length
>       ok 2 - decode length ok
>       ok 3 - encode/decode
> +    ok 4 - no space in out buffer
>   ok 17 - subtests
> -    1..6
> +    1..7
>       ok 1 - length
>       ok 2 - no \n symbols
>       ok 3 - no + symbols
>       ok 4 - no = symbols
>       ok 5 - decode length ok
>       ok 6 - encode/decode
> +    ok 7 - no space in out buffer
>   ok 18 - subtests
> -    1..4
> +    1..5
>       ok 1 - length
>       ok 2 - no = symbols
>       ok 3 - decode length ok
>       ok 4 - encode/decode
> +    ok 5 - no space in out buffer
>   ok 19 - subtests
> -    1..4
> +    1..5
>       ok 1 - length
>       ok 2 - no \n symbols
>       ok 3 - decode length ok
>       ok 4 - encode/decode
> +    ok 5 - no space in out buffer
>   ok 20 - subtests
> -    1..3
> +    1..4
>       ok 1 - length
>       ok 2 - decode length ok
>       ok 3 - encode/decode
> +    ok 4 - no space in out buffer
>   ok 21 - subtests
> -    1..6
> +    1..7
>       ok 1 - length
>       ok 2 - no \n symbols
>       ok 3 - no + symbols
>       ok 4 - no = symbols
>       ok 5 - decode length ok
>       ok 6 - encode/decode
> +    ok 7 - no space in out buffer
>   ok 22 - subtests
> -    1..4
> +    1..5
>       ok 1 - length
>       ok 2 - no = symbols
>       ok 3 - decode length ok
>       ok 4 - encode/decode
> +    ok 5 - no space in out buffer
>   ok 23 - subtests
> -    1..4
> +    1..5
>       ok 1 - length
>       ok 2 - no \n symbols
>       ok 3 - decode length ok
>       ok 4 - encode/decode
> +    ok 5 - no space in out buffer
>   ok 24 - subtests
> -    1..3
> +    1..4
>       ok 1 - length
>       ok 2 - decode length ok
>       ok 3 - encode/decode
> +    ok 4 - no space in out buffer
>   ok 25 - subtests
> -    1..6
> +    1..7
>       ok 1 - length
>       ok 2 - no \n symbols
>       ok 3 - no + symbols
>       ok 4 - no = symbols
>       ok 5 - decode length ok
>       ok 6 - encode/decode
> +    ok 7 - no space in out buffer
>   ok 26 - subtests
> -    1..4
> +    1..5
>       ok 1 - length
>       ok 2 - no = symbols
>       ok 3 - decode length ok
>       ok 4 - encode/decode
> +    ok 5 - no space in out buffer
>   ok 27 - subtests
> -    1..4
> +    1..5
>       ok 1 - length
>       ok 2 - no \n symbols
>       ok 3 - decode length ok
>       ok 4 - encode/decode
> +    ok 5 - no space in out buffer
>   ok 28 - subtests
>   	*** main: done ***
> diff --git a/third_party/base64.c b/third_party/base64.c
> index 3350a98ff..93442c04b 100644
> --- a/third_party/base64.c
> +++ b/third_party/base64.c
> @@ -257,10 +257,11 @@ base64_decode_block(const char *in_base64, int in_len,
>   		{
>   	case step_a:
>   			do {
> -				if (in_pos == in_end || out_pos >= out_end)
> +				if (in_pos >= in_end)
>   				{
>   					state->step = step_a;
> -					state->result = curr_byte;
> +					/* curr_byte is useless now */
> +					/* state->result = curr_byte; */

For multi-line comment, we use the following format:
/*
  * First line
  * Second line
  */

And leaving a commented code is not best practice.

>   					return out_pos - out_bin;
>   				}
>   				fragment = base64_decode_value(*in_pos++);
> @@ -268,7 +269,7 @@ base64_decode_block(const char *in_base64, int in_len,
>   			curr_byte = (fragment & 0x03f) << 2;
>   	case step_b:
>   			do {
> -				if (in_pos == in_end || out_pos >= out_end)
> +				if (in_pos >= in_end)
>   				{
>   					state->step = step_b;
>   					state->result = curr_byte;
> @@ -276,14 +277,19 @@ base64_decode_block(const char *in_base64, int in_len,
>   				}
>   				fragment = base64_decode_value(*in_pos++);
>   			} while (fragment < 0);
> +			if (out_pos >= out_end)
> +			{
> +				/* We are losing some data */

According to 
https://github.com/tarantool/tarantool/wiki/Code-review-procedure :
"Start sentences from a capital letter, end with a dot."
The same for the comments bellow.

> +				state->step = step_b;
> +				state->result = curr_byte;
> +				return out_pos - out_bin;
> +			}
>   			curr_byte |= (fragment & 0x030) >> 4;
>   			*out_pos++ = curr_byte;
>   			curr_byte = (fragment & 0x00f) << 4;
> -			if (out_pos < out_end)
> -				*out_pos = curr_byte;
>   	case step_c:
>   			do {
> -				if (in_pos == in_end || out_pos >= out_end)
> +				if (in_pos >= in_end)
>   				{
>   					state->step = step_c;
>   					state->result = curr_byte;
> @@ -291,14 +297,19 @@ base64_decode_block(const char *in_base64, int in_len,
>   				}
>   				fragment = base64_decode_value(*in_pos++);
>   			} while (fragment < 0);
> +			if (out_pos >= out_end)
> +			{
> +				/* We are losing some data */
> +				state->step = step_c;
> +				state->result = curr_byte;
> +				return out_pos - out_bin;
> +			}
>   			curr_byte |= (fragment & 0x03c) >> 2;
>   			*out_pos++ = curr_byte;
>   			curr_byte = (fragment & 0x003) << 6;
> -			if (out_pos < out_end)
> -				*out_pos = curr_byte;
>   	case step_d:
>   			do {
> -				if (in_pos == in_end || out_pos >= out_end)
> +				if (in_pos >= in_end)
>   				{
>   					state->step = step_d;
>   					state->result = curr_byte;
> @@ -306,6 +317,13 @@ base64_decode_block(const char *in_base64, int in_len,
>   				}
>   				fragment = base64_decode_value(*in_pos++);
>   			} while (fragment < 0);
> +			if (out_pos >= out_end)
> +			{
> +				/* We are losing some data */
> +				state->step = step_d;
> +				state->result = curr_byte;
> +				return out_pos - out_bin;
> +			}
>   			curr_byte |= (fragment & 0x03f);
>   			*out_pos++ = curr_byte;
>   		}
> 

Re: [Tarantool-patches] [PATCH v3 2/2] base64: improve decoder performance

[Sergey Nikiforov]

Hi!

On 24.12.2020 17:08, Leonid Vasiliev wrote:
> Hi! Thank you for the patch.
> AFAIU the status of the patch is follows:
> 
>  > But I see we are not going anywhere here. You don't really need LGTM
>  > from me on this patch, if you don't want to finish it. I am not
>  > strictly against these changes, because *probably* they don't add new
>  > bugs, and seem to be a tiny bit better for perf. I only don't like it
>  > being not finished.
> 
> I think the changes are ok, because they are good for perf (and we have
> confirmation) and don't add degradation (our tests should guarantee
> this).
> 
> See some comments below:
> 
> What about a benchmark. AFAIK A. Lyapunov propose saving all benchmarks
> that we used. Did you have a conversation with him?

Yes, I had. "perf" in tarantool. Benchmark framework is not yet merged 
(https://github.com/tarantool/tarantool/tree/i.kosarev/gh-5385-tiny-tuples-with-perf-test/perf) 
so I plan to commit my microbenchmark a little later to avoid duplication.

> On 22.12.2020 13:41, Sergey Nikiforov wrote:
>> Unnecessary checks were removed from internal loops.
>> Benchmark shows that performance is now ~1.19 times higher
>> (release build, Intel Core I7-9700K, only one thread).
>> ---
>>
>> Branch: 
>> https://github.com/tarantool/tarantool/tree/void234/gh-3069-fix-base64-memory-overrun-v3 
>>
>>
>>   test/unit/base64.c      |  7 +++-
>>   test/unit/base64.result | 84 +++++++++++++++++++++++++++--------------
>>   third_party/base64.c    | 36 +++++++++++++-----
>>   3 files changed, 89 insertions(+), 38 deletions(-)
>>
> 
> I left my questions about the test in the review of the previous patch.

Fixed.

>> diff --git a/third_party/base64.c b/third_party/base64.c
>> index 3350a98ff..93442c04b 100644
>> --- a/third_party/base64.c
>> +++ b/third_party/base64.c
>> @@ -257,10 +257,11 @@ base64_decode_block(const char *in_base64, int 
>> in_len,
>>           {
>>       case step_a:
>>               do {
>> -                if (in_pos == in_end || out_pos >= out_end)
>> +                if (in_pos >= in_end)
>>                   {
>>                       state->step = step_a;
>> -                    state->result = curr_byte;
>> +                    /* curr_byte is useless now */
>> +                    /* state->result = curr_byte; */
> 
> For multi-line comment, we use the following format:
> /*
>   * First line
>   * Second line
>   */
> 
> And leaving a commented code is not best practice.

Fixed.

Re: [Tarantool-patches] [PATCH v3 2/2] base64: improve decoder performance

[Leonid Vasiliev]

Hi!

On 25.12.2020 13:39, Sergey Nikiforov wrote:
> Hi!
> 
> On 24.12.2020 17:08, Leonid Vasiliev wrote:
>> Hi! Thank you for the patch.
>> AFAIU the status of the patch is follows:
>>
>>  > But I see we are not going anywhere here. You don't really need LGTM
>>  > from me on this patch, if you don't want to finish it. I am not
>>  > strictly against these changes, because *probably* they don't add new
>>  > bugs, and seem to be a tiny bit better for perf. I only don't like it
>>  > being not finished.
>>
>> I think the changes are ok, because they are good for perf (and we have
>> confirmation) and don't add degradation (our tests should guarantee
>> this).
>>
>> See some comments below:
>>
>> What about a benchmark. AFAIK A. Lyapunov propose saving all benchmarks
>> that we used. Did you have a conversation with him?
> 
> Yes, I had. "perf" in tarantool. Benchmark framework is not yet merged 
> (https://github.com/tarantool/tarantool/tree/i.kosarev/gh-5385-tiny-tuples-with-perf-test/perf) 
> so I plan to commit my microbenchmark a little later to avoid duplication.
> 

OK.

>> On 22.12.2020 13:41, Sergey Nikiforov wrote:
>>> Unnecessary checks were removed from internal loops.
>>> Benchmark shows that performance is now ~1.19 times higher
>>> (release build, Intel Core I7-9700K, only one thread).
>>> ---
>>>
>>> Branch: 
>>> https://github.com/tarantool/tarantool/tree/void234/gh-3069-fix-base64-memory-overrun-v3 
>>>
>>>
>>>   test/unit/base64.c      |  7 +++-
>>>   test/unit/base64.result | 84 +++++++++++++++++++++++++++--------------
>>>   third_party/base64.c    | 36 +++++++++++++-----
>>>   3 files changed, 89 insertions(+), 38 deletions(-)
>>>
>>
>> I left my questions about the test in the review of the previous patch.
> 
> Fixed.
> 
>>> diff --git a/third_party/base64.c b/third_party/base64.c
>>> index 3350a98ff..93442c04b 100644
>>> --- a/third_party/base64.c
>>> +++ b/third_party/base64.c
>>> @@ -257,10 +257,11 @@ base64_decode_block(const char *in_base64, int 
>>> in_len,
>>>           {
>>>       case step_a:
>>>               do {
>>> -                if (in_pos == in_end || out_pos >= out_end)
>>> +                if (in_pos >= in_end)
>>>                   {
>>>                       state->step = step_a;
>>> -                    state->result = curr_byte;
>>> +                    /* curr_byte is useless now */
>>> +                    /* state->result = curr_byte; */
>>
>> For multi-line comment, we use the following format:
>> /*
>>   * First line
>>   * Second line
>>   */
>>
>> And leaving a commented code is not best practice.
> 
> Fixed.

Re: [Tarantool-patches] [PATCH v3 2/2] base64: improve decoder performance

[Leonid Vasiliev]

[-- Attachment #1: Type: text/plain, Size: 9588 bytes --]


Hi! Thank you for the patch.
AFAIU the status of the patch is follows:

 > But I see we are not going anywhere here. You don't really need LGTM
 > from me on this patch, if you don't want to finish it. I am not
 > strictly against these changes, because *probably* they don't add new
 > bugs, and seem to be a tiny bit better for perf. I only don't like it
 > being not finished.

I think the changes are ok, because they are good for perf (and we have
confirmation) and don't add degradation (our tests should guarantee
this).

See some comments below:

What about a benchmark. AFAIK A. Lyapunov propose saving all benchmarks
that we used. Did you have a conversation with him?

On 22.12.2020 13:41, Sergey Nikiforov wrote:
> Unnecessary checks were removed from internal loops.
> Benchmark shows that performance is now ~1.19 times higher
> (release build, Intel Core I7-9700K, only one thread).
> ---
>
> Branch:  https://github.com/tarantool/tarantool/tree/void234/gh-3069-fix-base64-memory-overrun-v3
>
> test/unit/base64.c | 7 +++-
> test/unit/base64.result | 84 +++++++++++++++++++++++++++--------------
> third_party/base64.c | 36 +++++++++++++-----
> 3 files changed, 89 insertions(+), 38 deletions(-)
>

I left my questions about the test in the review of the previous patch.

> diff --git a/test/unit/base64.c b/test/unit/base64.c
> index ada497adf..76db7d782 100644
> --- a/test/unit/base64.c
> +++ b/test/unit/base64.c
> @@ -7,7 +7,7 @@ static void
> base64_test(const char *str, int options, const char *no_symbols,
> int no_symbols_len)
> {
> - plan(3 + no_symbols_len);
> + plan(4 + no_symbols_len);
>
> int len = strlen(str);
> int base64_buflen = base64_bufsize(len + 1, options);
> @@ -34,6 +34,11 @@ base64_test(const char *str, int options, const char *no_symbols,
> free(base64_buf);
> free(strbuf);
>
> + const char *in = "sIIpHw==";
> + int in_len = strlen(in);
> + rc = base64_decode(in, in_len, NULL, 0);
> + is(rc, 0, "no space in out buffer");
> +
> check_plan();
> }
>
> diff --git a/test/unit/base64.result b/test/unit/base64.result
> index cd1f2b3f6..d606772ea 100644
> --- a/test/unit/base64.result
> +++ b/test/unit/base64.result
> @@ -1,178 +1,206 @@
> 1..28
> *** main ***
> - 1..3
> + 1..4
> ok 1 - length
> ok 2 - decode length ok
> ok 3 - encode/decode
> + ok 4 - no space in out buffer
> ok 1 - subtests
> - 1..6
> + 1..7
> ok 1 - length
> ok 2 - no \n symbols
> ok 3 - no + symbols
> ok 4 - no = symbols
> ok 5 - decode length ok
> ok 6 - encode/decode
> + ok 7 - no space in out buffer
> ok 2 - subtests
> - 1..4
> + 1..5
> ok 1 - length
> ok 2 - no = symbols
> ok 3 - decode length ok
> ok 4 - encode/decode
> + ok 5 - no space in out buffer
> ok 3 - subtests
> - 1..4
> + 1..5
> ok 1 - length
> ok 2 - no \n symbols
> ok 3 - decode length ok
> ok 4 - encode/decode
> + ok 5 - no space in out buffer
> ok 4 - subtests
> - 1..3
> + 1..4
> ok 1 - length
> ok 2 - decode length ok
> ok 3 - encode/decode
> + ok 4 - no space in out buffer
> ok 5 - subtests
> - 1..6
> + 1..7
> ok 1 - length
> ok 2 - no \n symbols
> ok 3 - no + symbols
> ok 4 - no = symbols
> ok 5 - decode length ok
> ok 6 - encode/decode
> + ok 7 - no space in out buffer
> ok 6 - subtests
> - 1..4
> + 1..5
> ok 1 - length
> ok 2 - no = symbols
> ok 3 - decode length ok
> ok 4 - encode/decode
> + ok 5 - no space in out buffer
> ok 7 - subtests
> - 1..4
> + 1..5
> ok 1 - length
> ok 2 - no \n symbols
> ok 3 - decode length ok
> ok 4 - encode/decode
> + ok 5 - no space in out buffer
> ok 8 - subtests
> - 1..3
> + 1..4
> ok 1 - length
> ok 2 - decode length ok
> ok 3 - encode/decode
> + ok 4 - no space in out buffer
> ok 9 - subtests
> - 1..6
> + 1..7
> ok 1 - length
> ok 2 - no \n symbols
> ok 3 - no + symbols
> ok 4 - no = symbols
> ok 5 - decode length ok
> ok 6 - encode/decode
> + ok 7 - no space in out buffer
> ok 10 - subtests
> - 1..4
> + 1..5
> ok 1 - length
> ok 2 - no = symbols
> ok 3 - decode length ok
> ok 4 - encode/decode
> + ok 5 - no space in out buffer
> ok 11 - subtests
> - 1..4
> + 1..5
> ok 1 - length
> ok 2 - no \n symbols
> ok 3 - decode length ok
> ok 4 - encode/decode
> + ok 5 - no space in out buffer
> ok 12 - subtests
> - 1..3
> + 1..4
> ok 1 - length
> ok 2 - decode length ok
> ok 3 - encode/decode
> + ok 4 - no space in out buffer
> ok 13 - subtests
> - 1..6
> + 1..7
> ok 1 - length
> ok 2 - no \n symbols
> ok 3 - no + symbols
> ok 4 - no = symbols
> ok 5 - decode length ok
> ok 6 - encode/decode
> + ok 7 - no space in out buffer
> ok 14 - subtests
> - 1..4
> + 1..5
> ok 1 - length
> ok 2 - no = symbols
> ok 3 - decode length ok
> ok 4 - encode/decode
> + ok 5 - no space in out buffer
> ok 15 - subtests
> - 1..4
> + 1..5
> ok 1 - length
> ok 2 - no \n symbols
> ok 3 - decode length ok
> ok 4 - encode/decode
> + ok 5 - no space in out buffer
> ok 16 - subtests
> - 1..3
> + 1..4
> ok 1 - length
> ok 2 - decode length ok
> ok 3 - encode/decode
> + ok 4 - no space in out buffer
> ok 17 - subtests
> - 1..6
> + 1..7
> ok 1 - length
> ok 2 - no \n symbols
> ok 3 - no + symbols
> ok 4 - no = symbols
> ok 5 - decode length ok
> ok 6 - encode/decode
> + ok 7 - no space in out buffer
> ok 18 - subtests
> - 1..4
> + 1..5
> ok 1 - length
> ok 2 - no = symbols
> ok 3 - decode length ok
> ok 4 - encode/decode
> + ok 5 - no space in out buffer
> ok 19 - subtests
> - 1..4
> + 1..5
> ok 1 - length
> ok 2 - no \n symbols
> ok 3 - decode length ok
> ok 4 - encode/decode
> + ok 5 - no space in out buffer
> ok 20 - subtests
> - 1..3
> + 1..4
> ok 1 - length
> ok 2 - decode length ok
> ok 3 - encode/decode
> + ok 4 - no space in out buffer
> ok 21 - subtests
> - 1..6
> + 1..7
> ok 1 - length
> ok 2 - no \n symbols
> ok 3 - no + symbols
> ok 4 - no = symbols
> ok 5 - decode length ok
> ok 6 - encode/decode
> + ok 7 - no space in out buffer
> ok 22 - subtests
> - 1..4
> + 1..5
> ok 1 - length
> ok 2 - no = symbols
> ok 3 - decode length ok
> ok 4 - encode/decode
> + ok 5 - no space in out buffer
> ok 23 - subtests
> - 1..4
> + 1..5
> ok 1 - length
> ok 2 - no \n symbols
> ok 3 - decode length ok
> ok 4 - encode/decode
> + ok 5 - no space in out buffer
> ok 24 - subtests
> - 1..3
> + 1..4
> ok 1 - length
> ok 2 - decode length ok
> ok 3 - encode/decode
> + ok 4 - no space in out buffer
> ok 25 - subtests
> - 1..6
> + 1..7
> ok 1 - length
> ok 2 - no \n symbols
> ok 3 - no + symbols
> ok 4 - no = symbols
> ok 5 - decode length ok
> ok 6 - encode/decode
> + ok 7 - no space in out buffer
> ok 26 - subtests
> - 1..4
> + 1..5
> ok 1 - length
> ok 2 - no = symbols
> ok 3 - decode length ok
> ok 4 - encode/decode
> + ok 5 - no space in out buffer
> ok 27 - subtests
> - 1..4
> + 1..5
> ok 1 - length
> ok 2 - no \n symbols
> ok 3 - decode length ok
> ok 4 - encode/decode
> + ok 5 - no space in out buffer
> ok 28 - subtests
> *** main: done ***
> diff --git a/third_party/base64.c b/third_party/base64.c
> index 3350a98ff..93442c04b 100644
> --- a/third_party/base64.c
> +++ b/third_party/base64.c
> @@ -257,10 +257,11 @@ base64_decode_block(const char *in_base64, int in_len,
> {
> case step_a:
> do {
> - if (in_pos == in_end || out_pos >= out_end)
> + if (in_pos >= in_end)
> {
> state->step = step_a;
> - state->result = curr_byte;
> + /* curr_byte is useless now */
> + /* state->result = curr_byte; */

For multi-line comment, we use the following format:
/*
  * First line
  * Second line
  */

And leaving a commented code is not best practice.

> return out_pos - out_bin;
> }
> fragment = base64_decode_value(*in_pos++);
> @@ -268,7 +269,7 @@ base64_decode_block(const char *in_base64, int in_len,
> curr_byte = (fragment & 0x03f) << 2;
> case step_b:
> do {
> - if (in_pos == in_end || out_pos >= out_end)
> + if (in_pos >= in_end)
> {
> state->step = step_b;
> state->result = curr_byte;
> @@ -276,14 +277,19 @@ base64_decode_block(const char *in_base64, int in_len,
> }
> fragment = base64_decode_value(*in_pos++);
> } while (fragment < 0);
> + if (out_pos >= out_end)
> + {
> + /* We are losing some data */

According to
https://github.com/tarantool/tarantool/wiki/Code-review-procedure :
"Start sentences from a capital letter, end with a dot."
The same for the comments bellow.

> + state->step = step_b;
> + state->result = curr_byte;
> + return out_pos - out_bin;
> + }
> curr_byte |= (fragment & 0x030) >> 4;
> *out_pos++ = curr_byte;
> curr_byte = (fragment & 0x00f) << 4;
> - if (out_pos < out_end)
> - *out_pos = curr_byte;
> case step_c:
> do {
> - if (in_pos == in_end || out_pos >= out_end)
> + if (in_pos >= in_end)
> {
> state->step = step_c;
> state->result = curr_byte;
> @@ -291,14 +297,19 @@ base64_decode_block(const char *in_base64, int in_len,
> }
> fragment = base64_decode_value(*in_pos++);
> } while (fragment < 0);
> + if (out_pos >= out_end)
> + {
> + /* We are losing some data */
> + state->step = step_c;
> + state->result = curr_byte;
> + return out_pos - out_bin;
> + }
> curr_byte |= (fragment & 0x03c) >> 2;
> *out_pos++ = curr_byte;
> curr_byte = (fragment & 0x003) << 6;
> - if (out_pos < out_end)
> - *out_pos = curr_byte;
> case step_d:
> do {
> - if (in_pos == in_end || out_pos >= out_end)
> + if (in_pos >= in_end)
> {
> state->step = step_d;
> state->result = curr_byte;
> @@ -306,6 +317,13 @@ base64_decode_block(const char *in_base64, int in_len,
> }
> fragment = base64_decode_value(*in_pos++);
> } while (fragment < 0);
> + if (out_pos >= out_end)
> + {
> + /* We are losing some data */
> + state->step = step_d;
> + state->result = curr_byte;
> + return out_pos - out_bin;
> + }
> curr_byte |= (fragment & 0x03f);
> *out_pos++ = curr_byte;
> }
>
 

[-- Attachment #2: Type: text/html, Size: 12444 bytes --]