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

[Sergey Nikiforov via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

On 21.01.2021 5:16, Alexander Turenko wrote:
> On Mon, Jan 11, 2021 at 12:45:00PM +0300, Sergey Nikiforov wrote:
>> Was caught by base64 test with enabled ASAN.
> 
> It seems, we have a problem in CI, otherwise it would be detected. At
> least, I don't see an explicit suppression relevant to the base64 code
> or disabling the test under this CI job.
> 
> Can you, please, investigate, how to enable it in CI, so we'll catch the
> similar problem next time if it'll appear?

ASAN is not used in CI now. Which is clearly wrong.
Right now a lot of tests fail if LeakSanitizer is enabled (the default 
for ASAN), but only 1 test (unit/guard.test) fails if LeakSanitizer if 
disabled. So it is quite straightforward:

CC=clang CXX=clang++ cmake . -DENABLE_ASAN=ON && make -j
ASAN_OPTIONS=detect_leaks=0 test/test-run.py

(test-run.py is launched from several Makefiles)

I propose creating tasks to make unit/guard.test "ASAN-tolerant" (ASAN 
prints warning which causes .result mismatch) and to add ASAN targets to 
CI. Should it be GitLab or GitHub Actions?

We should probably also look on LeakSanitizer issues, some of them are 
probably real bugs and not just tests sloppiness.

>>
>> It also caused data corruption - garbage instead of "extra bits" was
>> saved into state->result if there was no space in output buffer.
> 
> We have the dead code and it appears to be broken. Why don't remove it?
> (AFAIS, the rest of the code does not read off the buffer.)
> 
> Is it due to a little probability that we'll decide to support chunked
> decoding and we'll decide to implement it in exactly this way (and not
> just leaving undecoded bytes in, say, ibuf)?
> 
> Another side of this little probability is:
> 
> * The code complexity and so waste of the time for anyone who need to
>    dive into it.
> * Have untested code in the code base that may give us more surprises.
> * Extra def-use dependencies may hide optimization opportunities and
>    increase register pressure.

And yet you are youself proposing to improve performance:
"entirely eliminate the output buffer lengthchecks for the first 
(out_len * 3 / 4) input bytes" (quoted from your e-mail about 2/2). This 
means saving state and reporting input buffer stop position. So: do we 
want complexity (and performance) or simplicity?

> This is not the question regarding the patch, but this code looks broken
> in several other ways. At least:
> 
> * It skips unrecognized symbols and does not report a decoding error.

Some of these symbols should "legally" be skipped - like newlines in 
e-mail use case. And there are probably other use cases which use some 
other symbols. We could break something and gain nothing.

Nothing prevents somebody from corrupting "legal" base64 characters, 
this is not detected nor should be. These issues are outside base64 
decoder scope, CRCs or digital signatures should be used when data can 
be accidentally or intentionally corrupted.

> * If the output buffer is too short, it neither report an error, nor a
>    required buffer length (like snprintf()). No way to distinguish a
>    successful and an 'interrupted' processing.

Here I agree.
There was no output buffer length checks in code imported into 
Tarantool. That is clearly stopgap to prevent buffer overrun.

Summary: I propose commiting this particular patch (1/2) "as is" (it was 
posted unmodified several times already) and discussing performance 
patch (2/2) a little further.

>>
>> Added test for "zero-sized output buffer" case.
> 
> Nice catch.
> 
>>
>> Fixes: #3069
>> ---
>>
>> Branch: https://github.com/tarantool/tarantool/tree/void234/gh-3069-fix-base64-memory-overrun-v6
>> Issue: https://github.com/tarantool/tarantool/issues/3069