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

[1.x] Streaming plugin #3288

Closed
amnonbb opened this issue Nov 9, 2023 · 4 comments
Closed

[1.x] Streaming plugin #3288

amnonbb opened this issue Nov 9, 2023 · 4 comments
Labels
multistream Related to Janus 1.x

Comments

@amnonbb
Copy link
Contributor

amnonbb commented Nov 9, 2023

What version of Janus is this happening on?
1.2.1
0ae1c6d

Have you tested a more recent version of Janus too?
It's latest master

Was this working before?
This crash i got in all version i tried

Is there a gdb or libasan trace of the issue?
https://gist.github.com/amnonbb/8e88f82859eede4e44a69f3f5555642d

Additional context
It's for sure cause when client do ICE restart. (When i turn off this on client, crash is gone) But i it's in some unknown condition. It's not possible to reproduce directly to use ICE restart.

@amnonbb amnonbb added the multistream Related to Janus 1.x label Nov 9, 2023
@atoppi
Copy link
Member

atoppi commented Nov 9, 2023

The previously allocated and freed by sections in the asan dump look inverted:

Nov 09 12:06:44 str1 janus[110770]: previously allocated by thread T6 here:
Nov 09 12:06:44 str1 janus[110770]:     #3 0x7f2fde0e0c98  (/lib64/libglib-2.0.so.0+0x3bc98)
Nov 09 12:06:44 str1 janus[110770]:     #2 0x7f2fd0d4eae7 in janus_streaming_session_destroy plugins/janus_streaming.c:1563
Nov 09 12:06:44 str1 janus[110770]:     #1 0x7f2fde0f86f1 in g_free (/lib64/libglib-2.0.so.0+0x536f1)
Nov 09 12:06:44 str1 janus[110770]:     #0 0x7f2fdef087f0 in __interceptor_free (/lib64/libasan.so.5+0xef7f0)
Nov 09 12:06:44 str1 janus[110770]: freed by thread T313 (hloop 845082543) here:
Nov 09 12:06:44 str1 janus[110770]: 0x60b0002617c0 is located 0 bytes inside of 112-byte region [0x60b0002617c0,0x60b000261830)
Nov 09 12:06:44 str1 janus[110770]:     #5 0x7f2fdc9f8e72 in __clone (/lib64/libc.so.6+0x39e72)
Nov 09 12:06:44 str1 janus[110770]:     #4 0x7f2fdcd8c1c9 in start_thread (/lib64/libpthread.so.0+0x81c9)
Nov 09 12:06:44 str1 janus[110770]:     #3 0x7f2fde11b4e9  (/lib64/libglib-2.0.so.0+0x764e9)
Nov 09 12:06:44 str1 janus[110770]:     #2 0x7f2fd0d86947 in janus_streaming_relay_thread plugins/janus_streaming.c:9584
Nov 09 12:06:44 str1 janus[110770]:     #1 0x7f2fde0eeda4 in g_list_foreach (/lib64/libglib-2.0.so.0+0x49da4)
Nov 09 12:06:44 str1 janus[110770]:     #0 0x7f2fd0d87feb in janus_streaming_relay_rtp_packet plugins/janus_streaming.c:10002

Q1: just for confirmation, are you using the API flag restart to force janus resending a JSEP offer (e.g. you are NOT sending an offer FROM the browser) ?

Anyway the crash has been caused by a freed session while trying to relay a packet to the same session.
Since the dump mentions janus_streaming_session_destroy, this means that something has requested the plugin session teardown.

Q2: could your client be sending by mistake a destroy session or closing the transport while trying to perform an ICE restart?

@atoppi
Copy link
Member

atoppi commented Nov 9, 2023

@amnonbb can you try again after applying this patch?

