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

[Leonid Vasiliev <lvasiliev@tarantool.org>]

Hi! Thank you for the patch.
LGTM.
See 1 comment below:

On 25.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.
> 
> Added test for "zero-sized output buffer" case.
> 
> Fixes: #3069
> ---
>   test/unit/base64.c      | 13 ++++++++++++-
>   test/unit/base64.result |  3 ++-
>   third_party/base64.c    | 28 +++++++++++++++++-----------
>   3 files changed, 31 insertions(+), 13 deletions(-)
> 
> diff --git a/test/unit/base64.c b/test/unit/base64.c
> index ada497adf..c0ccf1321 100644
> --- a/test/unit/base64.c
> +++ b/test/unit/base64.c
> @@ -58,9 +58,18 @@ base64_nowrap_test(const char *str)
>   	base64_test(str, BASE64_NOWRAP, symbols, lengthof(symbols));
>   }
>   
> +static void
> +base64_no_space_test(void)
> +{

Add `plan (1)` to the beginning of the function and `check_plan ()` to
the end to treat it as a separate subtest.

> +	const char *const in = "sIIpHw==";
> +	const int in_len = strlen(in);
> +	const int rc = base64_decode(in, in_len, NULL, 0);
> +	is(rc, 0, "no space in out buffer");
> +}
> +
>   int main(int argc, char *argv[])
>   {
> -	plan(28);
> +	plan(29);
>   	header();
>   
>   	const char *option_tests[] = {
> @@ -78,6 +87,8 @@ int main(int argc, char *argv[])
>   		base64_nowrap_test(option_tests[i]);
>   	}
>   
> +	base64_no_space_test();
> +
>   	footer();
>   	return check_plan();
>   }
> diff --git a/test/unit/base64.result b/test/unit/base64.result
> index cd1f2b3f6..71b3519bf 100644
> --- a/test/unit/base64.result
> +++ b/test/unit/base64.result
> @@ -1,4 +1,4 @@
> -1..28
> +1..29
>   	*** main ***
>       1..3
>       ok 1 - length
> @@ -175,4 +175,5 @@ ok 27 - subtests
>       ok 3 - decode length ok
>       ok 4 - encode/decode
>   ok 28 - subtests
> +ok 29 - no space in out buffer
>   	*** main: done ***
> 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 */
>