janus_videoroom_hangup_subscriber segfaults with lots of publishers / subscribers leaving at the same time

[cb22]

I'm not sure if this is related to #2034, but it looks like there's quite a specific setup required to trigger this bug and I can't see any information there linking the two besides both involving the videoroom plugin.

The setup:

• Latest janus master
• 8 event loop threads
• private_ids on (mentioned due to the stacktrace)
• Lots of active rooms, with publishers / subscribers leaving due to being killed (not leaving gracefully)

Janus segfaults when a large amount of participants leave the videoroom (stacktrace). My testing setup here consists of ~100x Google Cloud VM instances, each running 3x Chrome instances per videoroom, to two janus servers.

I can seemingly replicate this crash consistently. Interesting to note, is that the crash doesn't seem to happen if I turn off a fixed number of event loop threads, nor if the publishers / subscribers send leave requests. I'm not sure if those two points might just be a red herring due to an underlying race condition though.

The specific line mentioned in the stacktrace is here

Some speculation (I'm not familiar with GLib!) is perhaps there's a TOUTOC between the check for s->room != NULL and the usage of g_hash_table_lookup(s->room->private_ids. Anyhow, I'm going to do some more invetigation, but I'm happy to provide any more info or run tests as required!

[lminiero]

I had to google TOUTOC to see what it meant... 🤭
A libasan dump might help, in this case, as that would tell if that pointer was freed somewhere or just set to NULL.

[cb22]

Managed to recreate it with a simpler setup than before - 1 publisher and 8 subscribers in the same room will do it fairly repeatedly, as long as the subscribers all leave at the same time (killall -9 chromium does the job quite well if they're all on the same machine)

I've got a libasan dump (here) but it's not going to be too useful for others since I've added a few debugging statements and extra code to narrow down the issue (such as an extra check on s->room != NULL).

It looks like what happens is somehow janus_videoroom_hangup_subscriber gets called with the same s twice in succession (verified by adding in some extra logging). The first runs successfully, but when the second runs, the state has been cleaned up, which is why trying to unlock the mutex, or do a lookup on s->room->private_ids fails.

If I add a simple if guard after janus_mutex_lock, it seems to fix the bug (see here). I can no longer reproduce it on either my local testing, or on the larger testing setup with ~100 peers even after multiple runs. This does feel like a bit of hack though, so I'd appreciate any feedback!

[lminiero]

It still might be a bit frail, though. A potentially better solution might be to assign the s->room property to a dedicated variable, e.g.:

janus_videoroom_room *room = s->room;

incref that if null, use that for the checks and work, and then decref it when done. This way, it doesn't really matter whether s->room itself is NULL or not, since we only use the room info (private IDs) to update the publisher. If that makes sense I can prepare a PR later.

[lminiero]

@cb22 can you let me know if the PR above helps?

[cb22]

@lminiero your solution sounds much better to me indeed. I've given it some testing, and I can't reproduce the crash anymore - so LGTM 👍

[lminiero]

Thanks! I'll merge then 👍