Videoroom: crash in janus_videoroom_reqpli() function on Janus v1.3.2

General
1

(My english is not that good.)

When receiving an “unpublish” request in the publishing state, we occasionally encounter a crash at ① line 2996.

Upon investigation, I found that during the handling of ②③ the “unpublish” request, ps->publisher = NULL; is executed.

I would like your advice on how to resolve this issue.

pli crash
pli crash926×748 37.5 KB

2

This may be a good catch, I just pushed a commit that adds a NULL check for that. Please let me know if that still causes issues.

2 Likes
3

Lorenzo, Thanks for replying.
I understand and I have one opinion.

I think the core of this issue is that ps->publisher becomes NULL before the (publisher stream) ps is freed.
In the commit below, the janus_videoroom_publisher_stream_destroy() function was added, and when removing participant->streams_byid, ps->publisher is set to NULL. I assume there must be a reason for this.

git commit - 2022-02-11 Support for multistream PeerConnections (replaces #1459) (#2211)

	publisher->streams_byid = g_hash_table_new_full(NULL, NULL,
		NULL, (GDestroyNotify)janus_videoroom_publisher_stream_destroy);
	publisher->streams_bymid = g_hash_table_new_full(g_str_hash, g_str_equal,
		(GDestroyNotify)g_free, (GDestroyNotify)janus_videoroom_publisher_stream_unref);

static void janus_videoroom_hangup_media_internal(gpointer session_data) {
	...
	g_hash_table_remove_all(participant->streams_byid);
	g_hash_table_remove_all(participant->streams_bymid);
	...
}

static void janus_videoroom_publisher_stream_destroy(janus_videoroom_publisher_stream *ps) {
	if(ps && g_atomic_int_compare_and_exchange(&ps->destroyed, 0, 1)) {
		if(ps->publisher)
			janus_refcount_decrease(&ps->publisher->ref);
		ps->publisher = NULL;
		janus_refcount_decrease(&ps->ref);
	}
	/* TODO Should unref the publisher instance? */
}

I’m wondering if it can be handled using either method ① or ②.

① Add a janus_refcount_decrease(&ps->publisher->ref) call inside janus_videoroom_publisher_stream_free.
→ It is possible to access ps->publisher until the (publisher stream) ps is freed.

② When removing publisher->streams_byid, only perform janus_refcount_decrease.
→ In janus_videoroom_publisher_free, janus_videoroom_publisher_stream_destroy is executed via
g_list_free_full(p->streams, (GDestroyNotify)(janus_videoroom_publisher_stream_destroy));

1 Like
4

I’ve added the NULL check and am testing it now. I’ll share any issues if they come up.

5

No, free must only free stuff. Crossed references are already dealt in different ways.

The destroy callback function already only uses the refcount decrease. I don’t know what version you’re looking at, but I don’t see any g_list_free_full in janus_videoroom_publisher_stream_destroy. Stuff is only freed if a refcount decrease brings the reference count to 0.

6

(post deleted by author)

7

Thanks for replying.

g_list_free_full in janus_videoroom_publisher_free() function.

When a publisher is freed, all its streams are deleted through the janus_videoroom_publisher_stream_destroy() function."

janus_videoroom_publisher_free() {
	....
	/* Get rid of all the streams */
	g_list_free_full(p->streams, (GDestroyNotify)(janus_videoroom_publisher_stream_destroy));
	...
}
1 Like