FIX: out-of-bounds access of std::array

[reneme]

This adds a check to skip filling an AlignmentBuffer<> with zeros when it is already full (i.e. ready_to_consume()). Many thanks to @guidovranken for spotting this just before the 3.2.0 release and the detailed write-up.

From what I can see, this wouldn't have resulting in an actual memory bug, though. Albeit the pointer passed to clear_mem being bogus, it would have been ignored due to the size being zero. Nevertheless, such things are accidents waiting to happen. This will benefit from another look after the release (and once #3715 is merged).

@guidovranken I'd be glad if you could confirm that the patch actually fixes this.

Closes #3734.

[guidovranken]

Testing now.

[guidovranken]

Confirmed that this patch resolves the issue.

[randombit]

I'm a little put off that somehow nothing in the test suite caught this before merge, even though we do run CI with ASan on 3 different compilers. I'm not sure why that is.

Anyway patch looks good, thanks @reneme, and of course @guidovranken for CryptoFuzz

[reneme]

ASan doesn't pick this up because there was no out-of-bounds memory access. "Only" the pointer arithmetic in std::array went out of bounds, though the memory behind the pointer was never accessed.

I guess that the debug flag enables bounds checks on std::array which picks up on it.

[randombit]

@reneme Makes sense thanks

[thesamesam]

ASan doesn't pick this up because there was no out-of-bounds memory access. "Only" the pointer arithmetic in std::array went out of bounds, though the memory behind the pointer was never accessed.

I guess that the debug flag enables bounds checks on std::array which picks up on it.

For libstdc++, -D_GLIBCXX_ASSERTIONS should catch this. For libc++, you need -D_LIBCPP_ENABLE_ASSERTIONS=1 or for >= 18 -D_LIBCPP_ENABLE_HARDENED_MODE=1.

You can instead use the debug options (-D_GLIBCXX_DEBUG, -D_LIBCPP_DEBUG=1) for either libstdc++ or libc++, but those break ABI. In CI though, that should be fine for you to use and will catch more.

EDIT: Confirmed that -D_GLIBCXX_ASSERTIONS catches this with 664a6f7 reverted on master:

Twofish/EAX ran 1602 tests in 3.91 msec all ok
/usr/lib/gcc/x86_64-pc-linux-gnu/13/include/g++-v13/array:202: constexpr std::array<_Tp, _Nm>::value_type& std::array<_Tp, _Nm>::operator[](size_type) [with _Tp = unsigned char; long unsigned int _Nm = 128; reference = unsigned char&; size_type = long unsigned int]: Assertion '__n < this->size()' failed.

Thread 10 "Botan thread" received signal SIGABRT, Aborted.
[Switching to Thread 0x7ffff321d6c0 (LWP 3119485)]
0x00007ffff72bd91c in ?? () from /usr/lib64/libc.so.6
(gdb) bt
#0  0x00007ffff72bd91c in ?? () from /usr/lib64/libc.so.6
#1  0x00007ffff726a226 in raise () from /usr/lib64/libc.so.6
#2  0x00007ffff72518cb in abort () from /usr/lib64/libc.so.6
#3  0x00007ffff74da4af in std::__glibcxx_assert_fail (file=<optimized out>, line=<optimized out>, function=<optimized out>, condition=<optimized out>)
    at /usr/src/debug/sys-devel/gcc-14.0.0_pre20231001/gcc-14-20231001/libstdc++-v3/src/c++11/assert_fail.cc:41
#4  0x00007ffff7a38caf in Botan::BLAKE2b::final_result(std::span<unsigned char, 18446744073709551615ul>) () from /home/sam/git/botan/libbotan-3.so.2
#5  0x000055555576c38a in Botan_Tests::(anonymous namespace)::Message_Auth_Tests::run_one_test(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, Botan_Tests::VarMap const&) ()
#6  0x00005555559306ac in Botan_Tests::Text_Based_Test::run() ()
#7  0x000055555564fe6e in Botan_Tests::(anonymous namespace)::run_a_test(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) ()
#8  0x0000555555651633 in std::_Function_handler<std::unique_ptr<std::__future_base::_Result_base, std::__future_base::_Result_base::_Deleter> (), std::__future_base::_Task_setter<std::unique_ptr<std::__future_base::_Result<std::vector<Botan_Tests::Test::Result, std::allocator<Botan_Tests::Test::Result> > >, std::__future_base::_Result_base::_Deleter>, std::__future_base::_Task_state<std::_Bind<Botan_Tests::Test_Runner::run_tests_multithreaded(std::vector<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::allocator<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > > const&, unsigned long)::{lambda(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&)#1} (std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >)>, std::allocator<int>, std::vector<Botan_Tests::Test::Result, std::allocator<Botan_Tests::Test::Result> > ()>::_M_run()::{lambda()#1}, std::vector<Botan_Tests::Test::Result, std::allocator<Botan_Tests::Test::Result> > > >::_M_invoke(std::_Any_data const&) ()
#9  0x0000555555653cf8 in std::__future_base::_State_baseV2::_M_do_set(std::function<std::unique_ptr<std::__future_base::_Result_base, std::__future_base::_Result_base::_Deleter> ()>*, bool*) ()
#10 0x00007ffff72c0c27 in ?? () from /usr/lib64/libc.so.6
#11 0x000055555564efb1 in std::__future_base::_Task_state<std::_Bind<Botan_Tests::Test_Runner::run_tests_multithreaded(std::vector<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::allocator<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > > const&, unsigned long)::{lambda(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&)#1} (std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >)>, std::allocator<int>, std::vector<Botan_Tests::Test::Result, std::allocator<Botan_Tests::Test::Result> > ()>::_M_run() ()
#12 0x00007ffff7cc4d49 in Botan::Thread_Pool::worker_thread() () from /home/sam/git/botan/libbotan-3.so.2
#13 0x00007ffff74e7e14 in std::execute_native_thread_routine (__p=0x555555aa6a30) at /usr/src/debug/sys-devel/gcc-14.0.0_pre20231001/gcc-14-20231001/libstdc++-v3/src/c++11/thread.cc:104
#14 0x00007ffff72bba70 in ?? () from /usr/lib64/libc.so.6
#15 0x00007ffff733445c in ?? () from /usr/lib64/libc.so.6
(gdb)