<HTML><BODY><div class="js-helper js-readmsg-msg"><div><div id="style_16088188691709174521_BODY">Hi! Thank you for the patch.<br>AFAIU the status of the patch is follows:<br><br> > But I see we are not going anywhere here. You don't really need LGTM<br> > from me on this patch, if you don't want to finish it. I am not<br> > strictly against these changes, because *probably* they don't add new<br> > bugs, and seem to be a tiny bit better for perf. I only don't like it<br> > being not finished.<br><br>I think the changes are ok, because they are good for perf (and we have<br>confirmation) and don't add degradation (our tests should guarantee<br>this).<br><br>See some comments below:<br><br>What about a benchmark. AFAIK A. Lyapunov propose saving all benchmarks<br>that we used. Did you have a conversation with him?<br><br>On 22.12.2020 13:41, Sergey Nikiforov wrote:<br>> Unnecessary checks were removed from internal loops.<br>> Benchmark shows that performance is now ~1.19 times higher<br>> (release build, Intel Core I7-9700K, only one thread).<br>> ---<br>><br>> Branch: <a href="https://github.com/tarantool/tarantool/tree/void234/gh-3069-fix-base64-memory-overrun-v3" target="_blank">https://github.com/tarantool/tarantool/tree/void234/gh-3069-fix-base64-memory-overrun-v3</a><br>><br>> test/unit/base64.c | 7 +++-<br>> test/unit/base64.result | 84 +++++++++++++++++++++++++++--------------<br>> third_party/base64.c | 36 +++++++++++++-----<br>> 3 files changed, 89 insertions(+), 38 deletions(-)<br>><br><br>I left my questions about the test in the review of the previous patch.<br><br>> diff --git a/test/unit/base64.c b/test/unit/base64.c<br>> index ada497adf..76db7d782 100644<br>> --- a/test/unit/base64.c<br>> +++ b/test/unit/base64.c<br>> @@ -7,7 +7,7 @@ static void<br>> base64_test(const char *str, int options, const char *no_symbols,<br>> int no_symbols_len)<br>> {<br>> - plan(3 + no_symbols_len);<br>> + plan(4 + no_symbols_len);<br>><br>> int len = strlen(str);<br>> int base64_buflen = base64_bufsize(len + 1, options);<br>> @@ -34,6 +34,11 @@ base64_test(const char *str, int options, const char *no_symbols,<br>> free(base64_buf);<br>> free(strbuf);<br>><br>> + const char *in = "sIIpHw==";<br>> + int in_len = strlen(in);<br>> + rc = base64_decode(in, in_len, NULL, 0);<br>> + is(rc, 0, "no space in out buffer");<br>> +<br>> check_plan();<br>> }<br>><br>> diff --git a/test/unit/base64.result b/test/unit/base64.result<br>> index cd1f2b3f6..d606772ea 100644<br>> --- a/test/unit/base64.result<br>> +++ b/test/unit/base64.result<br>> @@ -1,178 +1,206 @@<br>> 1..28<br>> *** main ***<br>> - 1..3<br>> + 1..4<br>> ok 1 - length<br>> ok 2 - decode length ok<br>> ok 3 - encode/decode<br>> + ok 4 - no space in out buffer<br>> ok 1 - subtests<br>> - 1..6<br>> + 1..7<br>> ok 1 - length<br>> ok 2 - no \n symbols<br>> ok 3 - no + symbols<br>> ok 4 - no = symbols<br>> ok 5 - decode length ok<br>> ok 6 - encode/decode<br>> + ok 7 - no space in out buffer<br>> ok 2 - subtests<br>> - 1..4<br>> + 1..5<br>> ok 1 - length<br>> ok 2 - no = symbols<br>> ok 3 - decode length ok<br>> ok 4 - encode/decode<br>> + ok 5 - no space in out buffer<br>> ok 3 - subtests<br>> - 1..4<br>> + 1..5<br>> ok 1 - length<br>> ok 2 - no \n symbols<br>> ok 3 - decode length ok<br>> ok 4 - encode/decode<br>> + ok 5 - no space in out buffer<br>> ok 4 - subtests<br>> - 1..3<br>> + 1..4<br>> ok 1 - length<br>> ok 2 - decode length ok<br>> ok 3 - encode/decode<br>> + ok 4 - no space in out buffer<br>> ok 5 - subtests<br>> - 1..6<br>> + 1..7<br>> ok 1 - length<br>> ok 2 - no \n symbols<br>> ok 3 - no + symbols<br>> ok 4 - no = symbols<br>> ok 5 - decode length ok<br>> ok 6 - encode/decode<br>> + ok 7 - no space in out buffer<br>> ok 6 - subtests<br>> - 1..4<br>> + 1..5<br>> ok 1 - length<br>> ok 2 - no = symbols<br>> ok 3 - decode length ok<br>> ok 4 - encode/decode<br>> + ok 5 - no space in out buffer<br>> ok 7 - subtests<br>> - 1..4<br>> + 1..5<br>> ok 1 - length<br>> ok 2 - no \n symbols<br>> ok 3 - decode length ok<br>> ok 4 - encode/decode<br>> + ok 5 - no space in out buffer<br>> ok 8 - subtests<br>> - 1..3<br>> + 1..4<br>> ok 1 - length<br>> ok 2 - decode length ok<br>> ok 3 - encode/decode<br>> + ok 4 - no space in out buffer<br>> ok 9 - subtests<br>> - 1..6<br>> + 1..7<br>> ok 1 - length<br>> ok 2 - no \n symbols<br>> ok 3 - no + symbols<br>> ok 4 - no = symbols<br>> ok 5 - decode length ok<br>> ok 6 - encode/decode<br>> + ok 7 - no space in out buffer<br>> ok 10 - subtests<br>> - 1..4<br>> + 1..5<br>> ok 1 - length<br>> ok 2 - no = symbols<br>> ok 3 - decode length ok<br>> ok 4 - encode/decode<br>> + ok 5 - no space in out buffer<br>> ok 11 - subtests<br>> - 1..4<br>> + 1..5<br>> ok 1 - length<br>> ok 2 - no \n symbols<br>> ok 3 - decode length ok<br>> ok 4 - encode/decode<br>> + ok 5 - no space in out buffer<br>> ok 12 - subtests<br>> - 1..3<br>> + 1..4<br>> ok 1 - length<br>> ok 2 - decode length ok<br>> ok 3 - encode/decode<br>> + ok 4 - no space in out buffer<br>> ok 13 - subtests<br>> - 1..6<br>> + 1..7<br>> ok 1 - length<br>> ok 2 - no \n symbols<br>> ok 3 - no + symbols<br>> ok 4 - no = symbols<br>> ok 5 - decode length ok<br>> ok 6 - encode/decode<br>> + ok 7 - no space in out buffer<br>> ok 14 - subtests<br>> - 1..4<br>> + 1..5<br>> ok 1 - length<br>> ok 2 - no = symbols<br>> ok 3 - decode length ok<br>> ok 4 - encode/decode<br>> + ok 5 - no space in out buffer<br>> ok 15 - subtests<br>> - 1..4<br>> + 1..5<br>> ok 1 - length<br>> ok 2 - no \n symbols<br>> ok 3 - decode length ok<br>> ok 4 - encode/decode<br>> + ok 5 - no space in out buffer<br>> ok 16 - subtests<br>> - 1..3<br>> + 1..4<br>> ok 1 - length<br>> ok 2 - decode length ok<br>> ok 3 - encode/decode<br>> + ok 4 - no space in out buffer<br>> ok 17 - subtests<br>> - 1..6<br>> + 1..7<br>> ok 1 - length<br>> ok 2 - no \n symbols<br>> ok 3 - no + symbols<br>> ok 4 - no = symbols<br>> ok 5 - decode length ok<br>> ok 6 - encode/decode<br>> + ok 7 - no space in out buffer<br>> ok 18 - subtests<br>> - 1..4<br>> + 1..5<br>> ok 1 - length<br>> ok 2 - no = symbols<br>> ok 3 - decode length ok<br>> ok 4 - encode/decode<br>> + ok 5 - no space in out buffer<br>> ok 19 - subtests<br>> - 1..4<br>> + 1..5<br>> ok 1 - length<br>> ok 2 - no \n symbols<br>> ok 3 - decode length ok<br>> ok 4 - encode/decode<br>> + ok 5 - no space in out buffer<br>> ok 20 - subtests<br>> - 1..3<br>> + 1..4<br>> ok 1 - length<br>> ok 2 - decode length ok<br>> ok 3 - encode/decode<br>> + ok 4 - no space in out buffer<br>> ok 21 - subtests<br>> - 1..6<br>> + 1..7<br>> ok 1 - length<br>> ok 2 - no \n symbols<br>> ok 3 - no + symbols<br>> ok 4 - no = symbols<br>> ok 5 - decode length ok<br>> ok 6 - encode/decode<br>> + ok 7 - no space in out buffer<br>> ok 22 - subtests<br>> - 1..4<br>> + 1..5<br>> ok 1 - length<br>> ok 2 - no = symbols<br>> ok 3 - decode length ok<br>> ok 4 - encode/decode<br>> + ok 5 - no space in out buffer<br>> ok 23 - subtests<br>> - 1..4<br>> + 1..5<br>> ok 1 - length<br>> ok 2 - no \n symbols<br>> ok 3 - decode length ok<br>> ok 4 - encode/decode<br>> + ok 5 - no space in out buffer<br>> ok 24 - subtests<br>> - 1..3<br>> + 1..4<br>> ok 1 - length<br>> ok 2 - decode length ok<br>> ok 3 - encode/decode<br>> + ok 4 - no space in out buffer<br>> ok 25 - subtests<br>> - 1..6<br>> + 1..7<br>> ok 1 - length<br>> ok 2 - no \n symbols<br>> ok 3 - no + symbols<br>> ok 4 - no = symbols<br>> ok 5 - decode length ok<br>> ok 6 - encode/decode<br>> + ok 7 - no space in out buffer<br>> ok 26 - subtests<br>> - 1..4<br>> + 1..5<br>> ok 1 - length<br>> ok 2 - no = symbols<br>> ok 3 - decode length ok<br>> ok 4 - encode/decode<br>> + ok 5 - no space in out buffer<br>> ok 27 - subtests<br>> - 1..4<br>> + 1..5<br>> ok 1 - length<br>> ok 2 - no \n symbols<br>> ok 3 - decode length ok<br>> ok 4 - encode/decode<br>> + ok 5 - no space in out buffer<br>> ok 28 - subtests<br>> *** main: done ***<br>> diff --git a/third_party/base64.c b/third_party/base64.c<br>> index 3350a98ff..93442c04b 100644<br>> --- a/third_party/base64.c<br>> +++ b/third_party/base64.c<br>> @@ -257,10 +257,11 @@ base64_decode_block(const char *in_base64, int in_len,<br>> {<br>> case step_a:<br>> do {<br>> - if (in_pos == in_end || out_pos >= out_end)<br>> + if (in_pos >= in_end)<br>> {<br>> state->step = step_a;<br>> - state->result = curr_byte;<br>> + /* curr_byte is useless now */<br>> + /* state->result = curr_byte; */<br><br>For multi-line comment, we use the following format:<br>/*<br> * First line<br> * Second line<br> */<br><br>And leaving a commented code is not best practice.<br><br>> return out_pos - out_bin;<br>> }<br>> fragment = base64_decode_value(*in_pos++);<br>> @@ -268,7 +269,7 @@ base64_decode_block(const char *in_base64, int in_len,<br>> curr_byte = (fragment & 0x03f) << 2;<br>> case step_b:<br>> do {<br>> - if (in_pos == in_end || out_pos >= out_end)<br>> + if (in_pos >= in_end)<br>> {<br>> state->step = step_b;<br>> state->result = curr_byte;<br>> @@ -276,14 +277,19 @@ base64_decode_block(const char *in_base64, int in_len,<br>> }<br>> fragment = base64_decode_value(*in_pos++);<br>> } while (fragment < 0);<br>> + if (out_pos >= out_end)<br>> + {<br>> + /* We are losing some data */<br><br>According to<br><a href="https://github.com/tarantool/tarantool/wiki/Code-review-procedure" target="_blank">https://github.com/tarantool/tarantool/wiki/Code-review-procedure</a> :<br>"Start sentences from a capital letter, end with a dot."<br>The same for the comments bellow.<br><br>> + state->step = step_b;<br>> + state->result = curr_byte;<br>> + return out_pos - out_bin;<br>> + }<br>> curr_byte |= (fragment & 0x030) >> 4;<br>> *out_pos++ = curr_byte;<br>> curr_byte = (fragment & 0x00f) << 4;<br>> - if (out_pos < out_end)<br>> - *out_pos = curr_byte;<br>> case step_c:<br>> do {<br>> - if (in_pos == in_end || out_pos >= out_end)<br>> + if (in_pos >= in_end)<br>> {<br>> state->step = step_c;<br>> state->result = curr_byte;<br>> @@ -291,14 +297,19 @@ base64_decode_block(const char *in_base64, int in_len,<br>> }<br>> fragment = base64_decode_value(*in_pos++);<br>> } while (fragment < 0);<br>> + if (out_pos >= out_end)<br>> + {<br>> + /* We are losing some data */<br>> + state->step = step_c;<br>> + state->result = curr_byte;<br>> + return out_pos - out_bin;<br>> + }<br>> curr_byte |= (fragment & 0x03c) >> 2;<br>> *out_pos++ = curr_byte;<br>> curr_byte = (fragment & 0x003) << 6;<br>> - if (out_pos < out_end)<br>> - *out_pos = curr_byte;<br>> case step_d:<br>> do {<br>> - if (in_pos == in_end || out_pos >= out_end)<br>> + if (in_pos >= in_end)<br>> {<br>> state->step = step_d;<br>> state->result = curr_byte;<br>> @@ -306,6 +317,13 @@ base64_decode_block(const char *in_base64, int in_len,<br>> }<br>> fragment = base64_decode_value(*in_pos++);<br>> } while (fragment < 0);<br>> + if (out_pos >= out_end)<br>> + {<br>> + /* We are losing some data */<br>> + state->step = step_d;<br>> + state->result = curr_byte;<br>> + return out_pos - out_bin;<br>> + }<br>> curr_byte |= (fragment & 0x03f);<br>> *out_pos++ = curr_byte;<br>> }<br>></div></div></div><div> </div></BODY></HTML>