search

RPCS3 / rpcs3

PlayStation 3 emulator and debugger
https://rpcs3.net/
GNU General Public License v2.0
15.46k stars 1.92k forks source link

Vulkan: Incorrect use of vk::as_rtt in vk::resolve_image #6783

Closed cebtenzzre closed 5 years ago

cebtenzzre commented 5 years ago

RPCS3 version: 0.0.7-8901 Alpha (commit 0fe46934de258826e72bfb00eb20c606d0a635cd) OS: Arch Linux (x86_64), kernel v5.3.6-arch1

Steps to reproduce:

Expected behavior: The game starts.

Actual behavior: RPCS3 sometimes crashes after it has loaded precompiled shaders, but before the games starts. Other times, it silently corrupts the heap but works anyway.


Here's what I believe is going on:


From cppreference, under conversion number 2:

Such static_cast makes no runtime checks to ensure that the object's runtime type is actually D, and may only be used safely if this precondition is guaranteed by other means, such as when implementing static polymorphism. Safe downcast may be done with dynamic_cast.

Since RPCS3 implements no safety mechanism for this cast, it would make sense to follow the above recommendation and use dynamic_cast. This is a simple patch that would accomplish this:

diff --git a/rpcs3/Emu/RSX/GL/GLRenderTargets.h b/rpcs3/Emu/RSX/GL/GLRenderTargets.h
index f3060ea8e..932972a8b 100644
--- a/rpcs3/Emu/RSX/GL/GLRenderTargets.h
+++ b/rpcs3/Emu/RSX/GL/GLRenderTargets.h
@@ -119,7 +119,7 @@ namespace gl

    static inline gl::render_target* as_rtt(gl::texture* t)
    {
-       return reinterpret_cast<gl::render_target*>(t);
+       return &dynamic_cast<gl::render_target&>(*t);
    }
 }

diff --git a/rpcs3/Emu/RSX/VK/VKRenderTargets.h b/rpcs3/Emu/RSX/VK/VKRenderTargets.h
index 287b9eb68..a5805ef1a 100644
--- a/rpcs3/Emu/RSX/VK/VKRenderTargets.h
+++ b/rpcs3/Emu/RSX/VK/VKRenderTargets.h
@@ -541,7 +541,7 @@ namespace vk

    static inline vk::render_target* as_rtt(vk::image* t)
    {
-       return static_cast<vk::render_target*>(t);
+       return &dynamic_cast<vk::render_target&>(*t);
    }
 }

This uses dynamic_cast on a reference so that instead of unpredictable memory corruption, you get a reliable std::bad_cast exception in the RPCS3 console. gl::as_rtt performs a similar cast (with reinterpret_cast for some reason), which should also use dynamic_cast. Obviously, the bug is still there, it is just reliably caught this way.


The following ASAN report is triggered by this bug. It shows that vk::resolve_image is using vk::as_rtt to write past the end of resolve_surface, which in this instance contained memory that was previously freed.

