Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix the issue where Spine crashes due to container scaling. #17570

Merged
merged 1 commit into from
Aug 21, 2024

Conversation

bofeng-song
Copy link
Contributor

@bofeng-song bofeng-song commented Aug 21, 2024

Re: #
https://forum.cocos.org/t/topic/160784/3

Changelog


Continuous Integration

This pull request:

  • needs automatic test cases check.

    Manual trigger with @cocos-robot run test cases afterward.

  • does not change any runtime related code or build configuration

    If any reviewer thinks the CI checks are needed, please uncheck this option, then close and reopen the issue.


Compatibility Check

This pull request:

  • changes public API, and have ensured backward compatibility with deprecated features.
  • affects platform compatibility, e.g. system version, browser version, platform sdk version, platform toolchain, language version, hardware compatibility etc.
  • affects file structure of the build package or build configuration which requires user project upgrade.
  • introduces breaking changes, please list all changes, affected features and the scope of violation.

Greptile Summary

This pull request modifies the MiddlewareManager class in the Cocos engine to address a potential crash issue related to Spine and container scaling.

  • Modified native/cocos/editor-support/MiddlewareManager.cpp to replace range-based for loops with index-based loops in update and render methods
  • Change aims to prevent crashes that may occur due to container scaling during iteration
  • Affects the core functionality of the MiddlewareManager, which is crucial for managing middleware components
  • Potential performance impact due to loop structure change, though likely minimal
  • Improves stability for projects using Spine with scaled containers

@dumganhar
Copy link
Contributor

@cocos-robot run test cases

Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

1 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings

Copy link

Interface Check Report

This pull request does not change any public interfaces !

@bofeng-song
Copy link
Contributor Author

In addTimer, call shrink_to_fit to ensure that the container's capacity matches its size.
Test case: In skeleton.setCompleteListener, enable and disable other skeleton components. After that, the crash shown in the screenshot occurs.

image

image

@bofeng-song
Copy link
Contributor Author

test-case code:
image

@dumganhar
Copy link
Contributor

dumganhar commented Aug 21, 2024

Minimal code that reproduces the issue:

#include <vector>

int main(int argc, const char * argv[]) {
    std::vector<int> arr{0, 1,2, 3, 4, 5, 6, 7, 8, 9, 10};
    for (auto & e: arr) {
        if (e == 5) {
            arr.push_back(11);
        }
        printf("%d, ", e);
    }
    
    printf("\n");
    
    return 0;
}

Enable AddressSanitizer:

0, 1, 2, 3, 4, =================================================================
==57863==ERROR: AddressSanitizer: heap-use-after-free on address 0x000102f015e4 at pc 0x000100001374 bp 0x00016fdfee90 sp 0x00016fdfee88
READ of size 4 at 0x000102f015e4 thread T0
    #0 0x100001370 in main+0x5a8 (/Users/james/Library/Developer/Xcode/DerivedData/TestForLoopAddRemove-cwjutrmbxlwuljfcrfbeytrermdx/Build/Products/Debug/TestForLoopAddRemove:arm64+0x100001370)
    #1 0x19f1ce0dc  (<unknown module>)

