Re: [Tarantool-patches] [PATCH v2 2/2] base64: Improve decoder performance

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

Hi! Thanks for the patch!

On 15.12.2020 15:22, Sergey Nikiforov wrote:
> Unnecessary checks were removed from internal loops.
> Benchmark shows that performance is now ~1.15 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-v2
> 
>  third_party/base64.c | 36 +++++++++++++++++++++++++-----------
>  1 file changed, 25 insertions(+), 11 deletions(-)
> 
> diff --git a/third_party/base64.c b/third_party/base64.c
> index 3350a98ff..f4fbbf477 100644
> --- a/third_party/base64.c
> +++ b/third_party/base64.c
> @@ -257,7 +257,7 @@ 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)

This test now crashes:

====================
--- 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();
 }
====================

It didn't crash while the checks were in place. I would suggest to
add this test to the previous commit. Because technically it is
also 'buffer overrun'. And it will crash 3069 bug even without ASAN.

For this patch you may want to add a zero length in the beginning.
Also we can keep the number of 'if's unchanged like this:

====================
--- a/third_party/base64.c
+++ b/third_party/base64.c
@@ -256,6 +256,12 @@ base64_decode_block(const char *in_base64, int in_len,
 		while (1)
 		{
 	case step_a:
+			if (out_pos >= out_end)
+			{
+				state->step = step_a;
+				state->result = curr_byte;
+				return out_pos - out_bin;
+			}
 			do {
 				if (in_pos >= in_end)
 				{
@@ -316,12 +322,7 @@ base64_decode_block(const char *in_base64, int in_len,
 			} while (fragment < 0);
 			curr_byte |= (fragment & 0x03f);
 			*out_pos = curr_byte;
-			if (++out_pos >= out_end)
-			{
-				state->step = step_a;
-				state->result = curr_byte;
-				return out_pos - out_bin;
-			}
+			++out_pos;
 		}
 	}
 	/* control should not reach here */
====================

Will that work?