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

[Core]Fix coredump when m_pCryptoControl is Null #1193

Closed
wants to merge 3 commits into from

Conversation

kyolong
Copy link
Contributor

@kyolong kyolong commented Mar 20, 2020

#1182
maybe fixed ~

@maxsharabayko maxsharabayko added this to the v1.5.0 milestone Mar 20, 2020
@maxsharabayko maxsharabayko added [core] Area: Changes in SRT library core Type: Bug Indicates an unexpected problem or unintended behavior labels Mar 20, 2020
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.

By the time this function CUDT::packData(..) is called, the m_pCryptoControl should exist. However, if you somehow manage to close a socket while being in this function, the m_pCryptoControl might have been destroyed in another thread.
It looks like something is wrong with mutexes protecting both cases.

@ethouris
Copy link
Collaborator

I'd keenly see the debug log - it will be cleanly seen in what order things happen and therefore how it was possible that packData was called at all on a socket being closed. AFAIR there should be first the m_bClosing and m_bBroken flags set first, then the crypto object deleted. And every function that would do any next thing with the data should first check these closing/broken flags before doing anything.

@ethouris
Copy link
Collaborator

ethouris commented Mar 20, 2020

My suggestion:

For testing, keep this change, but in the condition when !m_pCryptoControl add a log message so that you can see that it has happened, but print also the state. Something like:

LOGC(mglog.Error, log << "CryptoControl DELETED. connected=" << m_bConnected << " broken=" << m_bBroken << " opened=" << m_bOpened);

When you can see this log during testing, please see if when it happens the broken flag is set and possibly also connected flag is clear. If it is so, it means that the code works still correctly predictably, so the proper fix should be:

if (m_bBroken || !m_bConnected)
{
       return std::make_pair(0, enter_time);
}

in the beginning of packData method (at least after enter_time was assigned). With that, checking for m_pCryptoControl shall be no longer necessary.

kyolong and others added 2 commits March 25, 2020 20:22
This reverts commit 06ddfc9.
@maxsharabayko
Copy link
Collaborator

Closing as fixed by #1315

@mbakholdina mbakholdina modified the milestones: v1.5.0, v1.4.2 Oct 14, 2020
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: Bug Indicates an unexpected problem or unintended behavior
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants