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

Fixed race condition: SocketException due to mySock closed while sending response #174

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

fre42
Copy link
Contributor

@fre42 fre42 commented Feb 9, 2018

We have a scenario where a load balancer (Linux Virtual Server - LVS) sends SIP OPTIONS every 5 seconds via TCP.
When receiving this OPTIONS, our application (using this SIP stack) sends 200 OK and the LVS immediately closes the TCP connection after having received it.
Im some cases the socket close is very fast and comes into the TCPMessageChannel while sending the 200 OK in method sendMessage is still in progress.
Unfortunately the socket close will set the mySock member to null while the sendMessage method is still accessing it.
This can happen, because the socket handling is not thread safe.
sendMessage will then run into the situation that it overwrites the mySock with the current send socket sock (line 297).
In our LVS case sock and mySock are the same because LVS puts it's own TCP port into the Via header and no additional TCP connection needs to be established for sending the response.
As a result accessing the overwritten mySock causes a SocketException.

My suggested fix will avoid accessing the live mySock member by making a copy of it in the beginning of sendMessage and checks afterwards, if the copy is still identical to mySock.
This avoids the problem we have but the socket handling is still not thread safe and needs further refactoring in the future.

sip-race-condition.log

@fre42
Copy link
Contributor Author

fre42 commented Feb 9, 2018

I know that signing the CLA is still missing. I'll do it as soon I get the OK from my company.

@gsaslis
Copy link
Contributor

gsaslis commented Feb 23, 2018

@fre42 plz also ping back here once CLA process is done ; )

thanks!!

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.

2 participants