Fix(ai): protect AI session state from concurrent access races
Some checks failed
CI Code / Check spelling (pull_request) Successful in 20s
CI Code / Check coding style (pull_request) Successful in 35s
CI Code / Linux (debian) (pull_request) Failing after 3m47s
CI Code / Linux (ubuntu) (pull_request) Failing after 3m54s
CI Code / Code Coverage (pull_request) Successful in 6m41s
CI Code / Linux (arch) (pull_request) Failing after 9m31s
Some checks failed
CI Code / Check spelling (pull_request) Successful in 20s
CI Code / Check coding style (pull_request) Successful in 35s
CI Code / Linux (debian) (pull_request) Failing after 3m47s
CI Code / Linux (ubuntu) (pull_request) Failing after 3m54s
CI Code / Code Coverage (pull_request) Successful in 6m41s
CI Code / Linux (arch) (pull_request) Failing after 9m31s
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
This commit is contained in:
@@ -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
|
||||
* ======================================================================== */
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -51,7 +51,6 @@
|
||||
#include <unistd.h>
|
||||
#include <langinfo.h>
|
||||
#include <ctype.h>
|
||||
#include <pthread.h>
|
||||
|
||||
// fork / execl
|
||||
#include <sys/types.h>
|
||||
@@ -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);
|
||||
|
||||
Reference in New Issue
Block a user