==73637==ERROR: AddressSanitizer: heap-use-after-free on address 0x612000083178 at pc 0x000001e39df6 bp 0x7fd68e884c50 sp 0x7fd68e884c40
WRITE of size 4 at 0x612000083178 thread T23 (rsx::thread)
    #0 0x1e39df5 in vk::resolve_image(vk::command_buffer&, vk::viewable_image*, vk::viewable_image*) /mnt/Data/src/rpcs3-git/src/rpcs3/rpcs3/Emu/RSX/VK/VKResolveHelper.cpp:106
    #1 0x1ca839c in vk::render_target::resolve(vk::command_buffer&) /mnt/Data/src/rpcs3-git/src/rpcs3/rpcs3/Emu/RSX/VK/VKRenderTargets.h:92
    #2 0x1d1b201 in vk::render_target::memory_barrier(vk::command_buffer&, rsx::surface_access) /mnt/Data/src/rpcs3-git/src/rpcs3/rpcs3/Emu/RSX/VK/VKRenderTargets.h:404
    #3 0x1d25069 in rsx::surface_store<rsx::vk_render_target_traits>::get_merged_texture_memory_region<vk::command_buffer>(vk::command_buffer&, unsigned int, unsigned int, unsigned int, unsigned int, unsigned char, rsx::surface_access)::{lambda(std::unordered_map<unsigned int, std::unique_ptr<vk::render_target, std::default_delete<vk::render_target> >, std::hash<unsigned int>, std::equal_to<unsigned int>, std::allocator<std::pair<unsigned int const, std::unique_ptr<vk::render_target, std::default_delete<vk::render_target> > > > >&, bool)#1}::operator()(std::unordered_map<unsigned int, std::unique_ptr<vk::render_target, std::default_delete<vk::render_target> >, std::hash<unsigned int>, std::equal_to<unsigned int>, std::allocator<std::pair<unsigned int const, std::unique_ptr<vk::render_target, std::default_delete<vk::render_target> > > > >&, bool) const /mnt/Data/src/rpcs3-git/src/rpcs3/rpcs3/Emu/RSX/VK/../Common/surface_store.h:831
    #4 0x1d73a95 in std::vector<rsx::surface_overlap_info_t<vk::render_target*>, std::allocator<rsx::surface_overlap_info_t<vk::render_target*> > > rsx::surface_store<rsx::vk_render_target_traits>::get_merged_texture_memory_region<vk::command_buffer>(vk::command_buffer&, unsigned int, unsigned int, unsigned int, unsigned int, unsigned char, rsx::surface_access) /mnt/Data/src/rpcs3-git/src/rpcs3/rpcs3/Emu/RSX/VK/../Common/surface_store.h:865
    #5 0x1d9fc84 in rsx::texture_cache<vk::texture_cache, vk::texture_cache_traits>::sampled_image_descriptor rsx::texture_cache<vk::texture_cache, vk::texture_cache_traits>::upload_texture<rsx::fragment_texture, rsx::vk_render_targets>(vk::command_buffer&, rsx::fragment_texture&, rsx::vk_render_targets&) /mnt/Data/src/rpcs3-git/src/rpcs3/rpcs3/Emu/RSX/VK/../Common/texture_cache.h:2119
    #6 0x1c4bc6e in VKGSRender::end() /mnt/Data/src/rpcs3-git/src/rpcs3/rpcs3/Emu/RSX/VK/VKGSRender.cpp:1280
    #7 0x2136aef in rsx::thread::run_FIFO() /mnt/Data/src/rpcs3-git/src/rpcs3/rpcs3/Emu/RSX/RSXFIFO.cpp:575
    #8 0x1954352 in rsx::thread::on_task() /mnt/Data/src/rpcs3-git/src/rpcs3/rpcs3/Emu/RSX/RSXThread.cpp:587
    #9 0x1930935 in rsx::thread::operator()() /mnt/Data/src/rpcs3-git/src/rpcs3/rpcs3/Emu/RSX/RSXThread.cpp:405
    #10 0x88d01d in named_thread<VKGSRender>::entry_point() /mnt/Data/src/rpcs3-git/src/rpcs3/Utilities/Thread.h:383
    #11 0x88d01d in named_thread<VKGSRender>::entry_point(void*) /mnt/Data/src/rpcs3-git/src/rpcs3/Utilities/Thread.h:315
    #12 0x7fd72ce474ce in start_thread (/usr/lib/libpthread.so.0+0x94ce)
    #13 0x7fd7293cd2d2 in clone (/usr/lib/libc.so.6+0xff2d2)

