search

versatica / mediasoup

Cutting Edge WebRTC Video Conferencing
https://mediasoup.org
ISC License
6.18k stars 1.12k forks source link

Fix rtp packet test #1318

Closed penguinol closed 2 months ago

ibc commented 8 months ago

What is this fixing exactly? I've never seen those tests failing due to memory problems.

penguinol commented 8 months ago

What is this fixing exactly? I've never seen those tests failing due to memory problems.

When i ran tests under windows, it says buffer is overflow. Maybe u can see the warning with AddressSanitzer or valgrind. In these tests, packet size will be 52 after setting excetions, so we need to make sure buffer is large enough.

nazar-pc commented 8 months ago

When i ran tests under windows, it says buffer is overflow. Maybe u can see the warning with AddressSanitzer or valgrind. In these tests, packet size will be 52 after setting excetions, so we need to make sure buffer is large enough.

We do run tests in CI on Windows too :thinking:

Either way, that is a useful details and exactly the kind of information that should be included in PR description from the beginning.

ibc commented 8 months ago

I think we have a test-asan task in Makefile/tasks.py. Let me please test it in 10 days when I'm back before we merge this PR just to confirm.

penguinol commented 8 months ago 
(mediasoup_buildvenv) [root@784dfa00-aa18-11ee-bad9-a4bf015565d5 worker]# ./out/Debug/mediasoup-worker-test-asan
Randomness seeded to: 446808779
=================================================================
==2099730==ERROR: AddressSanitizer: stack-buffer-overflow on address 0x7fff7ac12c80 at pc 0x7f231efafe7e bp 0x7fff7abf9940 sp 0x7fff7abf90f0
WRITE of size 16 at 0x7fff7ac12c80 thread T0
    #0 0x7f231efafe7d in memmove (/opt/sysroot64/lib/libasan.so.6+0x41e7d)
    #1 0x66d521 in RTC::RtpPacket::SetExtensions(unsigned char, std::vector<RTC::RtpPacket::GenericExtension, std::allocator<RTC::RtpPacket::GenericExtension> > const&) ../../../src/RTC/RtpPacket.cpp:471
    #2 0x9381bd in CATCH2_INTERNAL_TEST_0 ../../../test/src/RTC/TestRtpPacket.cpp:605
    #3 0xf1395d in invoke ../../../subprojects/Catch2-3.4.0/src/catch2/internal/catch_test_registry.cpp:58
    #4 0xf08a94 in Catch::TestCaseHandle::invoke() const ../../../subprojects/Catch2-3.4.0/src/catch2/catch_test_case_info.hpp:115
    #5 0xf078ac in Catch::RunContext::invokeActiveTestCase() ../../../subprojects/Catch2-3.4.0/src/catch2/internal/catch_run_context.cpp:552
    #6 0xf075fb in Catch::RunContext::runCurrentTest(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >&) ../../../subprojects/Catch2-3.4.0/src/catch2/internal/catch_run_context.cpp:515
    #7 0xf05cc7 in Catch::RunContext::runTest(Catch::TestCaseHandle const&) ../../../subprojects/Catch2-3.4.0/src/catch2/internal/catch_run_context.cpp:239
    #8 0xf1a773 in execute ../../../subprojects/Catch2-3.4.0/src/catch2/catch_session.cpp:111
    #9 0xf1bb3f in Catch::Session::runInternal() ../../../subprojects/Catch2-3.4.0/src/catch2/catch_session.cpp:333
    #10 0xf1b681 in Catch::Session::run() ../../../subprojects/Catch2-3.4.0/src/catch2/catch_session.cpp:264
    #11 0x8f528d in int Catch::Session::run<char>(int, char const* const*) ../../../subprojects/Catch2-3.4.0/src/catch2/catch_session.hpp:41
    #12 0x8f4f9a in main ../../../test/src/tests.cpp:56
    #13 0x7f231ea1e86c in __libc_start_main (/opt/sysroot64/lib/libc.so.6+0x2386c)
    #14 0x413029 in _start (/root/mediasoup-fix_tcc_feedback/worker/out/Debug/mediasoup-worker-test-asan+0x413029)
