[0.x, 1.x] Race condition in janus_plugin_close_pc_internal() causes SIGSEGV (use-after-free)

[moon8011]

Description

A race condition in janus_plugin_close_pc() / janus_plugin_close_pc_internal() can cause a crash (SIGSEGV) due to use-after-free of ice_handle.

Environment

• Janus version: 0.14.1
• OS: Linux (RHEL/CentOS)
• Plugin: VideoRoom
  Note: This issue exists in both 0.x and 1.x branches as the affected code is identical.

Crash Information

Program terminated with signal SIGSEGV, Segmentation fault.
#0 0x0000000000452b1a in janus_plugin_close_pc_internal (user_data=0x7f92e8049f00) at janus.c:4114
plugin_session = 0x7f92e8049f00
ice_handle = 0x7f95113f9ff9 <-- invalid/freed pointer (not 8-byte aligned)

Root Cause

In janus_plugin_close_pc(), only plugin_session->ref is increased before registering the timeout callback, but ice_handle->ref is NOT increased:

void janus_plugin_close_pc(janus_plugin_session *plugin_session) {
    // ...
    janus_refcount_increase(&plugin_session->ref);
    // ice_handle->ref is NOT increased here!
    GSource *timeout_source = g_timeout_source_new_seconds(0);
    g_source_set_callback(timeout_source, janus_plugin_close_pc_internal, plugin_session, NULL);
    g_source_attach(timeout_source, sessions_watchdog_context);
    // ...
}

This allows ice_handle to be freed by another thread (e.g., handling a "detach" request) before janus_plugin_close_pc_internal() is executed.

Race Condition Scenario

VideoRoom Handler Thread          Requests Thread              Watchdog Thread
        |                              |                            |
   close_pc()                          |                            |
        |                              |                            |
   timeout registered                  |                            |
        |                              |                            |
        |                     "detach" request received             |
        |                              |                            |
        |                     janus_ice_handle_destroy()            |
        |                              |                            |
        |                     ice_handle freed                      |
        |                              |                            |
        |                              |                   close_pc_internal()
        |                              |                            |
        |                              |                   ice_handle access
        |                              |                            |
        |                              |                   SIGSEGV 💥

Proposed Fix

Add janus_refcount_increase(&ice_handle->ref) in janus_plugin_close_pc() before registering the timeout:

void janus_plugin_close_pc(janus_plugin_session *plugin_session) {
    if(!janus_plugin_session_is_alive(plugin_session))
        return;
    janus_ice_handle *ice_handle = (janus_ice_handle *)plugin_session->gateway_handle;
    if(!ice_handle || !g_atomic_int_compare_and_exchange(&ice_handle->closepc, 0, 1))
        return;
    janus_refcount_increase(&plugin_session->ref);
    janus_refcount_increase(&ice_handle->ref);  // <-- ADD THIS
    GSource *timeout_source = g_timeout_source_new_seconds(0);
    g_source_set_callback(timeout_source, janus_plugin_close_pc_internal, plugin_session, NULL);
    g_source_attach(timeout_source, sessions_watchdog_context);
    g_source_unref(timeout_source);
}

And update janus_plugin_close_pc_internal() to handle the early return case:

static gboolean janus_plugin_close_pc_internal(gpointer user_data) {
    janus_plugin_session *plugin_session = (janus_plugin_session *) user_data;
    janus_ice_handle *ice_handle = (janus_ice_handle *)plugin_session->gateway_handle;
    if(!ice_handle || !g_atomic_int_compare_and_exchange(&ice_handle->closepc, 1, 0)) {
        janus_refcount_decrease(&plugin_session->ref);
        if(ice_handle)
            janus_refcount_decrease(&ice_handle->ref);  // <-- ADD THIS
        return G_SOURCE_REMOVE;
    }
    // Remove the existing increase since we already did it in close_pc()
    // janus_refcount_increase(&ice_handle->ref);  // <-- REMOVE THIS (line 4118)

    // ... rest of the function remains the same ...
}

Additional Notes

- The server was running with ~1000 threads at the time of crash (high load)
- The invalid pointer 0x7f95113f9ff9 is not 8-byte aligned, indicating memory corruption/reuse after free

[lminiero]

I have a strong suspicion this report is AI generated. janus_plugin_close_pc_internal is not a method anyone can invoke: it's only invoked by the loop responsible for the handle, and it's still the loop that physically frees the handle, so no race is possible there (same thread). The loop will also hold a reference to the handle too until it frees it (as each handle will be a source in the loop), so a call to janus_ice_handle_destroy on the requests thread cannot bring the references to 0. This is especially true when event loops are not used, as in that case there's a 1-1 mapping between loop (and so thread) and handle.

[moon8011]

@lminiero

Thank you for the detailed response.

First of all, I would like to clarify that the report was indeed organized with the help of AI.
The analysis itself was done by me, but I used AI to structure and explain the findings more clearly.
I apologize if that was not appropriate or caused any confusion.

Let me briefly explain the context in which we encountered this issue.

In our production environment, a single videoroom can have a large number of publishers and subscribers.
Before a videoroom is deleted, we send:

• leave and detach to each publisher
• detach to each subscriber

At the moment, this issue is extremely difficult for us to reproduce deterministically.
Because of that, we analyzed the crash stack trace together with the last logs observed at the time of the crash,
which consistently showed a sequence involving leave and detach.
Based on that analysis, we suspected a possible issue and decided to report it.

I have read your explanation carefully and I appreciate the detailed insight.
However, there is one point where my understanding of the code seems to differ slightly from yours,
and I would like to ask for clarification in case I am misunderstanding something.

1. The thread that invokes `janus_plugin_close_pc_internal
   ( leave message)

From what I can see in the code, janus_plugin_close_pc_internal is scheduled on
sessions_watchdog_context, which is run by the "sessions watchdog" thread:

g_source_set_callback(timeout_source,
janus_plugin_close_pc_internal,
plugin_session, NULL);

GThread *watchdog = g_thread_try_new("timeout watchdog",
&janus_sessions_watchdog,
watchdog_loop, &error);

This suggests that janus_plugin_close_pc_internal is executed in the watchdog thread,
not in the handle’s main event loop thread.

2. The thread that invokes janus_ice_handle_destroy ( detach message)

On the other hand, when a detach is received, the path
janus_session_handles_remove() → janus_ice_handle_destroy()
is executed in the “sessions requests” thread:

GThread *requests_thread =
g_thread_try_new("sessions requests",
&janus_transport_requests,
NULL, &error);

Inside janus_ice_handle_destroy(), the following operations occur:
• janus_ice_detach_handle() is called
• which leads to g_source_destroy(handle->rtp_source)
• janus_refcount_decrease(&handle->ref) is performed

Given this, my understanding is that (1) and (2) are happening on different threads.

Because of this, I was wondering whether it would be safer to increment the
ice_handle reference count in janus_plugin_close_pc(), before scheduling
janus_plugin_close_pc_internal(), to make the lifetime guarantees explicit
across threads.

If my understanding of the threading or ownership model is incorrect,
I would greatly appreciate your correction.

[nanguantong]

The same issue: This issue is extremely difficult for us to reproduce deterministically. --- Random memory error with weak network by linux tc tools.

• leave and detach to each publisher , detach to each subscriber
• The server was running with ~1000 threads at the time of crash (high load)
  @lminiero

[nanguantong]

Add the "tcmalloc" lib and update the newest 3td deps-libs to fix this crash. --- Done.