search

DaemonEngine / Daemon

The Dæmon game engine. With some bits of ioq3 and XreaL.
https://unvanquished.net
BSD 3-Clause "New" or "Revised" License
298 stars 61 forks source link

SDL2 heap-buffer-overflow #930

Open ghost opened 1 year ago

ghost commented 1 year ago

Running an ASAN enabled build under gdb (even without gdb actually), I got a memory corruption detected when summoning the conole:

=================================================================                                                                                               
==22097==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x6020001f7e34 at pc 0x7ffff784a731 bp 0x7fffffffd1a0 sp 0x7fffffffc950                       
READ of size 6 at 0x6020001f7e34 thread T0
    #0 0x7ffff784a730 in __interceptor_strlen ../../../../src/libsanitizer/sanitizer_common/sanitizer_common_interceptors.inc:389
    #1 0x7ffff76c81da  (/lib/x86_64-linux-gnu/libSDL2-2.0.so.0+0xa11da)
    #2 0x7ffff7732313  (/lib/x86_64-linux-gnu/libSDL2-2.0.so.0+0x10b313)
    #3 0x7ffff77330ea  (/lib/x86_64-linux-gnu/libSDL2-2.0.so.0+0x10c0ea)
    #4 0x7ffff767d382  (/lib/x86_64-linux-gnu/libSDL2-2.0.so.0+0x56382)
    #5 0x7ffff767d719  (/lib/x86_64-linux-gnu/libSDL2-2.0.so.0+0x56719)
    #6 0x55555595b2ff in IN_ProcessEvents Unvanquished/daemon/src/engine/sys/sdl_input.cpp:1071
    #7 0x55555595b647 in IN_Frame() Unvanquished/daemon/src/engine/sys/sdl_input.cpp:1278
    #8 0x5555556b215c in Com_Frame() Unvanquished/daemon/src/engine/qcommon/common.cpp:976
    #9 0x5555556a3eca in Application::ClientApplication::Frame() (Unvanquished/dllbuild/daemon+0x14feca)
    #10 0x555555d2b8f1 in Application::Frame() Unvanquished/daemon/src/engine/framework/Application.cpp:75
    #11 0x555555db5936 in main Unvanquished/daemon/src/engine/framework/System.cpp:754
    #12 0x7ffff6c461c9 in __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58
    #13 0x7ffff6c46284 in __libc_start_main_impl ../csu/libc-start.c:360
    #14 0x55555569fa20 in _start (Unvanquished/dllbuild/daemon+0x14ba20)

0x6020001f7e34 is located 0 bytes to the right of 4-byte region [0x6020001f7e30,0x6020001f7e34)
allocated by thread T0 here:
    #0 0x7ffff78b89cf in __interceptor_malloc ../../../../src/libsanitizer/asan/asan_malloc_linux.cpp:69
    #1 0x7ffff76c7822  (/lib/x86_64-linux-gnu/libSDL2-2.0.so.0+0xa0822)
    #1 0x7ffff76c7822  (/lib/x86_64-linux-gnu/libSDL2-2.0.so.0+0xa0822)

SUMMARY: AddressSanitizer: heap-buffer-overflow ../../../../src/libsanitizer/sanitizer_common/sanitizer_common_interceptors.inc:389 in __interceptor_strlen
Shadow bytes around the buggy address:
  0x0c0480036f70: fa fa fd fa fa fa fd fa fa fa fd fa fa fa fd fa
  0x0c0480036f80: fa fa fd fd fa fa fd fa fa fa 00 07 fa fa fd fa
  0x0c0480036f90: fa fa fd fd fa fa fd fa fa fa 00 fa fa fa fa fa
  0x0c0480036fa0: fa fa fd fd fa fa fd fa fa fa fd fd fa fa fd fa
  0x0c0480036fb0: fa fa fd fa fa fa fd fa fa fa fd fd fa fa fd fa