penguinol commented 8 months ago

By the way, when i ran mediasoup_test_asan release version, it show:

AddressSanitizer:DEADLYSIGNAL
=================================================================
==2092397==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x0000004e4d91 bp 0x00000000001f sp 0x7ffe438c49e0 T0)
==2092397==The signal is caused by a READ memory access.
==2092397==Hint: address points to the zero page.
    #0 0x4e4d91 in void absl::lts_20230802::container_internal::InitializeSlots<std::allocator<char>, 40ul, 8ul>(absl::lts_20230802::container_internal::CommonFields&, std::allocator<char>) [clone .isra.0] (/root/mediasoup-fix_tcc_feedback/worker/out/Release/mediasoup-worker-test-asan+0x4e4d91)
    #1 0x4f4def in absl::lts_20230802::container_internal::raw_hash_set<absl::lts_20230802::container_internal::FlatHashMapPolicy<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, LogLevel>, absl::lts_20230802::container_internal::StringHash, absl::lts_20230802::container_internal::StringEq, std::allocator<std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const, LogLevel> > >::raw_hash_set(unsigned long, absl::lts_20230802::container_internal::StringHash const&, absl::lts_20230802::container_internal::StringEq const&, std::allocator<std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const, LogLevel> > const&) (/root/mediasoup-fix_tcc_feedback/worker/out/Release/mediasoup-worker-test-asan+0x4f4def)
    #2 0x4a4411 in __static_initialization_and_destruction_0(int, int) [clone .constprop.0] (/root/mediasoup-fix_tcc_feedback/worker/out/Release/mediasoup-worker-test-asan+0x4a4411)
    #3 0x121fab4 in __libc_csu_init (/root/mediasoup-fix_tcc_feedback/worker/out/Release/mediasoup-worker-test-asan+0x121fab4)
    #4 0x7faa50b2d7fc in __libc_start_main (/opt/sysroot64/lib/libc.so.6+0x237fc)
    #5 0x4d0af9 in _start (/root/mediasoup-fix_tcc_feedback/worker/out/Release/mediasoup-worker-test-asan+0x4d0af9)

AddressSanitizer can not provide additional info.
SUMMARY: AddressSanitizer: SEGV (/root/mediasoup-fix_tcc_feedback/worker/out/Release/mediasoup-worker-test-asan+0x4e4d91) in void absl::lts_20230802::container_internal::InitializeSlots<std::allocator<char>, 40ul, 8ul>(absl::lts_20230802::container_internal::CommonFields&, std::allocator<char>) [clone .isra.0]
==2092397==ABORTING

But debug version won't.

penguinol commented 8 months ago

This PR fix the same problem as https://github.com/versatica/mediasoup/pull/1120

ibc commented 3 months ago

@penguinol, here a PR adding ASAN to Node tests: https://github.com/versatica/mediasoup/pull/1415

Perhaps we should have the same in worker tests?

ibc commented 3 months ago

@penguinol, here a PR adding ASAN to Node tests: #1415

Perhaps we should have the same in worker tests?

Actually we already have make/invoke test-asan...

ibc commented 3 months ago

@penguinol: https://github.com/versatica/mediasoup/pull/1416

ibc commented 2 months ago

@penguinol I don't see that changes in this PR fix the ASAN issues related to RtpPacket. Probably unrelated but those are the issues I get:

https://github.com/versatica/mediasoup/pull/1416#issuecomment-2199756593

ibc commented 2 months ago

@penguinol we are addressing these and other ASAN issues in this PR: https://github.com/versatica/mediasoup/pull/1416#issuecomment-2199897238

And yes, changes include those in your PR. So thanks.