Fix AI memory leaks #121

Manually merged

jabber.developer merged 4 commits from fix/ai-leaks into master 2026-05-15 14:21:40 +00:00

Conversation 1 Commits 4 Files Changed 2 +18 -15

jabber.developer commented 2026-05-15 09:20:10 +00:00

Owner

Copy Link

Fixes #120

Fixes #120

jabber.developer added 1 commit 2026-05-15 09:20:11 +00:00

fix(ai): fix memory leak in AI message stanza ID ... 5e329c77e1

Store stanza ID in an auto_gchar variable to ensure automatic memory cleanup.

jabber.developer2 commented 2026-05-15 10:27:22 +00:00

Collaborator

Copy Link

Branch: fix/ai-leaks → master · Closes: Fixes #120 · Commits: 5e329c77e, 7469f31c7, f9e0ba963

Coverage of issue #120

valgrind record	source	status
244 (60 B)	_load_identity in omemo.c:1434 (not AI)	unfixed
445 (784 B)	_write_callback via _ai_request_thread	fixed
447 (1008 B)	connection_create_stanza_id in cmd_funcs.c	fixed
474 (4434 B)	ai_session_create (worker never unref'd session)	fixed

3 of 4 closed. Record 244 is OMEMO, shall belongs to a separate PR.

What's correct

• Stanza id (auto_gchar) at cmd_funcs.c:8519 — g_strdup_printf-allocated string, scope-end g_free is safe; cl_ev_send_ai_msg uses the id only synchronously.
• local_provider_name / local_model / local_api_key → auto_gchar. Headers via _build_curl_headers copy the bearer value, so releasing the local after that call is safe.
• response.data → g_steal_pointer(&response.data) into auto_gchar response_data before the if/else chain — all three branches now free identically.
• Session refcount balance: ai_session_unref added in both early-exits and at the end of _ai_request_thread; cmd_ai_start releases its own ref after wins_new_ai because win_create_ai (window.c:344) takes its own.

---

Remaining items — all marked tolerable unless noted

Re-verified each against current HEAD; everything below is unchanged by this PR.

4. _write_callback returns realsize on g_realloc failure — tolerable

ai_client.c:66 — if g_realloc returns NULL the callback returns realsize, which (per libcurl semantics) says "all bytes written". curl would keep feeding more data into a buffer that didn't grow → silent data loss on OOM. Tolerable because g_realloc essentially never fails on modern systems before the 10 MB cap right above kicks in. Trivial fix though — return CURL_WRITEFUNC_ERROR; mirroring the cap path.

5. _load_identity GErrors leak — tolerable (this is record 244)

omemo.c 1434/1443/1451/1462 — g_set_error allocates, but _load_identity returns FALSE without g_clear_error(&error). Tolerable because it only fires when OMEMO identity load is already failing (broken keyfile), each leak is ~60 B, and it's outside AI scope. Belongs in a separate OMEMO PR.

6. Detached worker threads at shutdown — tolerable

g_thread_unref(thread) immediately after g_thread_new (1149, 1573) detaches. If profanity quits with a request in flight (60s CURLOPT_TIMEOUT), the thread is killed mid-flight and its locals are not released. Tolerable because the OS reclaims everything on process exit — this only shows up in valgrind output as noise. Out of scope for a leak-fix PR.

7. auto_char vs auto_gchar for connection_create_stanza_id() — tolerable

~70 places in the XMPP layer use auto_char (calls free) on a g_strdup_printf-allocated string. Tolerable on glibc because free and g_free are interchangeable there. Worth a separate sweep, not this PR.

8. _write_callback realloc is linear-quadratic — tolerable

ai_client.c:63 reallocs on every chunk. For a 1 MB response at ~16 KB chunks that's ~64 copies. Tolerable because the 10 MB cap puts a ceiling on it; only a perf concern under load.

9. Worker-thread cons_show fallback in _aiwin_display_response — tolerable

ai_client.c:138 — cons_show from a worker if the validated window is NULL. The surrounding global lock (profanity.h:46) is in practice the ncurses lock, so probably coordinated. Tolerable, but dropping the fallback (just log a warning) would be more defensive.

10. model may be a borrowed pointer — tolerable

cmd_funcs.c:10945 — ai_get_provider_default_model returns const gchar* borrowed from provider->default_model. Tolerable because all AI commands run on the main thread and ai_session_create copies via g_strdup before storing.

11. TOCTOU between API-key check in cmd_ai_start and the lookup in ai_session_create — tolerable

Key is fetched twice (cmd_funcs.c:10955, ai_client.c:1179). If rotated/removed between, session ends up empty-keyed. Tolerable: UX inconsistency, not a leak/crash.

12. _find_json_field does a bare strstr — tolerable

ai_client.c:898 — would match the field literal anywhere, including inside string values. Tolerable under well-formed JSON from real providers; could cause confusing parse failures with adversarial/unusual responses. A proper JSON parser is a follow-up issue.

13. ai_session_switch does an avoidable strdup of api_key — tolerable

ai_client.c:1311 + 1314 — session->api_key = g_strdup(api_key); g_free(api_key); despite already owning api_key (caller g_steal_pointers into it). Tolerable: wasted alloc/free pair per /ai switch, not a bug.

14. Provider hashtable lookup from worker threads is unlocked — tolerable (with caveat)

_ai_generic_request_thread (733) and _models_fetch_thread (1106, 1130) call ai_get_provider → unlocked g_hash_table_lookup. Refcounting protects struct lifetime but not the table read itself. Tolerable because /ai remove provider is a rare action that the user is unlikely to do mid-request, but a crash here is theoretically possible.

15. _aiwin_validate TOCTOU at shutdown — tolerable

ai_client.c:86-101 — wins_destroy doesn't take the global lock before destroying windows. Worker could deref a freed aiwin between validation and use. Tolerable: shutdown-only race, OS cleans up after.

16. assert(aiwin->memcheck == PROFAIWIN_MEMCHECK) may be no-op under NDEBUG — tolerable

If release builds set NDEBUG, the canary stops protecting in production. Tolerable as a debug-only safety net; turning it into a real conditional + abort would be a hardening pass, not a leak fix.

---

Suggested next steps

1. Fix local_provider leak in early-exits (+2 lines × 2 branches). Only blocker for merge.
2. Merge — this PR legitimately closes 3 of 4 leaks from #120.
3. Either drop Fixes for Refs #120 and keep #120 open until item 5 (OMEMO) lands, or close #120 and file a follow-up OMEMO issue.
4. Items 4, 13, 7 are quick separate one-shots if anyone has spare cycles. The rest are deliberate technical debt at acceptable levels.

**Branch:** `fix/ai-leaks` → `master` · **Closes:** `Fixes #120` · **Commits:** `5e329c77e`, `7469f31c7`, `f9e0ba963` ## Coverage of issue #120 | valgrind record | source | status | |---|---|---| | 244 (60 B) | `_load_identity` in `omemo.c:1434` (not AI) | unfixed | | 445 (784 B) | `_write_callback` via `_ai_request_thread` | fixed | | 447 (1008 B) | `connection_create_stanza_id` in `cmd_funcs.c` | fixed | | 474 (4434 B) | `ai_session_create` (worker never unref'd session) | fixed | 3 of 4 closed. Record 244 is OMEMO, shall belongs to a separate PR. ## What's correct - Stanza id (`auto_gchar`) at [cmd_funcs.c:8519](src/command/cmd_funcs.c#L8519) — `g_strdup_printf`-allocated string, scope-end `g_free` is safe; `cl_ev_send_ai_msg` uses the id only synchronously. - `local_provider_name` / `local_model` / `local_api_key` → `auto_gchar`. Headers via `_build_curl_headers` copy the bearer value, so releasing the local after that call is safe. - `response.data` → `g_steal_pointer(&response.data)` into `auto_gchar response_data` *before* the if/else chain — all three branches now free identically. - Session refcount balance: `ai_session_unref` added in both early-exits and at the end of `_ai_request_thread`; `cmd_ai_start` releases its own ref after `wins_new_ai` because `win_create_ai` ([window.c:344](src/ui/window.c#L344)) takes its own. --- ## Remaining items — all marked *tolerable* unless noted Re-verified each against current HEAD; everything below is unchanged by this PR. ### 4. `_write_callback` returns `realsize` on `g_realloc` failure — *tolerable* [ai_client.c:66](src/ai/ai_client.c#L66) — if `g_realloc` returns NULL the callback returns `realsize`, which (per libcurl semantics) says "all bytes written". curl would keep feeding more data into a buffer that didn't grow → silent data loss on OOM. **Tolerable** because `g_realloc` essentially never fails on modern systems before the 10 MB cap right above kicks in. Trivial fix though — `return CURL_WRITEFUNC_ERROR;` mirroring the cap path. ### 5. `_load_identity` GErrors leak — *tolerable (this is record 244)* `omemo.c` 1434/1443/1451/1462 — `g_set_error` allocates, but `_load_identity` returns FALSE without `g_clear_error(&error)`. **Tolerable** because it only fires when OMEMO identity load is already failing (broken keyfile), each leak is ~60 B, and it's outside AI scope. Belongs in a separate OMEMO PR. ### 6. Detached worker threads at shutdown — *tolerable* `g_thread_unref(thread)` immediately after `g_thread_new` ([1149](src/ai/ai_client.c#L1149), [1573](src/ai/ai_client.c#L1573)) detaches. If profanity quits with a request in flight (60s `CURLOPT_TIMEOUT`), the thread is killed mid-flight and its locals are not released. **Tolerable** because the OS reclaims everything on process exit — this only shows up in valgrind output as noise. Out of scope for a leak-fix PR. ### 7. `auto_char` vs `auto_gchar` for `connection_create_stanza_id()` — *tolerable* ~70 places in the XMPP layer use `auto_char` (calls `free`) on a `g_strdup_printf`-allocated string. **Tolerable** on glibc because `free` and `g_free` are interchangeable there. Worth a separate sweep, not this PR. ### 8. `_write_callback` realloc is linear-quadratic — *tolerable* [ai_client.c:63](src/ai/ai_client.c#L63) reallocs on every chunk. For a 1 MB response at ~16 KB chunks that's ~64 copies. **Tolerable** because the 10 MB cap puts a ceiling on it; only a perf concern under load. ### 9. Worker-thread `cons_show` fallback in `_aiwin_display_response` — *tolerable* [ai_client.c:138](src/ai/ai_client.c#L138) — `cons_show` from a worker if the validated window is NULL. The surrounding global `lock` ([profanity.h:46](src/profanity.h#L46)) is in practice the ncurses lock, so probably coordinated. **Tolerable**, but dropping the fallback (just log a warning) would be more defensive. ### 10. `model` may be a borrowed pointer — *tolerable* [cmd_funcs.c:10945](src/command/cmd_funcs.c#L10945) — `ai_get_provider_default_model` returns `const gchar*` borrowed from `provider->default_model`. **Tolerable** because all AI commands run on the main thread and `ai_session_create` copies via `g_strdup` before storing. ### 11. TOCTOU between API-key check in `cmd_ai_start` and the lookup in `ai_session_create` — *tolerable* Key is fetched twice ([cmd_funcs.c:10955](src/command/cmd_funcs.c#L10955), [ai_client.c:1179](src/ai/ai_client.c#L1179)). If rotated/removed between, session ends up empty-keyed. **Tolerable**: UX inconsistency, not a leak/crash. ### 12. `_find_json_field` does a bare `strstr` — *tolerable* [ai_client.c:898](src/ai/ai_client.c#L898) — would match the field literal anywhere, including inside string values. **Tolerable** under well-formed JSON from real providers; could cause confusing parse failures with adversarial/unusual responses. A proper JSON parser is a follow-up issue. ### 13. `ai_session_switch` does an avoidable strdup of `api_key` — *tolerable* [ai_client.c:1311 + 1314](src/ai/ai_client.c#L1311) — `session->api_key = g_strdup(api_key); g_free(api_key);` despite already owning `api_key` (caller `g_steal_pointer`s into it). **Tolerable**: wasted alloc/free pair per `/ai switch`, not a bug. ### 14. Provider hashtable lookup from worker threads is unlocked — *tolerable (with caveat)* `_ai_generic_request_thread` ([733](src/ai/ai_client.c#L733)) and `_models_fetch_thread` ([1106](src/ai/ai_client.c#L1106), [1130](src/ai/ai_client.c#L1130)) call `ai_get_provider` → unlocked `g_hash_table_lookup`. Refcounting protects struct lifetime but not the table read itself. **Tolerable** because `/ai remove provider` is a rare action that the user is unlikely to do mid-request, but a crash here is theoretically possible. ### 15. `_aiwin_validate` TOCTOU at shutdown — *tolerable* [ai_client.c:86-101](src/ai/ai_client.c#L86-L101) — `wins_destroy` doesn't take the global `lock` before destroying windows. Worker could deref a freed `aiwin` between validation and use. **Tolerable**: shutdown-only race, OS cleans up after. ### 16. `assert(aiwin->memcheck == PROFAIWIN_MEMCHECK)` may be no-op under `NDEBUG` — *tolerable* If release builds set `NDEBUG`, the canary stops protecting in production. **Tolerable** as a debug-only safety net; turning it into a real conditional + abort would be a hardening pass, not a leak fix. --- ## Suggested next steps 1. **Fix `local_provider` leak in early-exits** (+2 lines × 2 branches). Only blocker for merge. 2. Merge — this PR legitimately closes 3 of 4 leaks from #120. 3. Either drop `Fixes` for `Refs #120` and keep #120 open until item 5 (OMEMO) lands, or close #120 and file a follow-up OMEMO issue. 4. Items 4, 13, 7 are quick separate one-shots if anyone has spare cycles. The rest are deliberate technical debt at acceptable levels.

jabber.developer added 1 commit 2026-05-15 11:29:13 +00:00

fix(ai): fix memory leaks in _ai_request_thread ... 7469f31c78

Replace manual g_free calls with auto_gchar for local_provider_name,
local_model, local_api_key, and response_data to ensure automatic
cleanup and prevent memory leaks.

jabber.developer added 1 commit 2026-05-15 11:58:29 +00:00

fix(ai): fix memory leaks in ai session handling ... f9e0ba9630

Add missing `ai_session_unref` calls to prevent memory leaks.

- In `_ai_request_thread`, release session on error paths and
  after successful processing.
- In `cmd_ai_start`, release reference after passing ownership
  to the AI window.

jabber.developer self-assigned this 2026-05-15 12:21:26 +00:00

jabber.developer requested review from jabber.developer2 2026-05-15 12:21:40 +00:00

jabber.developer added 1 commit 2026-05-15 14:14:14 +00:00

fix(ai): fix memory leak of local provider in error paths ... 06b80bc89a

Add missing unref calls for local_provider in _ai_request_thread
error handling to prevent memory leaks.

jabber.developer manually merged commit 06b80bc89a into master 2026-05-15 14:21:40 +00:00

jabber.developer referenced this pull request 2026-05-15 14:23:03 +00:00

[AI-feature] Memory leaks #120

Sign in to join this conversation.

Reviewers

No Reviewers

jabber.developer2

Labels

Clear labels

Compat/Breaking

Breaking change that won't be backward compatible

Kind/Bug

Something is not working

Kind/Documentation

Documentation changes

Kind/Enhancement

Improve existing functionality

Kind/Feature

New functionality

Kind/Security

This is security issue

Kind/Testing

Issue or pull request related to testing

Priority

Critical

1

The priority is critical

Priority

High

2

The priority is high

Priority

Low

4

The priority is low

Priority

Medium

3

The priority is medium

Reviewed

Confirmed

1

Issue has been confirmed

Reviewed

Duplicate

2

This issue or pull request already exists

Reviewed

Invalid

3

Invalid issue

Reviewed

Won't Fix

3

This issue won't be fixed

Status

Abandoned

3

Somebody has started to work on this but abandoned work

Status

Blocked

1

Something is blocking this issue or pull request

Status

Need More Info

2

Feedback is required to reproduce issue or to continue work

No Label

Milestone

No items

No Milestone

Projects

Clear projects

No project

Assignees

Clear assignees

admin jabber.developer jabber.developer2

No Assignees jabber.developer

2 Participants

Notifications

Subscribe

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: devs/cproof#121

No description provided.