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

[Leonid Vasiliev <lvasiliev@tarantool.org>]

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 */
>