0x612000083178 is located 56 bytes inside of 260-byte region [0x612000083140,0x612000083244)
freed by thread T13 (PPU[0x1000000] ) here:
    #0 0x7fd72d4096b0 in __interceptor_free /build/gcc/src/gcc/libsanitizer/asan/asan_malloc_linux.cc:122
    #1 0x35f66c1 in llvm::SmallVector<unsigned int, 32u>::~SmallVector() /mnt/Data/src/rpcs3-git/src/rpcs3/llvm/include/llvm/ADT/SmallVector.h:336
    #2 0x35f66c1 in llvm::FoldingSetNodeID::~FoldingSetNodeID() /mnt/Data/src/rpcs3-git/src/rpcs3/llvm/include/llvm/ADT/FoldingSet.h:305
    #3 0x35f66c1 in llvm::PMTopLevelManager::findAnalysisUsage(llvm::Pass*) /mnt/Data/src/rpcs3-git/src/rpcs3/llvm/lib/IR/LegacyPassManager.cpp:682

previously allocated by thread T13 (PPU[0x1000000] ) here:
    #0 0x7fd72d409aca in __interceptor_malloc /build/gcc/src/gcc/libsanitizer/asan/asan_malloc_linux.cc:144
    #1 0x37c0dcf in llvm::safe_malloc(unsigned long) /mnt/Data/src/rpcs3-git/src/rpcs3/llvm/include/llvm/Support/MemAlloc.h:26
    #2 0x37c0dcf in llvm::SmallVectorBase::grow_pod(void*, unsigned long, unsigned long) /mnt/Data/src/rpcs3-git/src/rpcs3/llvm/lib/Support/SmallVector.cpp:54
    #3 0x72aa675ffffffff  (<unknown module>)

Thread T23 (rsx::thread) created by T0 here:
    #0 0x7fd72d333367 in __interceptor_pthread_create /build/gcc/src/gcc/libsanitizer/asan/asan_interceptors.cc:208
    #1 0xaa4258 in thread_base::start(void* (*)(void*)) /mnt/Data/src/rpcs3-git/src/rpcs3/Utilities/Thread.cpp:1655
    #2 0x887198 in named_thread<VKGSRender>::named_thread<true, void>() /mnt/Data/src/rpcs3-git/src/rpcs3/Utilities/Thread.h:424
    #3 0x887198 in named_thread<VKGSRender>* stx::manual_fixed_typemap<void>::init<rsx::thread, named_thread<VKGSRender>>() /mnt/Data/src/rpcs3-git/src/rpcs3/Utilities/Thread.h:420
    #4 0x887198 in operator() /mnt/Data/src/rpcs3-git/src/rpcs3/rpcs3/main_application.cpp:136
    #5 0x887198 in _M_invoke /usr/include/c++/9.2.0/bits/std_function.h:300
    #6 0x93db61 in std::function<void ()>::operator()() const /usr/include/c++/9.2.0/bits/std_function.h:690
    #7 0x93db61 in Emulator::Load(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, bool, bool) /mnt/Data/src/rpcs3-git/src/rpcs3/rpcs3/Emu/System.cpp:1582
    #8 0x94748d in Emulator::BootGame(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, bool, bool, bool) /mnt/Data/src/rpcs3-git/src/rpcs3/rpcs3/Emu/System.cpp:747
    #9 0x237763e in main_window::Boot(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, bool, bool, bool) /mnt/Data/src/rpcs3-git/src/rpcs3/rpcs3/rpcs3qt/main_window.cpp:247
    #10 0x237a85c in main_window::BootRecentAction(QAction const*) /mnt/Data/src/rpcs3-git/src/rpcs3/rpcs3/rpcs3qt/main_window.cpp:988
    #11 0x237c16d in main_window::OnPlayOrPause() /mnt/Data/src/rpcs3-git/src/rpcs3/rpcs3/rpcs3qt/main_window.cpp:223
    #12 0x23088d1 in QtPrivate::FunctorCall<QtPrivate::IndexesList<>, QtPrivate::List<>, void, void (main_window::*)()>::call(void (main_window::*)(), main_window*, void**) /usr/include/qt/QtCore/qobjectdefs_impl.h:152
    #13 0x23088d1 in void QtPrivate::FunctionPointer<void (main_window::*)()>::call<QtPrivate::List<>, void>(void (main_window::*)(), main_window*, void**) /usr/include/qt/QtCore/qobjectdefs_impl.h:185
    #14 0x23088d1 in QtPrivate::QSlotObject<void (main_window::*)(), QtPrivate::List<>, void>::impl(int, QtPrivate::QSlotObjectBase*, QObject*, void**, bool*) /usr/include/qt/QtCore/qobjectdefs_impl.h:414
    #15 0x23088d1 in QtPrivate::QSlotObject<void (main_window::*)(), QtPrivate::List<>, void>::impl(int, QtPrivate::QSlotObjectBase*, QObject*, void**, bool*) /usr/include/qt/QtCore/qobjectdefs_impl.h:407
    #16 0x7fd729b00b6f in QMetaObject::activate(QObject*, int, int, void**) (/usr/lib/libQt5Core.so.5+0x2bcb6f)
    #17 0x7fd72abb9eb2 in QAction::triggered(bool) (/usr/lib/libQt5Widgets.so.5+0x152eb2)

