Tarantool development patches archive
 help / color / mirror / Atom feed
From: Sergey Nikiforov via Tarantool-patches <tarantool-patches@dev.tarantool.org>
To: Alexander Turenko <alexander.turenko@tarantool.org>
Cc: tarantool-patches@dev.tarantool.org,
	Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
Subject: Re: [Tarantool-patches] [PATCH v6 1/2] base64: fix decoder output buffer overrun (reads)
Date: Tue, 26 Jan 2021 19:37:00 +0300	[thread overview]
Message-ID: <a015ce9d-5c46-a3f6-0fa0-b776b01d82b3@tarantool.org> (raw)
In-Reply-To: <20210121021627.2dzyh7fho2cvjlrz@tkn_work_nb>

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

  reply	other threads:[~2021-01-26 16:37 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-01-11  9:44 [Tarantool-patches] [PATCH v6 0/2] base64: Fix decoder, improve its performance Sergey Nikiforov via Tarantool-patches
2021-01-11  9:45 ` [Tarantool-patches] [PATCH v6 1/2] base64: fix decoder output buffer overrun (reads) Sergey Nikiforov via Tarantool-patches
2021-01-21  2:16   ` Alexander Turenko via Tarantool-patches
2021-01-26 16:37     ` Sergey Nikiforov via Tarantool-patches [this message]
2021-01-26 16:48       ` Sergey Nikiforov via Tarantool-patches
2021-02-20 11:30         ` Alexander Turenko via Tarantool-patches
2021-02-20 12:49       ` Alexander Turenko via Tarantool-patches
2021-02-25  8:25         ` Sergey Nikiforov via Tarantool-patches
2021-02-27 18:47           ` Alexander Turenko via Tarantool-patches
2021-01-11  9:45 ` [Tarantool-patches] [PATCH v6 2/2] base64: improve decoder performance Sergey Nikiforov via Tarantool-patches
2021-01-21 15:31   ` Alexander Turenko via Tarantool-patches
2021-01-26 16:37     ` Sergey Nikiforov via Tarantool-patches
2021-02-20 12:51       ` Alexander Turenko via Tarantool-patches
2021-01-12 19:39 ` [Tarantool-patches] [PATCH v6 0/2] base64: Fix decoder, improve its performance Alexander Turenko via Tarantool-patches

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=a015ce9d-5c46-a3f6-0fa0-b776b01d82b3@tarantool.org \
    --to=tarantool-patches@dev.tarantool.org \
    --cc=alexander.turenko@tarantool.org \
    --cc=v.shpilevoy@tarantool.org \
    --cc=void@tarantool.org \
    --subject='Re: [Tarantool-patches] [PATCH v6 1/2] base64: fix decoder output buffer overrun (reads)' \


* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox