Videoroom: stop_rtp_forward - need to decrease ps refcount when g_hash_table_lookup fail

General
1

(My english is not that good.)

I deeply appreciate the Janus.

It seems that when handling stop_rtp_forward, ps refcount should be decreased if g_hash_table_lookup fails.
Is that correct?

else if(!strcasecmp(request_text, "stop_rtp_forward")) {
...
	GList *temp = publisher->streams;
	while(temp) {
		janus_videoroom_publisher_stream *ps = (janus_videoroom_publisher_stream *)temp->data;
		janus_refcount_increase(&ps->ref);
		janus_mutex_lock(&ps->rtp_forwarders_mutex);
		janus_rtp_forwarder *f = g_hash_table_lookup(ps->rtp_forwarders, GUINT_TO_POINTER(stream_id));
		if(f != NULL) {
			if(f->metadata != NULL) {
				/* This belongs to a remotization, ignore */
				janus_mutex_unlock(&ps->rtp_forwarders_mutex);
				janus_refcount_decrease(&ps->ref);
				found = FALSE;
				break;
			}
			g_hash_table_remove(ps->rtp_forwarders, GUINT_TO_POINTER(stream_id));
			janus_mutex_unlock(&ps->rtp_forwarders_mutex);
			janus_refcount_decrease(&ps->ref);
			/* Found, remove from global index too */
			g_hash_table_remove(publisher->rtp_forwarders, GUINT_TO_POINTER(stream_id));
			found = TRUE;
			break;
		}
		janus_mutex_unlock(&ps->rtp_forwarders_mutex);
		//  *REFERENCE CODE* ->
		// janus_refcount_decrease(&ps->ref);
		// <- *REFERENCE CODE*
		temp = temp->next;
	}
	...
}
2

No, if the lookup fails, it means the forwarder isn’t there (or isn’t there anymore), and so if a refcount was there it’s been removed already.

1 Like
3

Thanks for replying.

As the comment in the code below suggests, it seems to be searching through all the streams.
/* Find the forwarder by iterating on all the streams */

stream ex)
idx=0, type=audio, rtp_fw_id=123,…
idx=1, type=video, rtp_fw_id=456,…

During the iteration, the ps refcount is increased, and it searches within ps->rtp_forwarders. If the stream type is different, the lookup will fail.
In that case, the ps refcount should be decreased again.

4

Ah I see what you mean, now. I thought you were referring to the references that we mantain for the presence in the hashtable, but that’s something different. Looking at the code, you may indeed be right, meaning there could be a leak any time stop_rtp_forward is called explicitly. Let me have a look at the history of that code block, and then I’ll try to replicate the leak.

1 Like
5

I can indeed replicate this, so I just pushed a commit to fix this. Thanks for spotting this! :folded_hands:

2 Likes
6

Thank you for checking it so quickly.
I’m thankful that I’m having a good time these days because of Janus.

1 Like