From 9f4621883a783f3c7c3ee40594bbdad6888e24da Mon Sep 17 00:00:00 2001 From: Jabber Developer Date: Mon, 11 May 2026 23:26:14 +0000 Subject: [PATCH] fix(ai): add mutex to AISession for thread safety Introduce a pthread mutex to protect all AISession fields from concurrent access between the main thread and background request worker. - Initialize and destroy mutex during session lifecycle - Lock session state when modifying or reading history, model, and provider fields - Snapshot session state under lock in the request worker thread to prevent UAF races during API calls - Refactor _build_json_payload to accept direct parameters for safe usage under lock - Update command handlers to safely switch provider/model under lock --- src/ai/ai_client.c | 105 ++++++++++++++++++++++++++++++---------- src/ai/ai_client.h | 40 +++++++++++++++ src/command/cmd_funcs.c | 12 ++++- 3 files changed, 131 insertions(+), 26 deletions(-) diff --git a/src/ai/ai_client.c b/src/ai/ai_client.c index f36767c2..5023d4ec 100644 --- a/src/ai/ai_client.c +++ b/src/ai/ai_client.c @@ -21,6 +21,7 @@ #include #include +#include #include /* Default providers */ @@ -1037,6 +1038,11 @@ ai_session_create(const gchar* provider_name, const gchar* model) } AISession* session = g_new0(AISession, 1); + if (pthread_mutex_init(&session->lock, NULL) != 0) { + log_error("Failed to initialize session mutex"); + g_free(session); + return NULL; + } session->provider_name = g_strdup(provider_name); session->provider = ai_provider_ref(provider); session->model = g_strdup(model); @@ -1067,6 +1073,7 @@ ai_session_unref(AISession* session) return; } + pthread_mutex_lock(&session->lock); g_free(session->provider_name); ai_provider_unref(session->provider); g_free(session->model); @@ -1081,6 +1088,7 @@ ai_session_unref(AISession* session) curr = g_list_next(curr); } g_list_free(session->history); + pthread_mutex_unlock(&session->lock); g_free(session); log_debug("AI session destroyed"); @@ -1092,10 +1100,12 @@ ai_session_add_message(AISession* session, const gchar* role, const gchar* conte if (!session || !role || !content) return; + pthread_mutex_lock(&session->lock); AIMessage* msg = g_new0(AIMessage, 1); msg->role = g_strdup(role); msg->content = g_strdup(content); session->history = g_list_append(session->history, msg); + pthread_mutex_unlock(&session->lock); log_debug("Added %s message to session (total: %d)", role, g_list_length(session->history)); } @@ -1106,6 +1116,7 @@ ai_session_clear_history(AISession* session) if (!session) return; + pthread_mutex_lock(&session->lock); GList* curr = session->history; while (curr) { AIMessage* msg = curr->data; @@ -1116,6 +1127,7 @@ ai_session_clear_history(AISession* session) } g_list_free(session->history); session->history = NULL; + pthread_mutex_unlock(&session->lock); log_info("AI session history cleared"); } @@ -1134,8 +1146,10 @@ ai_session_set_model(AISession* session, const gchar* model) if (!session || !model) return; + pthread_mutex_lock(&session->lock); g_free(session->model); session->model = g_strdup(model); + pthread_mutex_unlock(&session->lock); log_info("Session model changed to: %s", model); } @@ -1143,15 +1157,21 @@ ai_session_set_model(AISession* session, const gchar* model) * API Request Handling * ======================================================================== */ +/** + * Build JSON payload from a GList of AIMessage nodes and a prompt string. + * This is the thread-safe variant used by _ai_request_thread() which holds + * the session lock while calling it. + * + * @param model The model identifier + * @param history GList of AIMessage* (must be held under session lock) + * @param prompt The new user prompt to append + * @return Newly allocated JSON string (caller must free) + */ static gchar* -_build_json_payload(AISession* session, const gchar* prompt) +_build_json_payload_from_list(const gchar* model, GList* history, const gchar* prompt) { - - /* OpenAI-compatible Responses API format: - * {"model": "...", "input": [...], "stream": false, "store": false} - * store:false prevents providers from storing/using requests for training */ GString* messages_json = g_string_new(""); - GList* curr = session->history; + GList* curr = history; while (curr) { AIMessage* msg = curr->data; @@ -1174,7 +1194,7 @@ _build_json_payload(AISession* session, const gchar* prompt) } g_string_append_printf(messages_json, "{\"role\":\"user\",\"content\":\"%s\"}", escaped_prompt); - auto_gchar gchar* escaped_model = ai_json_escape(session->model); + auto_gchar gchar* escaped_model = ai_json_escape(model); gchar* json_payload = g_strdup_printf( "{\"model\":\"%s\",\"input\":[%s],\"stream\":false,\"store\":false}", escaped_model, messages_json->str); @@ -1335,14 +1355,31 @@ _ai_request_thread(gpointer data) auto_gchar gchar* prompt = args[1]; gpointer user_data = args[2]; - log_debug("[AI-THREAD] Session: %s/%s", session->provider_name, session->model); - log_debug("[AI-THREAD] API key length: %zu", session->api_key ? strlen(session->api_key) : 0); + /* ===================================================================== + * CRITICAL: Copy all session state under lock to prevent UAF races with + * cmd_ai_switch() and ai_session_clear_history() which mutate session + * fields on the main thread without holding a session ref. + * + * The worker holds a ref to the session (via ai_session_ref in + * ai_send_prompt), which prevents the session struct from being freed, + * but does NOT prevent field mutations (g_free/g_strdup of strings, + * list modifications). We must snapshot all fields atomically. + * ===================================================================== */ + pthread_mutex_lock(&session->lock); + gchar* local_provider_name = g_strdup(session->provider_name); + AIProvider* local_provider = ai_provider_ref(session->provider); + gchar* local_model = g_strdup(session->model); + gchar* local_api_key = g_strdup(session->api_key ? session->api_key : ""); + pthread_mutex_unlock(&session->lock); + + log_debug("[AI-THREAD] Session: %s/%s", local_provider_name, local_model); + log_debug("[AI-THREAD] API key length: %zu", strlen(local_api_key)); /* Check for API key first */ - if (!session->api_key || strlen(session->api_key) == 0) { + if (!local_api_key || strlen(local_api_key) == 0) { auto_gchar gchar* error_msg = g_strdup_printf("No API key set for provider '%s'. Use '/ai set token %s ' to configure.", - session->provider_name, session->provider_name); - log_error("AI request failed for %s/%s: %s", session->provider_name, session->model, error_msg); + local_provider_name, local_provider_name); + log_error("AI request failed for %s/%s: %s", local_provider_name, local_model, error_msg); _aiwin_display_error(user_data, error_msg); g_free(args); return NULL; @@ -1352,23 +1389,30 @@ _ai_request_thread(gpointer data) log_debug("[AI-THREAD] Curl initialized: %s", curl ? "OK" : "FAILED"); if (!curl) { log_error("AI request failed for %s/%s: Failed to initialize curl", - session->provider_name, session->model); + local_provider_name, local_model); _aiwin_display_error(user_data, "Failed to initialize curl."); g_free(args); return NULL; } - /* Add user message to history FIRST */ - ai_session_add_message(session, "user", prompt); + /* Add user message to history FIRST (under lock) */ + pthread_mutex_lock(&session->lock); + AIMessage* user_msg = g_new0(AIMessage, 1); + user_msg->role = g_strdup("user"); + user_msg->content = g_strdup(prompt); + session->history = g_list_append(session->history, user_msg); + pthread_mutex_unlock(&session->lock); log_debug("[AI-THREAD] Added user message to history"); - /* Build JSON payload (includes the message we just added) */ + /* Build JSON payload from local history copy (under lock) */ log_debug("[AI-THREAD] Building JSON payload..."); - auto_gchar gchar* json_payload = _build_json_payload(session, prompt); + pthread_mutex_lock(&session->lock); + auto_gchar gchar* json_payload = _build_json_payload_from_list(local_model, session->history, prompt); + pthread_mutex_unlock(&session->lock); log_debug("[AI-THREAD] JSON payload: %s", json_payload); /* Build headers using shared helper */ - struct curl_slist* headers = _build_curl_headers(session->provider, session->api_key); + struct curl_slist* headers = _build_curl_headers(local_provider, local_api_key); /* Response buffer */ struct curl_response_t response; @@ -1376,11 +1420,11 @@ _ai_request_thread(gpointer data) response.size = 0; /* Build request URL */ - const gchar* api_url = session->provider ? session->provider->api_url : DEFAULT_OPENAI_URL; + const gchar* api_url = local_provider ? local_provider->api_url : DEFAULT_OPENAI_URL; auto_gchar gchar* request_url = g_strdup_printf("%s%sv1/responses", api_url, g_str_has_suffix(api_url, "/") ? "" : "/"); log_debug("[AI-THREAD] API URL: %s", api_url); log_debug("[AI-THREAD] API Request URL: %s", request_url); - log_debug("[AI-THREAD] Model: %s", session->model); + log_debug("[AI-THREAD] Model: %s", local_model); /* Set URL and execute request */ curl_easy_setopt(curl, CURLOPT_URL, request_url); @@ -1400,7 +1444,7 @@ _ai_request_thread(gpointer data) if (res != CURLE_OK) { auto_gchar gchar* error_msg = g_strdup(curl_easy_strerror(res)); - log_error("AI request failed for %s/%s: %s", session->provider_name, session->model, error_msg); + log_error("AI request failed for %s/%s: %s", local_provider_name, local_model, error_msg); _aiwin_display_error(user_data, error_msg); } else if (http_code >= 400) { /* Handle HTTP errors */ @@ -1409,7 +1453,7 @@ _ai_request_thread(gpointer data) /* Try to extract the actual error message from the JSON response */ auto_gchar gchar* parsed_error = _parse_error_response(response.data); auto_gchar gchar* error_msg = parsed_error ? g_strdup_printf("HTTP %ld: %s", http_code, parsed_error) : g_strdup_printf("HTTP %ld: %s", http_code, response.data && strlen(response.data) > 0 ? response.data : "Unknown error"); - log_error("AI request failed for %s/%s: %s", session->provider_name, session->model, error_msg); + log_error("AI request failed for %s/%s: %s", local_provider_name, local_model, error_msg); _aiwin_display_error(user_data, error_msg); } else { /* Parse response - transfer ownership to auto_gchar for cleanup */ @@ -1418,12 +1462,17 @@ _ai_request_thread(gpointer data) response.data = NULL; auto_gchar gchar* content = _parse_ai_response(response_data); if (content) { - /* Add assistant response to history */ - ai_session_add_message(session, "assistant", content); + /* Add assistant response to history (under lock) */ + pthread_mutex_lock(&session->lock); + AIMessage* assistant_msg = g_new0(AIMessage, 1); + assistant_msg->role = g_strdup("assistant"); + assistant_msg->content = g_strdup(content); + session->history = g_list_append(session->history, assistant_msg); + pthread_mutex_unlock(&session->lock); _aiwin_display_response(user_data, content); } else { log_error("AI response parse failed for %s/%s: %.200s...", - session->provider_name, session->model, response_data); + local_provider_name, local_model, response_data); _aiwin_display_error(user_data, "Failed to parse AI response."); } } @@ -1431,6 +1480,12 @@ _ai_request_thread(gpointer data) curl_slist_free_all(headers); curl_easy_cleanup(curl); + /* Cleanup local copies */ + g_free(local_provider_name); + ai_provider_unref(local_provider); + g_free(local_model); + g_free(local_api_key); + g_free(args); return NULL; diff --git a/src/ai/ai_client.h b/src/ai/ai_client.h index 7728b3ab..840b1cfb 100644 --- a/src/ai/ai_client.h +++ b/src/ai/ai_client.h @@ -32,6 +32,7 @@ typedef struct ai_provider_t */ typedef struct ai_session_t { + pthread_mutex_t lock; /* Protects all session fields below */ gchar* provider_name; /* Provider name */ AIProvider* provider; /* Provider configuration */ gchar* model; /* Model identifier (e.g., "gpt-4", "sonar") */ @@ -238,6 +239,45 @@ 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 + * _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); + /* ======================================================================== * Request Handling * ======================================================================== */ diff --git a/src/command/cmd_funcs.c b/src/command/cmd_funcs.c index 9d6678d4..8e59f76f 100644 --- a/src/command/cmd_funcs.c +++ b/src/command/cmd_funcs.c @@ -51,6 +51,7 @@ #include #include #include +#include // fork / execl #include @@ -11096,7 +11097,14 @@ 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; + } model = arg1; } @@ -11108,7 +11116,8 @@ cmd_ai_switch(ProfWin* window, const char* const command, gchar** args) return TRUE; } - // Update the existing session's provider and model + // 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); @@ -11121,6 +11130,7 @@ cmd_ai_switch(ProfWin* window, const char* const command, gchar** args) 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); // Update window title win_println((ProfWin*)aiwin, THEME_DEFAULT, "-", "AI Chat: %s/%s", provider_name, model);