=>0x0c0480036fc0: fa fa fa fa fa fa[04]fa fa fa fd fa fa fa fa fa
  0x0c0480036fd0: fa fa fa fa fa fa fa fa fa fa fd fa fa fa fa fa
  0x0c0480036fe0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c0480036ff0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c0480037000: fa fa fd fa fa fa fd fa fa fa fd fd fa fa fd fd
  0x0c0480037010: fa fa fd fa fa fa fd fa fa fa fd fa fa fa fd fa
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07
  Heap left redzone:       fa
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  Stack after return:      f5
  Stack use after scope:   f8
  Global redzone:          f9
  Global init order:       f6
  Poisoned by user:        f7
  Container overflow:      fc
  Array cookie:            ac
  Intra object redzone:    bb
  ASan internal:           fe
  Left alloca redzone:     ca
  Right alloca redzone:    cb
==22097==ABORTING
[Thread 0x7fffb21ff6c0 (LWP 30261) exited]
[Thread 0x7fffb156c6c0 (LWP 30187) exited]
[Thread 0x7fffb8ef86c0 (LWP 30045) exited]
[Thread 0x7fffc33fb6c0 (LWP 29786) exited]
[Thread 0x7fffc3bfc6c0 (LWP 29783) exited]
[Thread 0x7fffc43fd6c0 (LWP 29782) exited]
[Thread 0x7fffc4bfe6c0 (LWP 29781) exited]
[Thread 0x7fffc53ff6c0 (LWP 29780) exited]
[Thread 0x7ffff27ff6c0 (LWP 26099) exited]
[Thread 0x7ffff32ff6c0 (LWP 26098) exited]
[Thread 0x7ffff4f10240 (LWP 22097) exited]
[Thread 0x7fffb9f866c0 (LWP 30044) exited]
[New process 22097]
[Inferior 1 (process 22097) exited with code 01]