diff --git a/src/plugins/janus_streaming.c b/src/plugins/janus_streaming.c
index 2cadc73d..4cca8d29 100644
--- a/src/plugins/janus_streaming.c
+++ b/src/plugins/janus_streaming.c
@@ -5843,6 +5843,27 @@ static void *janus_streaming_handler(void *data) {
 			/* Check if this is a new viewer, or if an update is taking place (i.e., ICE restart) */
 			gboolean audio = TRUE, video = TRUE, data = TRUE;
 			if(do_restart) {
+				if(session->mountpoint == NULL) {
+					JANUS_LOG(LOG_ERR, "Can't perform ICE restart: no mountpoint set\n");
+					error_code = JANUS_STREAMING_ERROR_NO_SUCH_MOUNTPOINT;
+					g_snprintf(error_cause, 512, "Can't perform ICE restart: no mountpoint set");
+					janus_mutex_unlock(&session->mutex);
+					janus_mutex_unlock(&mp->mutex);
+					janus_mutex_unlock(&sessions_mutex);
+					janus_refcount_decrease(&mp->ref);
+					goto error;
+				}
+				if(session->mountpoint != mp) {
+					/* Already watching something else */
+					JANUS_LOG(LOG_ERR, "Already watching mountpoint %s\n", session->mountpoint->id_str);
+					error_code = JANUS_STREAMING_ERROR_INVALID_STATE;
+					g_snprintf(error_cause, 512, "Already watching mountpoint %s", session->mountpoint->id_str);
+					janus_mutex_unlock(&session->mutex);
+					janus_mutex_unlock(&mp->mutex);
+					janus_mutex_unlock(&sessions_mutex);
+					janus_refcount_decrease(&mp->ref);
+					goto error;
+				}
 				/* User asked for an ICE restart: provide a new offer */
 				if(!g_atomic_int_compare_and_exchange(&session->renegotiating, 0, 1)) {
 					/* Already triggered a renegotiation, and still waiting for an answer */

@amnonbb
Copy link
Contributor Author

amnonbb commented Nov 9, 2023

  1. I send {request: 'watch', id, restart: true}. And it's actually work.
  2. I think it's can happen. ICE restart trigger on ICE state disconnected and case maybe bad network connection.

I'll try patch and give you feedback. Thank you!

@amnonbb
Copy link
Contributor Author

amnonbb commented Nov 11, 2023

Looks like patch solve issue. Two days without any crash. Before this i got 5-10 crash in one day. Thank you!

@atoppi atoppi closed this as completed in 7b91e80 Nov 13, 2023
atoppi added a commit that referenced this issue Nov 13, 2023
amnonbb added a commit to Bnei-Baruch/galaxy3 that referenced this issue Nov 13, 2023
amnonbb added a commit to Bnei-Baruch/trl-system that referenced this issue Nov 13, 2023
lminiero pushed a commit that referenced this issue Nov 28, 2023
mwalbeck pushed a commit to mwalbeck/docker-janus-gateway that referenced this issue Dec 8, 2023
This PR contains the following updates:

| Package | Update | Change |
|---|---|---|
| [meetecho/janus-gateway](https://github.com/meetecho/janus-gateway) | minor | `v1.1.4` -> `v1.2.1` |

---

### Release Notes

<details>
<summary>meetecho/janus-gateway (meetecho/janus-gateway)</summary>

### [`v1.2.1`](https://github.com/meetecho/janus-gateway/blob/HEAD/CHANGELOG.md#v121---2023-12-06)

[Compare Source](meetecho/janus-gateway@v1.2.0...v1.2.1)

-   Added support for abs-capture-time RTP extension \[[PR-3161](meetecho/janus-gateway#3161)]
-   Fixed truncated label in datachannels (thanks [@&#8203;veeting](https://github.com/veeting)!) \[[PR-3265](meetecho/janus-gateway#3265)]
-   Support larger values for SDP attributes (thanks [@&#8203;petarminchev](https://github.com/petarminchev)!) \[[PR-3282](meetecho/janus-gateway#3282)]
-   Fixed rare crash in VideoRoom plugin (thanks [@&#8203;tmatth](https://github.com/tmatth)!) \[[PR-3259](meetecho/janus-gateway#3259)]
-   Don't create VideoRoom subscriptions to publisher streams with no associated codecs
-   Added suspend/resume participant API to AudioBridge \[[PR-3301](meetecho/janus-gateway#3301)]
-   Fixed rare crash in AudioBridge
-   Fixed rare crash in Streaming plugin when doing ICE restarts \[[Issue-3288](meetecho/janus-gateway#3288)]
-   Allow SIP and NoSIP plugins to bind media to a specific address (thanks [@&#8203;pawnnail](https://github.com/pawnnail)!) \[[PR-3263](meetecho/janus-gateway#3263)]
-   Removed advertised support for SIP UPDATE in SIP plugin
-   Parse RFC2833 DTMF to custom events in SIP plugin (thanks [@&#8203;ywmoyue](https://github.com/ywmoyue)!) \[[PR-3280](meetecho/janus-gateway#3280)]
-   Fixed missing Contact header in some dialogs in SIP plugin (thanks [@&#8203;ycherniavskyi](https://github.com/ycherniavskyi)!) \[[PR-3286](meetecho/janus-gateway#3286)]
-   Properly set mid when notifying about ended tracks in janus.js
-   Fixed broken restamping in janus-pp-rec
-   Other smaller fixes and improvements (thanks to all who contributed pull requests and reported issues!)

### [`v1.2.0`](https://github.com/meetecho/janus-gateway/blob/HEAD/CHANGELOG.md#v120---2023-08-09)

[Compare Source](meetecho/janus-gateway@v1.1.4...v1.2.0)

-   Added support for VP9/AV1 simulcast, and fixed broken AV1/SVC support \[[PR-3218](meetecho/janus-gateway#3218)]
-   Fixed RTCP out quality stats \[[PR-3228](meetecho/janus-gateway#3228)]
-   Default link quality stats to 100
-   Added support for ICE consent freshness \[[PR-3234](meetecho/janus-gateway#3234)]
-   Fixed rare race condition in VideoRoom \[[PR-3219](meetecho/janus-gateway#3219)] \[[PR-3247](meetecho/janus-gateway#3247)]
-   Use speexdsp's jitter buffer in the AudioBridge \[[PR-3233](meetecho/janus-gateway#3233)]
-   Fixed crash in Streaming plugin on mountpoints with too many streams \[[Issue-3225](meetecho/janus-gateway#3225)]
-   Support for batched configure requests in Streaming plugin (thanks [@&#8203;petarminchev](https://github.com/petarminchev)!) \[[PR-3239](meetecho/janus-gateway#3239)]
-   Fixed rare deadlock in Streamin plugin \[[PR-3250](meetecho/janus-gateway#3250)]
-   Fix simulated leave message for longer string ID rooms in TextRoom (thanks [@&#8203;adnanel](https://github.com/adnanel)!) [PR-3243](meetecho/janus-gateway#3243)]
-   Notify about count of sessions, handles and PeerConnections on a regular basis, when event handlers are enabled \[[PR-3221](meetecho/janus-gateway#3221)]
-   Fixed broken Insertable Streams for recvonly streams when answering in janus.js
-   Added background selector and blur support to Virtual Background demo
-   Other smaller fixes and improvements (thanks to all who contributed pull requests and reported issues!)

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update again.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box

---

This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).
<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNi4zNC4wIiwidXBkYXRlZEluVmVyIjoiMzcuODEuNCIsInRhcmdldEJyYW5jaCI6Im1hc3RlciJ9-->

Reviewed-on: https://git.walbeck.it/walbeck-it/docker-janus-gateway/pulls/122
Co-authored-by: renovate-bot <bot@walbeck.it>
Co-committed-by: renovate-bot <bot@walbeck.it>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
multistream Related to Janus 1.x
Projects
None yet
Development

No branches or pull requests

2 participants