From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng1.m.smailru.net (smtpng1.m.smailru.net [94.100.181.251]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id E063245C304 for ; Thu, 17 Dec 2020 02:22:10 +0300 (MSK) References: From: Vladislav Shpilevoy Message-ID: <33050dea-c4b8-44cf-02fb-67928d5f8a93@tarantool.org> Date: Thu, 17 Dec 2020 00:22:08 +0100 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [Tarantool-patches] [PATCH v2 2/2] base64: Improve decoder performance List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Sergey Nikiforov , tarantool-patches@dev.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?