0x000102f015e4 is located 20 bytes inside of 44-byte region [0x000102f015d0,0x000102f015fc)
freed by thread T0 here:
    #0 0x1008d9b8c in wrap__ZdlPv+0x74 (/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/lib/clang/15.0.0/lib/darwin/libclang_rt.asan_osx_dynamic.dylib:arm64e+0x61b8c)
    #1 0x10000a8f4 in void std::__1::__libcpp_operator_delete[abi:ue170006]<void*>(void*)+0x14 (/Users/james/Library/Developer/Xcode/DerivedData/TestForLoopAddRemove-cwjutrmbxlwuljfcrfbeytrermdx/Build/Products/Debug/TestForLoopAddRemove:arm64+0x10000a8f4)
    #2 0x10000a8a4 in void std::__1::__do_deallocate_handle_size[abi:ue170006]<>(void*, unsigned long)+0x18 (/Users/james/Library/Developer/Xcode/DerivedData/TestForLoopAddRemove-cwjutrmbxlwuljfcrfbeytrermdx/Build/Products/Debug/TestForLoopAddRemove:arm64+0x10000a8a4)
    #3 0x10000a848 in std::__1::__libcpp_deallocate[abi:ue170006](void*, unsigned long, unsigned long)+0x4c (/Users/james/Library/Developer/Xcode/DerivedData/TestForLoopAddRemove-cwjutrmbxlwuljfcrfbeytrermdx/Build/Products/Debug/TestForLoopAddRemove:arm64+0x10000a848)
    #4 0x10000a7d8 in std::__1::allocator<int>::deallocate[abi:ue170006](int*, unsigned long)+0x54 (/Users/james/Library/Developer/Xcode/DerivedData/TestForLoopAddRemove-cwjutrmbxlwuljfcrfbeytrermdx/Build/Products/Debug/TestForLoopAddRemove:arm64+0x10000a7d8)
    #5 0x10000a424 in std::__1::allocator_traits<std::__1::allocator<int>>::deallocate[abi:ue170006](std::__1::allocator<int>&, int*, unsigned long)+0x24 (/Users/james/Library/Developer/Xcode/DerivedData/TestForLoopAddRemove-cwjutrmbxlwuljfcrfbeytrermdx/Build/Products/Debug/TestForLoopAddRemove:arm64+0x10000a424)
    #6 0x10001230c in std::__1::__split_buffer<int, std::__1::allocator<int>&>::~__split_buffer()+0x144 (/Users/james/Library/Developer/Xcode/DerivedData/TestForLoopAddRemove-cwjutrmbxlwuljfcrfbeytrermdx/Build/Products/Debug/TestForLoopAddRemove:arm64+0x10001230c)
    #7 0x10000c8dc in std::__1::__split_buffer<int, std::__1::allocator<int>&>::~__split_buffer()+0x64 (/Users/james/Library/Developer/Xcode/DerivedData/TestForLoopAddRemove-cwjutrmbxlwuljfcrfbeytrermdx/Build/Products/Debug/TestForLoopAddRemove:arm64+0x10000c8dc)
    #8 0x10000b76c in void std::__1::vector<int, std::__1::allocator<int>>::__push_back_slow_path<int>(int&&)+0x444 (/Users/james/Library/Developer/Xcode/DerivedData/TestForLoopAddRemove-cwjutrmbxlwuljfcrfbeytrermdx/Build/Products/Debug/TestForLoopAddRemove:arm64+0x10000b76c)
    #9 0x100002130 in std::__1::vector<int, std::__1::allocator<int>>::push_back[abi:ue170006](int&&)+0x1e8 (/Users/james/Library/Developer/Xcode/DerivedData/TestForLoopAddRemove-cwjutrmbxlwuljfcrfbeytrermdx/Build/Products/Debug/TestForLoopAddRemove:arm64+0x100002130)
    #10 0x1000012e0 in main+0x518 (/Users/james/Library/Developer/Xcode/DerivedData/TestForLoopAddRemove-cwjutrmbxlwuljfcrfbeytrermdx/Build/Products/Debug/TestForLoopAddRemove:arm64+0x1000012e0)
    #11 0x19f1ce0dc  (<unknown module>)

