Skip to content

transports/http: avoid segfault on JSON error#2629

Closed
tmatth wants to merge 1 commit intomeetecho:masterfrom
tmatth:bugfix/segfault-http-sendmsg
Closed

transports/http: avoid segfault on JSON error#2629
tmatth wants to merge 1 commit intomeetecho:masterfrom
tmatth:bugfix/segfault-http-sendmsg

Conversation

@tmatth
Copy link
Copy Markdown
Contributor

@tmatth tmatth commented Apr 9, 2021 •
edited
Loading

Refs #2626

@tmatth tmatth mentioned this pull request Apr 9, 2021
Closed
@lminiero
Copy link
Copy Markdown
Member

lminiero commented Apr 9, 2021

Thanks! This feels like a check that should be added to the other transports (and event handlers?) as well, in case, since they'd suffer from the same issue with overly large objects.

@tmatth
Copy link
Copy Markdown
Contributor Author

tmatth commented Apr 9, 2021

Thanks! This feels like a check that should be added to the other transports (and event handlers?) as well, in case, since they'd suffer from the same issue with overly large objects.

Yeah that's why I marked as draft, if @winlinvip is able to confirm that this fixes the one crash there are probably a lot of other places where jansson's output needs to be checked for NULL, as unlike glib it won't abort on e.g. a failed allocation (or even invalid JSON).

@tmatth tmatth force-pushed the bugfix/segfault-http-sendmsg branch from ee82d11 to 0271c54 Compare April 9, 2021 19:17
@lminiero
Copy link
Copy Markdown
Member

lminiero commented Apr 16, 2021

I doubt we'll get feedback from the OP, and I think these are good checks to have: since they seem straightforward enough, I can take care of them all in a dedicated commit.

lminiero added a commit that referenced this pull request Apr 16, 2021
@lminiero
Added NULL checks for json_dumps (see #2629)
2165a50
@lminiero
Copy link
Copy Markdown
Member

lminiero commented Apr 16, 2021

The above commit should address them all, so I'll close this PR. Thanks!

@lminiero lminiero closed this Apr 16, 2021
@winlinvip
Copy link
Copy Markdown

winlinvip commented Apr 19, 2021

Sorry, I'm a bit quit busy these days, I will check it now.

@tmatth tmatth deleted the bugfix/segfault-http-sendmsg branch April 19, 2021 00:08
@winlinvip
Copy link
Copy Markdown

winlinvip commented Apr 19, 2021 •
edited
Loading

I did some benchmarks, NO crash now, thanks~

  • For 4CPU, although the load is very high, and some sessions were timeout, but no crash now.
  • For 8CPU, the max threads can be about 1800, but lots of timeout and decrease to about 900 threads. No crash now.

It's possible to publish more streams to Janus than before. I got some data but I'm not sure it's ok to show it, but I think the perf and benchmark snapshot is OK, which might be useful for performance improvement.

The bellow is benchmark for 4CPUs, about 900 threads of Janus:

image

I think it's ok to close this issue and #2626

@atoppi
Copy link
Copy Markdown
Contributor

atoppi commented Apr 19, 2021 •
edited
Loading

@winlinvip thanks for sharing your findings.
I have some observations:

  1. could you please inspect the janus_ice_cb_nice_recv function with annotate to find the hot path ? We suspect that the very high usage is due to nack handling
  2. srtp_aes_encyrpt as the second highest function makes me suspect that you are testing with a VM / srtp library without hw acceleration for AES-NI
  3. also the high usage from srtp_aes_encrypt in a publishers-only test could mean that you are generating A LOT of RTCP traffic and that might be explained by A LOT of nacks sent by Janus (network loss? the bench tool struggling?)
  4. could you share the remaining data that you're unsure to post here? Please drop an email to dev@meetecho.com

@atoppi
Copy link
Copy Markdown
Contributor

atoppi commented Apr 19, 2021

Also when expecting such a high load on a server, it would be better to tune the event_loops setting in janus.jcfg to
N x #CORES
with N >= 1

@winlinvip
Copy link
Copy Markdown

winlinvip commented Apr 20, 2021 •
edited
Loading

@atoppi Thanks for your help~

  1. For the performance improvement about functions, I'm really sorry, because it takes a lot of time to test and analysis. I think it's better way, if you use srs-bench to benchmark locally, or by other tools.
  2. I will try to set event_loops to 8 and give some feedback.

@winlinvip
Copy link
Copy Markdown

winlinvip commented Apr 20, 2021 •
edited
Loading

@atoppi It works! Thanks very much.

# janus.jcfg
event_loops = 8

For 8 CPU server, janus is 779% and load is 10, and threads is 28. The data is much much better than before.

The bellow is perf data, and I will email you the compare data to dev@meetecho.com

image

Thanks~

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

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@tmatth @lminiero @winlinvip @atoppi