Thread T13 (PPU[0x1000000] ) created by T0 here:
    #0 0x7fd72d333367 in __interceptor_pthread_create /build/gcc/src/gcc/libsanitizer/asan/asan_interceptors.cc:208
    #1 0xaa4258 in thread_base::start(void* (*)(void*)) /mnt/Data/src/rpcs3-git/src/rpcs3/Utilities/Thread.cpp:1655
    #2 0xbb1764 in named_thread<ppu_thread>::named_thread<ppu_thread_params&, char const (&) [12], int&, int, void>(std::basic_string_view<char, std::char_traits<char> >, ppu_thread_params&, char const (&) [12], int&, int&&) /mnt/Data/src/rpcs3-git/src/rpcs3/rpcs3/../Utilities/Thread.h:433
    #3 0xbb1764 in void __gnu_cxx::new_allocator<named_thread<ppu_thread> >::construct<named_thread<ppu_thread>, char const (&) [36], ppu_thread_params&, char const (&) [12], int&, int>(named_thread<ppu_thread>*, char const (&) [36], ppu_thread_params&, char const (&) [12], int&, int&&) /usr/include/c++/9.2.0/ext/new_allocator.h:147
    #4 0xbb1764 in void std::allocator_traits<std::allocator<named_thread<ppu_thread> > >::construct<named_thread<ppu_thread>, char const (&) [36], ppu_thread_params&, char const (&) [12], int&, int>(std::allocator<named_thread<ppu_thread> >&, named_thread<ppu_thread>*, char const (&) [36], ppu_thread_params&, char const (&) [12], int&, int&&) /usr/include/c++/9.2.0/bits/alloc_traits.h:484
    #5 0xbb1764 in std::_Sp_counted_ptr_inplace<named_thread<ppu_thread>, std::allocator<named_thread<ppu_thread> >, (__gnu_cxx::_Lock_policy)2>::_Sp_counted_ptr_inplace<char const (&) [36], ppu_thread_params&, char const (&) [12], int&, int>(std::allocator<named_thread<ppu_thread> >, char const (&) [36], ppu_thread_params&, char const (&) [12], int&, int&&) /usr/include/c++/9.2.0/bits/shared_ptr_base.h:548
    #6 0xbb1764 in std::__shared_count<(__gnu_cxx::_Lock_policy)2>::__shared_count<named_thread<ppu_thread>, std::allocator<named_thread<ppu_thread> >, char const (&) [36], ppu_thread_params&, char const (&) [12], int&, int>(named_thread<ppu_thread>*&, std::_Sp_alloc_shared_tag<std::allocator<named_thread<ppu_thread> > >, char const (&) [36], ppu_thread_params&, char const (&) [12], int&, int&&) /usr/include/c++/9.2.0/bits/shared_ptr_base.h:679
    #7 0xbb1764 in std::__shared_ptr<named_thread<ppu_thread>, (__gnu_cxx::_Lock_policy)2>::__shared_ptr<std::allocator<named_thread<ppu_thread> >, char const (&) [36], ppu_thread_params&, char const (&) [12], int&, int>(std::_Sp_alloc_shared_tag<std::allocator<named_thread<ppu_thread> > >, char const (&) [36], ppu_thread_params&, char const (&) [12], int&, int&&) /usr/include/c++/9.2.0/bits/shared_ptr_base.h:1344
    #8 0xbb1764 in std::shared_ptr<named_thread<ppu_thread> >::shared_ptr<std::allocator<named_thread<ppu_thread> >, char const (&) [36], ppu_thread_params&, char const (&) [12], int&, int>(std::_Sp_alloc_shared_tag<std::allocator<named_thread<ppu_thread> > >, char const (&) [36], ppu_thread_params&, char const (&) [12], int&, int&&) /usr/include/c++/9.2.0/bits/shared_ptr.h:359
    #9 0xbb1764 in std::shared_ptr<named_thread<ppu_thread> > std::allocate_shared<named_thread<ppu_thread>, std::allocator<named_thread<ppu_thread> >, char const (&) [36], ppu_thread_params&, char const (&) [12], int&, int>(std::allocator<named_thread<ppu_thread> > const&, char const (&) [36], ppu_thread_params&, char const (&) [12], int&, int&&) /usr/include/c++/9.2.0/bits/shared_ptr.h:702
    #10 0xbb1764 in std::shared_ptr<named_thread<ppu_thread> > std::make_shared<named_thread<ppu_thread>, char const (&) [36], ppu_thread_params&, char const (&) [12], int&, int>(char const (&) [36], ppu_thread_params&, char const (&) [12], int&, int&&) /usr/include/c++/9.2.0/bits/shared_ptr.h:718
    #11 0xbb1764 in idm::make_ptr<named_thread<ppu_thread>, named_thread<ppu_thread>, char const (&) [36], ppu_thread_params&, char const (&) [12], int&, int>(char const (&) [36], ppu_thread_params&, char const (&) [12], int&, int&&)::{lambda()#1}::operator()() const /mnt/Data/src/rpcs3-git/src/rpcs3/rpcs3/Emu/IdManager.h:304
    #12 0xbb1764 in std::pair<id_manager::id_key, std::shared_ptr<void> >* idm::create_id<named_thread<ppu_thread>, named_thread<ppu_thread>, idm::make_ptr<named_thread<ppu_thread>, named_thread<ppu_thread>, char const (&) [36], ppu_thread_params&, char const (&) [12], int&, int>(char const (&) [36], ppu_thread_params&, char const (&) [12], int&, int&&)::{lambda()#1}>(idm::make_ptr<named_thread<ppu_thread>, named_thread<ppu_thread>, char const (&) [36], ppu_thread_params&, char const (&) [12], int&, int>(char const (&) [36], ppu_thread_params&, char const (&) [12], int&, int&&)::{lambda()#1}&&) /mnt/Data/src/rpcs3-git/src/rpcs3/rpcs3/Emu/IdManager.h:276
    #13 0xbb1764 in std::enable_if<std::is_constructible<named_thread<ppu_thread>, char const (&) [36], ppu_thread_params&, char const (&) [12], int&, int>::value, std::shared_ptr<named_thread<ppu_thread> > >::type idm::make_ptr<named_thread<ppu_thread>, named_thread<ppu_thread>, char const (&) [36], ppu_thread_params&, char const (&) [12], int&, int>(char const (&) [36], ppu_thread_params&, char const (&) [12], int&, int&&) /mnt/Data/src/rpcs3-git/src/rpcs3/rpcs3/Emu/IdManager.h:304
    #14 0xbb1764 in ppu_load_exec(elf_object<elf_be, unsigned long, (elf_machine)21, (elf_os)0, (elf_type)2> const&) /mnt/Data/src/rpcs3-git/src/rpcs3/rpcs3/Emu/Cell/PPUModule.cpp:1429
    #15 0x93a4cd in Emulator::Load(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, bool, bool) /mnt/Data/src/rpcs3-git/src/rpcs3/rpcs3/Emu/System.cpp:1559
    #16 0x94748d in Emulator::BootGame(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, bool, bool, bool) /mnt/Data/src/rpcs3-git/src/rpcs3/rpcs3/Emu/System.cpp:747
    #17 0x237763e in main_window::Boot(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, bool, bool, bool) /mnt/Data/src/rpcs3-git/src/rpcs3/rpcs3/rpcs3qt/main_window.cpp:247
    #18 0x237a85c in main_window::BootRecentAction(QAction const*) /mnt/Data/src/rpcs3-git/src/rpcs3/rpcs3/rpcs3qt/main_window.cpp:988
    #19 0x237c16d in main_window::OnPlayOrPause() /mnt/Data/src/rpcs3-git/src/rpcs3/rpcs3/rpcs3qt/main_window.cpp:223
    #20 0x23088d1 in QtPrivate::FunctorCall<QtPrivate::IndexesList<>, QtPrivate::List<>, void, void (main_window::*)()>::call(void (main_window::*)(), main_window*, void**) /usr/include/qt/QtCore/qobjectdefs_impl.h:152
    #21 0x23088d1 in void QtPrivate::FunctionPointer<void (main_window::*)()>::call<QtPrivate::List<>, void>(void (main_window::*)(), main_window*, void**) /usr/include/qt/QtCore/qobjectdefs_impl.h:185
    #22 0x23088d1 in QtPrivate::QSlotObject<void (main_window::*)(), QtPrivate::List<>, void>::impl(int, QtPrivate::QSlotObjectBase*, QObject*, void**, bool*) /usr/include/qt/QtCore/qobjectdefs_impl.h:414
    #23 0x23088d1 in QtPrivate::QSlotObject<void (main_window::*)(), QtPrivate::List<>, void>::impl(int, QtPrivate::QSlotObjectBase*, QObject*, void**, bool*) /usr/include/qt/QtCore/qobjectdefs_impl.h:407
    #24 0x7fd729b00b6f in QMetaObject::activate(QObject*, int, int, void**) (/usr/lib/libQt5Core.so.5+0x2bcb6f)
    #25 0x7fd72abb9eb2 in QAction::triggered(bool) (/usr/lib/libQt5Widgets.so.5+0x152eb2)

