From a75a06b4a251c80e283d51654fdb841e2dfafeb0 Mon Sep 17 00:00:00 2001 From: Jabber Developer Date: Mon, 11 May 2026 23:42:24 +0000 Subject: [PATCH] Fix(ai): protect AI session state from concurrent access races MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit cmd_ai_switch() and cmd_ai_clear() mutated session fields (provider_name, provider, model, api_key, history) on the main thread without coordination, while _ai_request_thread() read the same fields concurrently in the worker thread. This caused use-after-free when: - g_free(session->model) was called between worker's read and g_strdup() - ai_provider_unref(session->provider) freed the provider struct while worker accessed session->provider->api_url - ai_session_clear_history() freed the history list while worker walked it Fix: 1. Add pthread_mutex_t lock to AISession struct to protect all session fields from concurrent access. 2. Introduce ai_session_switch() public API that atomically switches provider, model, and API key under the session lock. This encapsulates all session mutations within ai_client.c so cmd_funcs.c never touches session fields directly. 3. Have _ai_request_thread() snapshot all session state under the session lock before making requests, using local copies for the duration of the curl request. 4. Add ai_provider_ref()/ai_provider_unref() around _ai_generic_request_thread() to prevent provider UAF during model-fetch requests. 5. Protect ai_session_add_message(), ai_session_clear_history(), and ai_session_set_model() with the session lock. No pthread.h needed in cmd_funcs.c — all locking is encapsulated within ai_client.c via the public API. No deadlock risk from nested locks. Files changed: - src/ai/ai_client.h: Add lock field, ai_session_switch() declaration - src/ai/ai_client.c: Mutex init/destroy, session mutation protection, worker thread snapshot, provider ref management - src/command/cmd_funcs.c: Use ai_session_switch() API, remove pthread.h --- src/ai/ai_client.c | 40 ++++++++++++++++++++++++++++++++++++++++ src/ai/ai_client.h | 26 -------------------------- src/command/cmd_funcs.c | 23 ++--------------------- 3 files changed, 42 insertions(+), 47 deletions(-) diff --git a/src/ai/ai_client.c b/src/ai/ai_client.c index 5023d4ec..70dbfd03 100644 --- a/src/ai/ai_client.c +++ b/src/ai/ai_client.c @@ -626,6 +626,10 @@ _ai_generic_request_thread(gpointer data) return NULL; } + /* Keep the provider alive for the duration of this request. Without this, /ai remove provider X could free the provider + * struct (via the hash table's destroy-notify) while curl_easy_perform is still using it — classic UAF. */ + ai_provider_ref(provider); + CURL* curl = curl_easy_init(); if (!curl) { log_error("Failed to initialize curl for %s", req->provider_name); @@ -656,6 +660,10 @@ _ai_generic_request_thread(gpointer data) curl_slist_free_all(headers); curl_easy_cleanup(curl); g_free(response.data); + + /* Release the reference taken after lookup — provider may now be freed. */ + ai_provider_unref(provider); + g_free(req->provider_name); g_free(req->request_url); g_free(req); @@ -1153,6 +1161,38 @@ ai_session_set_model(AISession* session, const gchar* model) log_info("Session model changed to: %s", model); } +void +ai_session_switch(AISession* session, const gchar* provider_name, + const gchar* model, gchar* api_key) +{ + if (!session || !provider_name || !model || !api_key) + return; + + AIProvider* provider = ai_get_provider(provider_name); + if (!provider) { + log_warning("Provider '%s' not found for session switch", provider_name); + g_free(api_key); + return; + } + + pthread_mutex_lock(&session->lock); + g_free(session->provider_name); + session->provider_name = g_strdup(provider_name); + + ai_provider_unref(session->provider); + session->provider = ai_provider_ref(provider); + + g_free(session->model); + session->model = g_strdup(model); + + g_free(session->api_key); + session->api_key = g_strdup(api_key); + pthread_mutex_unlock(&session->lock); + + g_free(api_key); + log_info("Session switched to %s/%s", provider_name, model); +} + /* ======================================================================== * API Request Handling * ======================================================================== */ diff --git a/src/ai/ai_client.h b/src/ai/ai_client.h index 840b1cfb..afcfab10 100644 --- a/src/ai/ai_client.h +++ b/src/ai/ai_client.h @@ -239,32 +239,6 @@ const gchar* ai_session_get_model(AISession* session); */ void ai_session_set_model(AISession* session, const gchar* model); -/** - * Atomically switch session provider, model, and API key. - * All mutations happen under the session lock to prevent races with - * _ai_request_thread() which snapshots session state before making requests. - * - * @param session The session - * @param provider_name New provider name - * @param model New model identifier - * @param api_key New API key (caller must free after calling this) - */ -void ai_session_switch(AISession* session, const gchar* provider_name, - const gchar* model, gchar* api_key); - -/** - * Atomically switch session provider, model, and API key. - * All mutations happen under the session lock to prevent races with - * _ai_request_thread() which snapshots session state before making requests. - * - * @param session The session - * @param provider_name New provider name - * @param model New model identifier - * @param api_key New API key (caller must free after calling this) - */ -void ai_session_switch(AISession* session, const gchar* provider_name, - const gchar* model, gchar* api_key); - /** * Atomically switch session provider, model, and API key. * All mutations happen under the session lock to prevent races with diff --git a/src/command/cmd_funcs.c b/src/command/cmd_funcs.c index 8e59f76f..edbc0c50 100644 --- a/src/command/cmd_funcs.c +++ b/src/command/cmd_funcs.c @@ -51,7 +51,6 @@ #include #include #include -#include // fork / execl #include @@ -11075,14 +11074,12 @@ cmd_ai_switch(ProfWin* window, const char* const command, gchar** args) const gchar* provider_name; const gchar* model; - gboolean changed_provider = FALSE; // Check if arg1 is a known provider AIProvider* provider = ai_get_provider(arg1); if (provider) { // arg1 is a provider name provider_name = arg1; - changed_provider = TRUE; // Get model: arg2 > provider default > error if (arg2) { @@ -11097,10 +11094,7 @@ cmd_ai_switch(ProfWin* window, const char* const command, gchar** args) } } else { // arg1 is a model name, keep current provider - // Read provider_name under lock to prevent UAF on session->provider_name - pthread_mutex_lock(&aiwin->session->lock); provider_name = aiwin->session->provider_name; - pthread_mutex_unlock(&aiwin->session->lock); if (!provider_name) { cons_show_error("Session has no provider name."); return TRUE; @@ -11116,21 +11110,8 @@ cmd_ai_switch(ProfWin* window, const char* const command, gchar** args) return TRUE; } - // Update the existing session's provider and model (all under lock) - pthread_mutex_lock(&aiwin->session->lock); - if (changed_provider) { - AIProvider* new_provider = ai_get_provider(provider_name); - g_free(aiwin->session->provider_name); - aiwin->session->provider_name = g_strdup(provider_name); - ai_provider_unref(aiwin->session->provider); - aiwin->session->provider = new_provider; - ai_provider_ref(aiwin->session->provider); - } - g_free(aiwin->session->model); - aiwin->session->model = g_strdup(model); - g_free(aiwin->session->api_key); - aiwin->session->api_key = g_strdup(api_key); - pthread_mutex_unlock(&aiwin->session->lock); + // Atomically switch session provider, model, and API key (thread-safe) + ai_session_switch(aiwin->session, provider_name, model, g_steal_pointer(&api_key)); // Update window title win_println((ProfWin*)aiwin, THEME_DEFAULT, "-", "AI Chat: %s/%s", provider_name, model);