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

[REFAX] Refactored function calls at close socket for preparing a change #2639

Open
wants to merge 8 commits into
base: dev
Choose a base branch
from

Conversation

ethouris
Copy link
Collaborator

No description provided.

@ethouris ethouris added Type: Maintenance Work required to maintain or clean up the code [core] Area: Changes in SRT library core labels Jan 31, 2023
@maxsharabayko maxsharabayko added this to the v1.5.2 milestone Jan 31, 2023
@maxsharabayko
Copy link
Collaborator

Extracted from #1829.

@ethouris ethouris marked this pull request as draft February 1, 2023 16:00
@ethouris
Copy link
Collaborator Author

ethouris commented Feb 1, 2023

Careful. This last fix has caused tests to crash.

@ethouris
Copy link
Collaborator Author

ethouris commented Feb 3, 2023

Now the refax should be ok, but all attempts to provide a reliable closing a socket have failed, so this refax is useless.

Copy link
Collaborator

@maxsharabayko maxsharabayko left a comment

Choose a reason for hiding this comment

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

I am concerned with this function because it provides not much hints of what it does internally.
It notably comes from the overall mess with socket lifetime management.

There is m_ClosedSockets for sockets to be closed and m_Sockets for active sockets.
A socket can sometimes be present in both lists for a short time.
swipeSocket_LOCKED(i->first, s, SWIPE_LATER) with SWIPE_LATER is only used in checkBrokenSockets(..). All other places use SWIPE_NOW. Does it make sense to have a function that does both?

newConnection error, srt_close, removeSocket -> SWIPE_NOW.
checkBrokenSockets(..), srt_cleanup -> SWIPE_LATER.

@ethouris
Copy link
Collaborator Author

The SWIPE_NOW and SWIPE_LATER differ only with that when SWIPE_LATER is used, then the call will be followed by a loop that will delete all sockets that were collected in the temporary list. So the socket being worked about at this call need not be swiped at this moment.

@maxsharabayko maxsharabayko modified the milestones: v1.5.2, v1.6.0 Feb 10, 2023
Co-authored-by: Maxim Sharabayko <maxlovic@gmail.com>
Copy link

codecov bot commented Feb 21, 2024

Codecov Report

Attention: Patch coverage is 86.66667% with 2 lines in your changes missing coverage. Please review.

Project coverage is 70.02%. Comparing base (53a09f3) to head (a1b84c7).
Report is 263 commits behind head on dev.

Files with missing lines Patch % Lines
srtcore/api.cpp 86.66% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##              dev    #2639      +/-   ##
==========================================
+ Coverage   69.38%   70.02%   +0.64%     
==========================================
  Files         101      101              
  Lines       18729    19522     +793     
==========================================
+ Hits        12995    13671     +676     
- Misses       5734     5851     +117     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@maxsharabayko maxsharabayko modified the milestones: v1.5.4, v1.6.0 Aug 5, 2024
@ethouris ethouris mentioned this pull request Mar 13, 2025
30 tasks
@ethouris ethouris changed the base branch from master to dev March 24, 2025 09:43
@ethouris
Copy link
Collaborator Author

Crash found (address sanitizer):

==9685==ERROR: AddressSanitizer: heap-use-after-free on address 0x52800001a6f8 at pc 0x0000007f4c7e bp 0x7f0404dfb960 sp 0x7f0404dfb958
WRITE of size 1 at 0x52800001a6f8 thread T1
    #0 0x7f4c7d in std::__atomic_base<bool>::store(bool, std::memory_order) /usr/include/c++/14/bits/atomic_base.h:477
    #1 0x7f4c7d in std::atomic<bool>::store(bool, std::memory_order) /usr/include/c++/14/atomic:109
    #2 0x7f4c7d in srt::sync::atomic<bool>::store(bool) /home/sektor/repos/srt.gitd/main/srtcore/atomic.h:243
    #3 0x7f4c7d in srt::CUnitQueue::makeUnitFree(srt::CUnit*) /home/sektor/repos/srt.gitd/main/srtcore/queue.cpp:192
    #4 0x5e8fd8 in srt::CRcvBuffer::~CRcvBuffer() /home/sektor/repos/srt.gitd/main/srtcore/buffer_rcv.cpp:132
    #5 0x62c86c in srt::CUDT::~CUDT() /home/sektor/repos/srt.gitd/main/srtcore/core.cpp:395
    #6 0x58280e in srt::CUDTUnited::removeSocket(int) /home/sektor/repos/srt.gitd/main/srtcore/api.cpp:3264
    #7 0x599f55 in srt::CUDTUnited::checkBrokenSockets() /home/sektor/repos/srt.gitd/main/srtcore/api.cpp:3155
    #8 0x5a0132 in srt::CUDTUnited::garbageCollect(void*) /home/sektor/repos/srt.gitd/main/srtcore/api.cpp:3802
    #9 0x7f0408330ff5  (/usr/lib64/libasan.so.8+0x5eff5) (BuildId: 4ee117fa2a132af1da9f17a0a5fe1f888398d50f)
    #10 0x7f0407ed46e9 in start_thread (/lib64/libpthread.so.0+0xa6e9) (BuildId: 938e42b7e407d175ee3ef9a89c038168101d330c)
    #11 0x7f0407a1258e in clone (/lib64/libc.so.6+0x11858e) (BuildId: 74f77bf013a66413c77197c121955e029c32d259)

0x52800001a6f8 is located 9720 bytes inside of 15368-byte region [0x528000018100,0x52800001bd08)
freed by thread T1 here:
    #0 0x7f04083cae08 in operator delete[](void*, unsigned long) (/usr/lib64/libasan.so.8+0xf8e08) (BuildId: 4ee117fa2a132af1da9f17a0a5fe1f888398d50f)
    #1 0x7f3274 in srt::CUnitQueue::~CUnitQueue() /home/sektor/repos/srt.gitd/main/srtcore/queue.cpp:92

previously allocated by thread T0 (srt-live-transm) here:
    #0 0x7f04083c9f38 in operator new[](unsigned long) (/usr/lib64/libasan.so.8+0xf7f38) (BuildId: 4ee117fa2a132af1da9f17a0a5fe1f888398d50f)
    #1 0x7f34d6 in srt::CUnitQueue::allocateEntry(int, int) /home/sektor/repos/srt.gitd/main/srtcore/queue.cpp:113

Thread T1 created by T0 (srt-live-transm) here:
    #0 0x7f04083c1191 in pthread_create (/usr/lib64/libasan.so.8+0xef191) (BuildId: 4ee117fa2a132af1da9f17a0a5fe1f888398d50f)
    #1 0x8fd4b4 in srt::sync::CThread::create(void* (*)(void*), void*) /home/sektor/repos/srt.gitd/main/srtcore/sync_posix.cpp:436

@ethouris
Copy link
Collaborator Author

Seems like all previous problems have been solved. Clearing draft state.

@ethouris ethouris marked this pull request as ready for review March 25, 2025 07:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[core] Area: Changes in SRT library core Type: Maintenance Work required to maintain or clean up the code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants