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.

Sign up for GitHub

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

Jump to bottom

Prevent race conditions on socket close in SIP and NoSIP plugins #2599

Merged
merged 3 commits into from
Apr 28, 2021
Merged

Prevent race conditions on socket close in SIP and NoSIP plugins #2599

merged 3 commits into from
Apr 28, 2021

Conversation

lminiero
Copy link
Member

We noticed that in some cases, in misconfigured instances of Janus with high socket contention and high traffic, a race condition could happen where a socket would be closed twice. This is an issue whenever the second time we close a socket it was actually assigned to someone else in the meanwhile: if the socket was used internally by getifaddrs, for instance, this could cause that method to lock and never return, and since in Janus that call may be on a critical path, deadlock relevant functionality.

This PR tries to fix this behaviour in SIP and NoSIP plugins, as the SIP plugin is where we noticed this happening, and the NoSIP plugin borrows a lot of the same code. The Streaming plugin doesn't seem to be affected instead, as only a mountpoint thread messes with the struct file descriptor values.

Feedback welcome, especially if you use those plugins a lot in production.

lminiero added a commit that referenced this pull request Mar 25, 2021
@lminiero
Added mutex around socket close (see #2599)
a73f20d
@bhakimi
Copy link

bhakimi commented Mar 25, 2021 •
edited
Loading

thanks a ton!!!! @lminiero we will give it a shot and report back

@lminiero
Copy link
Member Author

@ihusejnovic since I know you guys use the SIP plugin a lot, we'd welcome feedback on this PR: we'd like to merge it soon. Thx!

@ihusejnovic
Copy link
Contributor

Sure, we will test it. Can you just sync this branch with master?

@lminiero
Copy link
Member Author

Good point, done! Thanks 😃

@ihusejnovic
Copy link
Contributor

We are using this branch last few days and we didn't notice any issues so far.

@lminiero
Copy link
Member Author

Merging.

@lminiero lminiero merged commit ac0db0a into master Apr 28, 2021
@lminiero lminiero deleted the fd-mutex branch April 28, 2021 10:11
heavyrain2012 pushed a commit to heavyrain2012/janus-gateway that referenced this pull request Apr 29, 2021
@imhao183
Merge branch 'meetecho_master'
262e4c7 
* meetecho_master: (345 commits)
  Remove support for framemarking RTP extension (meetecho#2640)
  Prevent race conditions on socket close in SIP and NoSIP plugins (meetecho#2599)
  Fixed overflow runtime error
  Fix for race condition between VideoRoom publisher leaving and subscriber hanging up (fixes meetecho#2582) (meetecho#2637)
  Send PLI when starting a paused stream (meetecho#2645)
  Prevent too high shift exponent
  Fixed type of seq/ts in file-based Streaming mountpoint threads
  Fix missing g_thread_unref when a streaming helper thread quits.
  Added custom headers for SIP INFO request (meetecho#2644)
  Free participant->user_id_str in case of opus enc/decoder error.
  Reject flexfec when offered, as still unsupported (see meetecho#2639)
  Don't add rtx ssrc if m-line is recvonly/inactive (see meetecho#2639)
  Added NULL checks for json_dumps (see meetecho#2629)
  Parse custom headers, if required, in successful REGISTER response (fixes meetecho#2636)
  Timestamp correction for janus-pp-rec (meetecho#2573)
  Don't chain error handler to success handler in Janus.httpAPICall (meetecho#2569)
  Add missing library link for WS event handler (fixes meetecho#2628)
  Fixed broken switch in Streaming plugin when using helper threads
  Resolves meetecho#2624 jansson double referencing (meetecho#2634)
  Unlock mountpoints mutex after the spawning of helper threads.
  ...

# Conflicts:
#	transports/janus_mqtt.c
heavyrain2012 pushed a commit to heavyrain2012/janus-gateway that referenced this pull request Jul 29, 2021
@imhao183 @heavyrain2012
Merge branch 'meetecho_master'
3529b8d 
* meetecho_master: (345 commits)
  Remove support for framemarking RTP extension (meetecho#2640)
  Prevent race conditions on socket close in SIP and NoSIP plugins (meetecho#2599)
  Fixed overflow runtime error
  Fix for race condition between VideoRoom publisher leaving and subscriber hanging up (fixes meetecho#2582) (meetecho#2637)
  Send PLI when starting a paused stream (meetecho#2645)
  Prevent too high shift exponent
  Fixed type of seq/ts in file-based Streaming mountpoint threads
  Fix missing g_thread_unref when a streaming helper thread quits.
  Added custom headers for SIP INFO request (meetecho#2644)
  Free participant->user_id_str in case of opus enc/decoder error.
  Reject flexfec when offered, as still unsupported (see meetecho#2639)
  Don't add rtx ssrc if m-line is recvonly/inactive (see meetecho#2639)
  Added NULL checks for json_dumps (see meetecho#2629)
  Parse custom headers, if required, in successful REGISTER response (fixes meetecho#2636)
  Timestamp correction for janus-pp-rec (meetecho#2573)
  Don't chain error handler to success handler in Janus.httpAPICall (meetecho#2569)
  Add missing library link for WS event handler (fixes meetecho#2628)
  Fixed broken switch in Streaming plugin when using helper threads
  Resolves meetecho#2624 jansson double referencing (meetecho#2634)
  Unlock mountpoints mutex after the spawning of helper threads.
  ...

# Conflicts:
#	transports/janus_mqtt.c
heavyrain2012 pushed a commit to heavyrain2012/janus-gateway that referenced this pull request Jul 29, 2021
@imhao183 @heavyrain2012
Merge branch 'meetecho_master'
feec490 
* meetecho_master: (345 commits)
  Remove support for framemarking RTP extension (meetecho#2640)
  Prevent race conditions on socket close in SIP and NoSIP plugins (meetecho#2599)
  Fixed overflow runtime error
  Fix for race condition between VideoRoom publisher leaving and subscriber hanging up (fixes meetecho#2582) (meetecho#2637)
  Send PLI when starting a paused stream (meetecho#2645)
  Prevent too high shift exponent
  Fixed type of seq/ts in file-based Streaming mountpoint threads
  Fix missing g_thread_unref when a streaming helper thread quits.
  Added custom headers for SIP INFO request (meetecho#2644)
  Free participant->user_id_str in case of opus enc/decoder error.
  Reject flexfec when offered, as still unsupported (see meetecho#2639)
  Don't add rtx ssrc if m-line is recvonly/inactive (see meetecho#2639)
  Added NULL checks for json_dumps (see meetecho#2629)
  Parse custom headers, if required, in successful REGISTER response (fixes meetecho#2636)
  Timestamp correction for janus-pp-rec (meetecho#2573)
  Don't chain error handler to success handler in Janus.httpAPICall (meetecho#2569)
  Add missing library link for WS event handler (fixes meetecho#2628)
  Fixed broken switch in Streaming plugin when using helper threads
  Resolves meetecho#2624 jansson double referencing (meetecho#2634)
  Unlock mountpoints mutex after the spawning of helper threads.
  ...

# Conflicts:
#	transports/janus_mqtt.c
heavyrain2012 pushed a commit to heavyrain2012/janus-gateway that referenced this pull request Jul 29, 2021
@imhao183 @heavyrain2012
Merge branch 'meetecho_master'
dab3e25 
* meetecho_master: (345 commits)
  Remove support for framemarking RTP extension (meetecho#2640)
  Prevent race conditions on socket close in SIP and NoSIP plugins (meetecho#2599)
  Fixed overflow runtime error
  Fix for race condition between VideoRoom publisher leaving and subscriber hanging up (fixes meetecho#2582) (meetecho#2637)
  Send PLI when starting a paused stream (meetecho#2645)
  Prevent too high shift exponent
  Fixed type of seq/ts in file-based Streaming mountpoint threads
  Fix missing g_thread_unref when a streaming helper thread quits.
  Added custom headers for SIP INFO request (meetecho#2644)
  Free participant->user_id_str in case of opus enc/decoder error.
  Reject flexfec when offered, as still unsupported (see meetecho#2639)
  Don't add rtx ssrc if m-line is recvonly/inactive (see meetecho#2639)
  Added NULL checks for json_dumps (see meetecho#2629)
  Parse custom headers, if required, in successful REGISTER response (fixes meetecho#2636)
  Timestamp correction for janus-pp-rec (meetecho#2573)
  Don't chain error handler to success handler in Janus.httpAPICall (meetecho#2569)
  Add missing library link for WS event handler (fixes meetecho#2628)
  Fixed broken switch in Streaming plugin when using helper threads
  Resolves meetecho#2624 jansson double referencing (meetecho#2634)
  Unlock mountpoints mutex after the spawning of helper threads.
  ...

# Conflicts:
#	transports/janus_mqtt.c
heavyrain2012 pushed a commit to heavyrain2012/janus-gateway that referenced this pull request Aug 2, 2021
@imhao183 @heavyrain2012
Merge branch 'meetecho_master'
e231d7a 
* meetecho_master: (345 commits)
  Remove support for framemarking RTP extension (meetecho#2640)
  Prevent race conditions on socket close in SIP and NoSIP plugins (meetecho#2599)
  Fixed overflow runtime error
  Fix for race condition between VideoRoom publisher leaving and subscriber hanging up (fixes meetecho#2582) (meetecho#2637)
  Send PLI when starting a paused stream (meetecho#2645)
  Prevent too high shift exponent
  Fixed type of seq/ts in file-based Streaming mountpoint threads
  Fix missing g_thread_unref when a streaming helper thread quits.
  Added custom headers for SIP INFO request (meetecho#2644)
  Free participant->user_id_str in case of opus enc/decoder error.
  Reject flexfec when offered, as still unsupported (see meetecho#2639)
  Don't add rtx ssrc if m-line is recvonly/inactive (see meetecho#2639)
  Added NULL checks for json_dumps (see meetecho#2629)
  Parse custom headers, if required, in successful REGISTER response (fixes meetecho#2636)
  Timestamp correction for janus-pp-rec (meetecho#2573)
  Don't chain error handler to success handler in Janus.httpAPICall (meetecho#2569)
  Add missing library link for WS event handler (fixes meetecho#2628)
  Fixed broken switch in Streaming plugin when using helper threads
  Resolves meetecho#2624 jansson double referencing (meetecho#2634)
  Unlock mountpoints mutex after the spawning of helper threads.
  ...

# Conflicts:
#	transports/janus_mqtt.c
heavyrain2012 pushed a commit to heavyrain2012/janus-gateway that referenced this pull request Nov 14, 2021
@imhao183 @heavyrain2012
Merge branch 'meetecho_master'
1827315 
* meetecho_master: (345 commits)
  Remove support for framemarking RTP extension (meetecho#2640)
  Prevent race conditions on socket close in SIP and NoSIP plugins (meetecho#2599)
  Fixed overflow runtime error
  Fix for race condition between VideoRoom publisher leaving and subscriber hanging up (fixes meetecho#2582) (meetecho#2637)
  Send PLI when starting a paused stream (meetecho#2645)
  Prevent too high shift exponent
  Fixed type of seq/ts in file-based Streaming mountpoint threads
  Fix missing g_thread_unref when a streaming helper thread quits.
  Added custom headers for SIP INFO request (meetecho#2644)
  Free participant->user_id_str in case of opus enc/decoder error.
  Reject flexfec when offered, as still unsupported (see meetecho#2639)
  Don't add rtx ssrc if m-line is recvonly/inactive (see meetecho#2639)
  Added NULL checks for json_dumps (see meetecho#2629)
  Parse custom headers, if required, in successful REGISTER response (fixes meetecho#2636)
  Timestamp correction for janus-pp-rec (meetecho#2573)
  Don't chain error handler to success handler in Janus.httpAPICall (meetecho#2569)
  Add missing library link for WS event handler (fixes meetecho#2628)
  Fixed broken switch in Streaming plugin when using helper threads
  Resolves meetecho#2624 jansson double referencing (meetecho#2634)
  Unlock mountpoints mutex after the spawning of helper threads.
  ...

# Conflicts:
#	transports/janus_mqtt.c
heavyrain2012 pushed a commit to heavyrain2012/janus-gateway that referenced this pull request Dec 7, 2021
@imhao183
Merge branch 'meetecho_master'
b9318b7 
* meetecho_master: (345 commits)
  Remove support for framemarking RTP extension (meetecho#2640)
  Prevent race conditions on socket close in SIP and NoSIP plugins (meetecho#2599)
  Fixed overflow runtime error
  Fix for race condition between VideoRoom publisher leaving and subscriber hanging up (fixes meetecho#2582) (meetecho#2637)
  Send PLI when starting a paused stream (meetecho#2645)
  Prevent too high shift exponent
  Fixed type of seq/ts in file-based Streaming mountpoint threads
  Fix missing g_thread_unref when a streaming helper thread quits.
  Added custom headers for SIP INFO request (meetecho#2644)
  Free participant->user_id_str in case of opus enc/decoder error.
  Reject flexfec when offered, as still unsupported (see meetecho#2639)
  Don't add rtx ssrc if m-line is recvonly/inactive (see meetecho#2639)
  Added NULL checks for json_dumps (see meetecho#2629)
  Parse custom headers, if required, in successful REGISTER response (fixes meetecho#2636)
  Timestamp correction for janus-pp-rec (meetecho#2573)
  Don't chain error handler to success handler in Janus.httpAPICall (meetecho#2569)
  Add missing library link for WS event handler (fixes meetecho#2628)
  Fixed broken switch in Streaming plugin when using helper threads
  Resolves meetecho#2624 jansson double referencing (meetecho#2634)
  Unlock mountpoints mutex after the spawning of helper threads.
  ...

# Conflicts:
#	transports/janus_mqtt.c

(cherry picked from commit 1827315)
heavyrain2012 pushed a commit to heavyrain2012/janus-gateway that referenced this pull request Jan 29, 2022
@imhao183
Merge branch 'meetecho_master'
cf53cb9 
* meetecho_master: (345 commits)
  Remove support for framemarking RTP extension (meetecho#2640)
  Prevent race conditions on socket close in SIP and NoSIP plugins (meetecho#2599)
  Fixed overflow runtime error
  Fix for race condition between VideoRoom publisher leaving and subscriber hanging up (fixes meetecho#2582) (meetecho#2637)
  Send PLI when starting a paused stream (meetecho#2645)
  Prevent too high shift exponent
  Fixed type of seq/ts in file-based Streaming mountpoint threads
  Fix missing g_thread_unref when a streaming helper thread quits.
  Added custom headers for SIP INFO request (meetecho#2644)
  Free participant->user_id_str in case of opus enc/decoder error.
  Reject flexfec when offered, as still unsupported (see meetecho#2639)
  Don't add rtx ssrc if m-line is recvonly/inactive (see meetecho#2639)
  Added NULL checks for json_dumps (see meetecho#2629)
  Parse custom headers, if required, in successful REGISTER response (fixes meetecho#2636)
  Timestamp correction for janus-pp-rec (meetecho#2573)
  Don't chain error handler to success handler in Janus.httpAPICall (meetecho#2569)
  Add missing library link for WS event handler (fixes meetecho#2628)
  Fixed broken switch in Streaming plugin when using helper threads
  Resolves meetecho#2624 jansson double referencing (meetecho#2634)
  Unlock mountpoints mutex after the spawning of helper threads.
  ...

# Conflicts:
#	transports/janus_mqtt.c

(cherry picked from commit 1827315)
(cherry picked from commit b9318b7)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
please-test
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@lminiero @bhakimi @ihusejnovic