SUMMARY: AddressSanitizer: heap-use-after-free /mnt/Data/src/rpcs3-git/src/rpcs3/rpcs3/Emu/RSX/VK/VKResolveHelper.cpp:106 in vk::resolve_image(vk::command_buffer&, vk::viewable_image*, vk::viewable_image*)
Shadow bytes around the buggy address:
  0x0c24800085d0: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x0c24800085e0: fd fd fd fd fd fd fd fd fd fd fd fd fa fa fa fa
  0x0c24800085f0: fa fa fa fa fa fa fa fa 00 00 00 00 00 00 00 00
  0x0c2480008600: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c2480008610: 00 00 00 00 00 00 00 00 00 00 00 00 00 fa fa fa
=>0x0c2480008620: fa fa fa fa fa fa fa fa fd fd fd fd fd fd fd[fd]
  0x0c2480008630: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x0c2480008640: fd fd fd fd fd fd fd fd fd fa fa fa fa fa fa fa
  0x0c2480008650: fa fa fa fa fa fa fa fa fd fd fd fd fd fd fd fd
  0x0c2480008660: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x0c2480008670: fd fd fd fd fd fd fd fd fd fa fa fa fa fa fa 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
  Shadow gap:              cc
==73637==ABORTING
kd-11 commented 5 years ago

Yea, that line shouldn't be there. It was left over from some earlier implementation. As for why I don't use dynamic cast all the time - it has (or used to have?) an annoying requirement that the derived must override some method in the base class which often times makes no sense when the derived is a pure extension of the base. I found this intrusive enough to avoid using it.

kd-11 commented 5 years ago

Should be fixed by https://github.com/RPCS3/rpcs3/pull/6784