Has to be noted that clang (Debian clang version 14.0.6) do complains (warn about possible corruptions in strlen and other C-string management functions (some stringop stuff, I don't have the warning right now) so it might be unrelated to SDL itself (but I would not be that surprised if SDL played a role).

ghost commented 1 year ago

I was wrong, it was not clang emiting this warning but gcc:

In file included from /usr/include/c++/12/memory:63,
                 from Unvanquished/daemon/src/engine/qcommon/q_shared.h:98,
                 from Unvanquished/daemon/src/common/Common.h:35:
In static member function ‘static _Tp* std::__copy_move<_IsMove, true, std::random_access_iterator_tag>::__copy_m(const _Tp*, const _Tp*, _Tp*) [with _Tp = char; bool _IsMove = true]’,
    inlined from ‘_OI std::__copy_move_a2(_II, _II, _OI) [with bool _IsMove = true; _II = char*; _OI = char*]’ at /usr/include/c++/12/bits/stl_algobase.h:495:30,
    inlined from ‘_OI std::__copy_move_a1(_II, _II, _OI) [with bool _IsMove = true; _II = char*; _OI = char*]’ at /usr/include/c++/12/bits/stl_algobase.h:522:42,
    inlined from ‘_OI std::__copy_move_a(_II, _II, _OI) [with bool _IsMove = true; _II = char*; _OI = char*]’ at /usr/include/c++/12/bits/stl_algobase.h:529:31,
    inlined from ‘_OI std::copy(_II, _II, _OI) [with _II = move_iterator<char*>; _OI = char*]’ at /usr/include/c++/12/bits/stl_algobase.h:620:7,
    inlined from ‘static _ForwardIterator std::__uninitialized_copy<true>::__uninit_copy(_InputIterator, _InputIterator, _ForwardIterator) [with _InputIterator = std::move_iterator<char*>; _ForwardIterator = char*]’ at /usr/include/c++/12/bits/stl_uninitialized.h:147:27,
    inlined from ‘_ForwardIterator std::uninitialized_copy(_InputIterator, _InputIterator, _ForwardIterator) [with _InputIterator = move_iterator<char*>; _ForwardIterator = char*]’ at /usr/include/c++/12/bits/stl_uninitialized.h:185:15,
    inlined from ‘_ForwardIterator std::__uninitialized_copy_a(_InputIterator, _InputIterator, _ForwardIterator, allocator<_Tp>&) [with _InputIterator = move_iterator<char*>; _ForwardIterator = char*; _Tp = char]’ at /usr/include/c++/12/bits/stl_uninitialized.h:372:37,
    inlined from ‘_ForwardIterator std::__uninitialized_move_if_noexcept_a(_InputIterator, _InputIterator, _ForwardIterator, _Allocator&) [with _InputIterator = char*; _ForwardIterator = char*; _Allocator = allocator<char>]’ at /usr/include/c++/12/bits/stl_uninitialized.h:397:2,
    inlined from ‘void std::vector<_Tp, _Alloc>::_M_range_insert(iterator, _ForwardIterator, _ForwardIterator, std::forward_iterator_tag) [with _ForwardIterator = const char*; _Tp = char; _Alloc = std::allocator<char>]’ at /usr/include/c++/12/bits/vector.tcc:801:9,
    inlined from ‘void std::vector<_Tp, _Alloc>::_M_insert_dispatch(iterator, _InputIterator, _InputIterator, std::__false_type) [with _InputIterator = const char*; _Tp = char; _Alloc = std::allocator<char>]’ at /usr/include/c++/12/bits/stl_vector.h:1779:19,
    inlined from ‘std::vector<_Tp, _Alloc>::iterator std::vector<_Tp, _Alloc>::insert(const_iterator, _InputIterator, _InputIterator) [with _InputIterator = const char*; <template-parameter-2-2> = void; _Tp = char; _Alloc = std::allocator<char>]’ at /usr/include/c++/12/bits/stl_vector.h:1481:22,
    inlined from ‘void Util::Writer::WriteData(const void*, size_t)’ at Unvanquished/daemon/src/common/Serialize.h:77:15,
    inlined from ‘static void Util::SerializeTraits<T, typename std::enable_if<(std::is_pod<_Tp>::value && (! std::is_array< <template-parameter-1-1> >::value))>::type>::Write(Util::Writer&, const T&) [with T = unsigned int]’ at Unvanquished/daemon/src/common/Serialize.h:232:20,
    inlined from ‘void Util::Writer::Write(Arg&&) [with T = unsigned int; Arg = const unsigned int&]’ at Unvanquished/daemon/src/common/Serialize.h:87:29,
    inlined from ‘void VM::VMBase::Free()’ at Unvanquished/daemon/src/engine/framework/VirtualMachine.cpp:487:24:
/usr/include/c++/12/bits/stl_algobase.h:431:30: warning: ‘void* __builtin_memcpy(void*, const void*, long unsigned int)’ writing 1 or more bytes into a region of size 0 overflows the destination [-Wstringop-overflow=]
  431 |             __builtin_memmove(__result, __first, sizeof(_Tp) * _Num);
      |             ~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from /usr/include/x86_64-linux-gnu/c++/12/bits/c++allocator.h:33,
                 from /usr/include/c++/12/bits/allocator.h:46,
                 from /usr/include/c++/12/memory:64:
In member function ‘_Tp* std::__new_allocator<_Tp>::allocate(size_type, const void*) [with _Tp = char]’,
    inlined from ‘static _Tp* std::allocator_traits<std::allocator<_Tp1> >::allocate(allocator_type&, size_type) [with _Tp = char]’ at /usr/include/c++/12/bits/alloc_traits.h:464:28,
    inlined from ‘std::_Vector_base<_Tp, _Alloc>::pointer std::_Vector_base<_Tp, _Alloc>::_M_allocate(std::size_t) [with _Tp = char; _Alloc = std::allocator<char>]’ at /usr/include/c++/12/bits/stl_vector.h:378:33,
    inlined from ‘void std::vector<_Tp, _Alloc>::_M_range_insert(iterator, _ForwardIterator, _ForwardIterator, std::forward_iterator_tag) [with _ForwardIterator = const char*; _Tp = char; _Alloc = std::allocator<char>]’ at /usr/include/c++/12/bits/vector.tcc:787:40,
    inlined from ‘void std::vector<_Tp, _Alloc>::_M_insert_dispatch(iterator, _InputIterator, _InputIterator, std::__false_type) [with _InputIterator = const char*; _Tp = char; _Alloc = std::allocator<char>]’ at /usr/include/c++/12/bits/stl_vector.h:1779:19,
    inlined from ‘std::vector<_Tp, _Alloc>::iterator std::vector<_Tp, _Alloc>::insert(const_iterator, _InputIterator, _InputIterator) [with _InputIterator = const char*; <template-parameter-2-2> = void; _Tp = char; _Alloc = std::allocator<char>]’ at /usr/include/c++/12/bits/stl_vector.h:1481:22,
    inlined from ‘void Util::Writer::WriteData(const void*, size_t)’ at Unvanquished/daemon/src/common/Serialize.h:77:15,
    inlined from ‘static void Util::SerializeTraits<T, typename std::enable_if<(std::is_pod<_Tp>::value && (! std::is_array< <template-parameter-1-1> >::value))>::type>::Write(Util::Writer&, const T&) [with T = unsigned int]’ at Unvanquished/daemon/src/common/Serialize.h:232:20,
    inlined from ‘void Util::Writer::Write(Arg&&) [with T = unsigned int; Arg = const unsigned int&]’ at Unvanquished/daemon/src/common/Serialize.h:87:29,
    inlined from ‘void VM::VMBase::Free()’ at Unvanquished/daemon/src/engine/framework/VirtualMachine.cpp:487:24:
/usr/include/c++/12/bits/new_allocator.h:137:55: note: at offset 4 into destination object of size 4 allocated by ‘operator new’
  137 |         return static_cast<_Tp*>(_GLIBCXX_OPERATOR_NEW(__n * sizeof(_Tp)));
      |                                                       ^
In static member function ‘static _Tp* std::__copy_move<_IsMove, true, std::random_access_iterator_tag>::__copy_m(const _Tp*, const _Tp*, _Tp*) [with _Tp = char; bool _IsMove = true]’,
    inlined from ‘_OI std::__copy_move_a2(_II, _II, _OI) [with bool _IsMove = true; _II = char*; _OI = char*]’ at /usr/include/c++/12/bits/stl_algobase.h:495:30,
    inlined from ‘_OI std::__copy_move_a1(_II, _II, _OI) [with bool _IsMove = true; _II = char*; _OI = char*]’ at /usr/include/c++/12/bits/stl_algobase.h:522:42,
    inlined from ‘_OI std::__copy_move_a(_II, _II, _OI) [with bool _IsMove = true; _II = char*; _OI = char*]’ at /usr/include/c++/12/bits/stl_algobase.h:529:31,
    inlined from ‘_OI std::copy(_II, _II, _OI) [with _II = move_iterator<char*>; _OI = char*]’ at /usr/include/c++/12/bits/stl_algobase.h:620:7,
    inlined from ‘static _ForwardIterator std::__uninitialized_copy<true>::__uninit_copy(_InputIterator, _InputIterator, _ForwardIterator) [with _InputIterator = std::move_iterator<char*>; _ForwardIterator = char*]’ at /usr/include/c++/12/bits/stl_uninitialized.h:147:27,
    inlined from ‘_ForwardIterator std::uninitialized_copy(_InputIterator, _InputIterator, _ForwardIterator) [with _InputIterator = move_iterator<char*>; _ForwardIterator = char*]’ at /usr/include/c++/12/bits/stl_uninitialized.h:185:15,
    inlined from ‘_ForwardIterator std::__uninitialized_copy_a(_InputIterator, _InputIterator, _ForwardIterator, allocator<_Tp>&) [with _InputIterator = move_iterator<char*>; _ForwardIterator = char*; _Tp = char]’ at /usr/include/c++/12/bits/stl_uninitialized.h:372:37,
    inlined from ‘_ForwardIterator std::__uninitialized_move_if_noexcept_a(_InputIterator, _InputIterator, _ForwardIterator, _Allocator&) [with _InputIterator = char*; _ForwardIterator = char*; _Allocator = allocator<char>]’ at /usr/include/c++/12/bits/stl_uninitialized.h:397:2,
    inlined from ‘void std::vector<_Tp, _Alloc>::_M_range_insert(iterator, _ForwardIterator, _ForwardIterator, std::forward_iterator_tag) [with _ForwardIterator = const char*; _Tp = char; _Alloc = std::allocator<char>]’ at /usr/include/c++/12/bits/vector.tcc:801:9,
    inlined from ‘void std::vector<_Tp, _Alloc>::_M_insert_dispatch(iterator, _InputIterator, _InputIterator, std::__false_type) [with _InputIterator = const char*; _Tp = char; _Alloc = std::allocator<char>]’ at /usr/include/c++/12/bits/stl_vector.h:1779:19,
    inlined from ‘std::vector<_Tp, _Alloc>::iterator std::vector<_Tp, _Alloc>::insert(const_iterator, _InputIterator, _InputIterator) [with _InputIterator = const char*; <template-parameter-2-2> = void; _Tp = char; _Alloc = std::allocator<char>]’ at /usr/include/c++/12/bits/stl_vector.h:1481:22,
    inlined from ‘void Util::Writer::WriteData(const void*, size_t)’ at Unvanquished/daemon/src/common/Serialize.h:77:15,
    inlined from ‘static void Util::SerializeTraits<T, typename std::enable_if<(std::is_pod<_Tp>::value && (! std::is_array< <template-parameter-1-1> >::value))>::type>::Write(Util::Writer&, const T&) [with T = unsigned int]’ at Unvanquished/daemon/src/common/Serialize.h:232:20,
    inlined from ‘void Util::Writer::Write(Arg&&) [with T = unsigned int; Arg = const unsigned int&]’ at Unvanquished/daemon/src/common/Serialize.h:87:29,
    inlined from ‘void VM::VMBase::Free()’ at Unvanquished/daemon/src/engine/framework/VirtualMachine.cpp:487:24:
/usr/include/c++/12/bits/stl_algobase.h:431:30: warning: ‘void* __builtin_memcpy(void*, const void*, long unsigned int)’ writing 1 or more bytes into a region of size 0 overflows the destination [-Wstringop-overflow=]
  431 |             __builtin_memmove(__result, __first, sizeof(_Tp) * _Num);
      |             ~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In member function ‘_Tp* std::__new_allocator<_Tp>::allocate(size_type, const void*) [with _Tp = char]’,
    inlined from ‘static _Tp* std::allocator_traits<std::allocator<_Tp1> >::allocate(allocator_type&, size_type) [with _Tp = char]’ at /usr/include/c++/12/bits/alloc_traits.h:464:28,
    inlined from ‘std::_Vector_base<_Tp, _Alloc>::pointer std::_Vector_base<_Tp, _Alloc>::_M_allocate(std::size_t) [with _Tp = char; _Alloc = std::allocator<char>]’ at /usr/include/c++/12/bits/stl_vector.h:378:33,
    inlined from ‘void std::vector<_Tp, _Alloc>::_M_range_insert(iterator, _ForwardIterator, _ForwardIterator, std::forward_iterator_tag) [with _ForwardIterator = const char*; _Tp = char; _Alloc = std::allocator<char>]’ at /usr/include/c++/12/bits/vector.tcc:787:40,
    inlined from ‘void std::vector<_Tp, _Alloc>::_M_insert_dispatch(iterator, _InputIterator, _InputIterator, std::__false_type) [with _InputIterator = const char*; _Tp = char; _Alloc = std::allocator<char>]’ at /usr/include/c++/12/bits/stl_vector.h:1779:19,
    inlined from ‘std::vector<_Tp, _Alloc>::iterator std::vector<_Tp, _Alloc>::insert(const_iterator, _InputIterator, _InputIterator) [with _InputIterator = const char*; <template-parameter-2-2> = void; _Tp = char; _Alloc = std::allocator<char>]’ at /usr/include/c++/12/bits/stl_vector.h:1481:22,
    inlined from ‘void Util::Writer::WriteData(const void*, size_t)’ at Unvanquished/daemon/src/common/Serialize.h:77:15,
    inlined from ‘static void Util::SerializeTraits<T, typename std::enable_if<(std::is_pod<_Tp>::value && (! std::is_array< <template-parameter-1-1> >::value))>::type>::Write(Util::Writer&, const T&) [with T = unsigned int]’ at Unvanquished/daemon/src/common/Serialize.h:232:20,
    inlined from ‘void Util::Writer::Write(Arg&&) [with T = unsigned int; Arg = const unsigned int&]’ at Unvanquished/daemon/src/common/Serialize.h:87:29,
    inlined from ‘void VM::VMBase::Free()’ at Unvanquished/daemon/src/engine/framework/VirtualMachine.cpp:487:24:
/usr/include/c++/12/bits/new_allocator.h:137:55: note: at offset 4 into destination object of size 4 allocated by ‘operator new’
  137 |         return static_cast<_Tp*>(_GLIBCXX_OPERATOR_NEW(__n * sizeof(_Tp)));
      |                                                       ^
ghost commented 1 year ago

The ASAN problem and the compiler problem are apparently different ones, as this patch:

diff --git a/src/common/Serialize.h b/src/common/Serialize.h
index f7eb72ad..1d68cff3 100644
--- a/src/common/Serialize.h
+++ b/src/common/Serialize.h
@@ -229,12 +229,12 @@ namespace Util {
  struct SerializeTraits<T, typename std::enable_if<std::is_pod<T>::value && !std::is_array<T>::value>::type> {
    static void Write(Writer& stream, const T& value)
    {
-     stream.WriteData(std::addressof(value), sizeof(value));
+     stream.WriteData(std::addressof(value), sizeof(T));
    }
    static T Read(Reader& stream)
    {
      T value;
-     stream.ReadData(std::addressof(value), sizeof(value));
+     stream.ReadData(std::addressof(value), sizeof(T));
      return value;
    }
  };

Fixes the warning but not the corruption (patch is: don't take the size of a reference, but the size of the actual type we want to read or write).

DolceTriade commented 1 year ago

I think this fix looks correct. @slipher WDYT?

ghost commented 1 year ago

I have a "good news": the ASAN issue I had is indeed unrelated to unvanquished, but is simply yet another SDL2 bug, which has been fixed: good thing. But Debian stable do have this bug. In any case, this issue can be closed, but since I mentioned and possibly fixed a different one in here, I'll let you handle it.

PS: considering the troubles SDL2 never stopped giving me since few years (I do remember with high distaste when they introduced dbus dependency and it eated all my CPU... for example), and those I've seen it gives to other people using rolling release distros, I think I might tinker with a glfw3 backend someday, since SDL2 is only used for the windowing system anyway.

DolceTriade commented 1 year ago

SDL2 is also used for input

slipher commented 1 year ago

I think this fix looks correct. @slipher WDYT?

Well it certainly doesn't make any difference to the functioning of the code - sizeof(T) and sizeof(value). If it manages to work around the spurious GCC warning, it's fine. Hard to see why it would make a difference though as sizeof(whatever) is just a number which is the same in both cases, so it shouldn't affect anything else...