previously allocated by thread T0 here:
    #0 0x1008d974c in wrap__Znwm+0x74 (/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/lib/clang/15.0.0/lib/darwin/libclang_rt.asan_osx_dynamic.dylib:arm64e+0x6174c)
    #1 0x100005da8 in void* std::__1::__libcpp_operator_new[abi:ue170006]<unsigned long>(unsigned long)+0x14 (/Users/james/Library/Developer/Xcode/DerivedData/TestForLoopAddRemove-cwjutrmbxlwuljfcrfbeytrermdx/Build/Products/Debug/TestForLoopAddRemove:arm64+0x100005da8)
    #2 0x100005cec in std::__1::__libcpp_allocate[abi:ue170006](unsigned long, unsigned long)+0x44 (/Users/james/Library/Developer/Xcode/DerivedData/TestForLoopAddRemove-cwjutrmbxlwuljfcrfbeytrermdx/Build/Products/Debug/TestForLoopAddRemove:arm64+0x100005cec)
    #3 0x100005c1c in std::__1::allocator<int>::allocate[abi:ue170006](unsigned long)+0xbc (/Users/james/Library/Developer/Xcode/DerivedData/TestForLoopAddRemove-cwjutrmbxlwuljfcrfbeytrermdx/Build/Products/Debug/TestForLoopAddRemove:arm64+0x100005c1c)
    #4 0x100004ac4 in std::__1::__allocation_result<std::__1::allocator_traits<std::__1::allocator<int>>::pointer> std::__1::__allocate_at_least[abi:ue170006]<std::__1::allocator<int>>(std::__1::allocator<int>&, unsigned long)+0x124 (/Users/james/Library/Developer/Xcode/DerivedData/TestForLoopAddRemove-cwjutrmbxlwuljfcrfbeytrermdx/Build/Products/Debug/TestForLoopAddRemove:arm64+0x100004ac4)
    #5 0x100002f7c in std::__1::vector<int, std::__1::allocator<int>>::__vallocate[abi:ue170006](unsigned long)+0x1f4 (/Users/james/Library/Developer/Xcode/DerivedData/TestForLoopAddRemove-cwjutrmbxlwuljfcrfbeytrermdx/Build/Products/Debug/TestForLoopAddRemove:arm64+0x100002f7c)
    #6 0x10000272c in std::__1::vector<int, std::__1::allocator<int>>::vector[abi:ue170006](std::initializer_list<int>)+0x3c8 (/Users/james/Library/Developer/Xcode/DerivedData/TestForLoopAddRemove-cwjutrmbxlwuljfcrfbeytrermdx/Build/Products/Debug/TestForLoopAddRemove:arm64+0x10000272c)
    #7 0x100001784 in std::__1::vector<int, std::__1::allocator<int>>::vector[abi:ue170006](std::initializer_list<int>)+0x1f4 (/Users/james/Library/Developer/Xcode/DerivedData/TestForLoopAddRemove-cwjutrmbxlwuljfcrfbeytrermdx/Build/Products/Debug/TestForLoopAddRemove:arm64+0x100001784)
    #8 0x100001048 in main+0x280 (/Users/james/Library/Developer/Xcode/DerivedData/TestForLoopAddRemove-cwjutrmbxlwuljfcrfbeytrermdx/Build/Products/Debug/TestForLoopAddRemove:arm64+0x100001048)
    #9 0x19f1ce0dc  (<unknown module>)

SUMMARY: AddressSanitizer: heap-use-after-free (/Users/james/Library/Developer/Xcode/DerivedData/TestForLoopAddRemove-cwjutrmbxlwuljfcrfbeytrermdx/Build/Products/Debug/TestForLoopAddRemove:arm64+0x100001370) in main+0x5a8
Shadow bytes around the buggy address:
  0x000102f01300: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x000102f01380: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x000102f01400: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x000102f01480: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x000102f01500: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
=>0x000102f01580: fa fa fa fa fa fa fa fa fa fa fd fd[fd]fd fd fd
  0x000102f01600: fa fa fd fd fd fd fd fd fa fa 00 00 00 00 00 00
  0x000102f01680: fa fa 00 00 00 00 00 00 fa fa 00 00 00 00 00 00
  0x000102f01700: fa fa 00 00 00 00 00 00 fa fa 00 00 00 00 00 00
  0x000102f01780: fa fa 00 00 00 00 00 00 fa fa 00 00 00 00 00 00
  0x000102f01800: fa fa 00 00 00 00 00 00 fa fa 00 00 00 00 00 00
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
==57863==ABORTING

Copy link

@bofeng-song, Please check the result of run test cases:

Task Details

Platform build boot runned crashScene FailScene
web-mobile PASS PASS PASS
ios PASS PASS PASS
mac PASS PASS FAIL SpineMesh

Copy link

@bofeng-song, Please check the result of run test cases:

Task Details

@minggo minggo merged commit f263d40 into cocos:v3.8.4 Aug 21, 2024
24 checks passed
@dumganhar
Copy link
Contributor

dumganhar commented Aug 23, 2024

This bug appeared from v3.6.0 in PR ( minggo/engine-native#442 )

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants