diff --git a/plans/memory_leak_investigation_plan.md b/plans/memory_leak_investigation_plan.md new file mode 100644 index 0000000..0c76e78 --- /dev/null +++ b/plans/memory_leak_investigation_plan.md @@ -0,0 +1,207 @@ +# C-Relay Memory Leak Investigation & Fix Plan + +## Problem Statement + +c-relay reached **1.56 GB** of its 1.5 GB `MemoryHigh` cgroup limit after only **1 hour 37 minutes** of runtime. The kernel is actively throttling the process with swap pressure (1.8M swap used). This strongly indicates one or more memory leaks causing unbounded growth. + +## Root Cause Analysis + +After thorough code review, I identified **three categories** of memory issues: + +1. **Confirmed `get_config_value()` leaks** — the most pervasive issue +2. **Potential structural leaks** in subscription/connection lifecycle +3. **Stack pressure** from large stack-allocated arrays + +--- + +## Category 1: `get_config_value()` Leaks (HIGH SEVERITY — Most Likely Root Cause) + +### The Core Problem + +[`get_config_value()`](src/config.c:293) calls [`get_config_value_from_table()`](src/config.c:1690) which returns `strdup(value)` — a **heap-allocated string that the caller must free**. Many call sites treat the return value as a borrowed pointer and never free it. + +**Every unfree'd call leaks ~64-256 bytes per invocation.** On a busy relay processing hundreds of events/second, this accumulates to GB-scale leaks within hours. + +### Confirmed Leak Sites + +#### `websockets.c` — Called on EVERY incoming event + +| Line | Code | Frequency | Severity | +|------|------|-----------|----------| +| [1527](src/websockets.c:1527) | `get_config_value("relay_pubkey")` — auth bypass check for kind 14 DMs | Every kind-14 event | **HIGH** | +| [1549](src/websockets.c:1549) | `get_config_value("admin_pubkey")` — auth bypass check for kind 23456 | Every kind-23456 event | **MEDIUM** | +| [2704](src/websockets.c:2704) | `get_config_value("relay_pubkey")` — DM stats command processing | Every DM to relay | **MEDIUM** | +| [2742](src/websockets.c:2742) | `get_config_value("admin_pubkey")` — DM admin check | Every DM to relay | **MEDIUM** | + +#### `main.c` — Called on EVERY admin event check + +| Line | Code | Frequency | Severity | +|------|------|-----------|----------| +| [1718](src/main.c:1718) | `get_config_value("relay_pubkey")` — inside a loop iterating tags | Every admin event, per tag | **CRITICAL** | +| [1742](src/main.c:1742) | `get_config_value("admin_pubkey")` — admin authorization | Every admin event | **HIGH** | + +#### `config.c` — Called on EVERY event kind check + +| Line | Code | Frequency | Severity | +|------|------|-----------|----------| +| [377](src/config.c:377) | `get_config_value("nip42_auth_required_kinds")` — NIP-42 kind check | Every incoming event | **CRITICAL** | + +#### `ip_ban.c` — Called on EVERY ban check + +| Line | Code | Frequency | Severity | +|------|------|-----------|----------| +| [255](src/ip_ban.c:255) | `get_config_value("idle_ban_whitelist")` — whitelist check | Every connection check | **HIGH** | + +#### `nip042.c` — Called on every auth verification + +| Line | Code | Frequency | Severity | +|------|------|-----------|----------| +| [99](src/nip042.c:99) | `get_config_value("relay_url")` — relay URL for auth verification | Every NIP-42 auth | **MEDIUM** | + +#### `request_validator.c` — Called on every auth rules check + +| Line | Code | Frequency | Severity | +|------|------|-----------|----------| +| [208](src/request_validator.c:208) | `get_config_value("auth_enabled")` — properly freed | N/A | OK | +| [216](src/request_validator.c:216) | `get_config_value("auth_rules_enabled")` — properly freed | N/A | OK | +| [304](src/request_validator.c:304) | `get_config_value("admin_pubkey")` — NOT freed | Every event validation | **HIGH** | +| [320](src/request_validator.c:320) | `get_config_value("nip42_auth_enabled")` — NOT freed | Every event validation | **HIGH** | + +### Files That DO Free Correctly (for reference) + +- [`nip011.c`](src/nip011.c:127) — Properly frees all `get_config_value()` results +- [`nip013.c`](src/nip013.c:43) — Properly frees `pow_mode` +- [`request_validator.c`](src/request_validator.c:191) — Properly frees `nip42_timeout` and `nip42_tolerance` + +### Estimated Impact + +On a relay processing ~100 events/second: +- `is_nip42_auth_required_for_kind()` leaks ~100-200 bytes × 100/sec = **~10-20 KB/sec** +- `ip_is_whitelisted()` leaks ~100 bytes × connections/sec +- `is_authorized_admin_event()` leaks inside tag loop — potentially **multiple leaks per event** +- Combined: **~50-100 KB/sec → ~180-360 MB/hour** — consistent with the observed 1.56 GB in 97 minutes + +--- + +## Category 2: Structural Lifecycle Leaks (MEDIUM SEVERITY) + +### 2a. `LWS_CALLBACK_CLOSED` — Inactive Subscription Skip + +In [`websockets.c:2339`](src/websockets.c:2339), the cleanup handler only collects subscription IDs where `sub->active` is true: + +```c +if (sub->active) { // Only process active subscriptions + temp_sub_id_t* temp = malloc(sizeof(temp_sub_id_t)); +``` + +If a subscription was marked inactive by another thread between the time it was added and the connection close, it will be **skipped** — never removed from the global manager, never freed. The subscription object, its filters, and all cJSON objects within leak permanently. + +### 2b. `connection_list_remove` Potential + +The `connection_list_remove(wsi)` call at [`websockets.c:2252`](src/websockets.c:2252) happens before pss cleanup. Need to verify the connection list implementation doesn't hold references that prevent cleanup. + +### 2c. `ip_connection_info_t` Leak on Disconnect + +The per-IP connection tracking in [`subscriptions.h:79`](src/subscriptions.h:79) creates `ip_connection_info_t` nodes via `calloc`. These are only removed by `remove_ip_connection()` — need to verify this is called on every disconnect path. + +--- + +## Category 3: Stack Pressure (LOW SEVERITY but notable) + +### 3a. `candidates_to_check` Stack Array + +In [`subscriptions.c:810`](src/subscriptions.c:810): + +```c +subscription_t* candidates_to_check[MAX_TOTAL_SUBSCRIPTIONS]; // 5000 × 8 bytes = 40 KB +``` + +This allocates **40 KB on the stack** for every `broadcast_event_to_subscriptions()` call. While not a heap leak, it contributes to high memory pressure under concurrent load. + +### 3b. `added_kinds` Bitmap + +In [`subscriptions.c:68`](src/subscriptions.c:68): + +```c +unsigned char added_kinds[8192] = {0}; // 8 KB on stack +``` + +--- + +## Fix Implementation Plan + +### Phase 1: Fix `get_config_value()` Leaks (Highest Impact) + +**Strategy A — Preferred: Add a config cache layer** + +Instead of fixing 20+ call sites, add a **cached config system** that reads values once and caches them in memory, invalidating on config change events. This eliminates both the leak AND the repeated SQLite queries per event. + +``` +Config Cache Architecture: +- Static hash table of key-value pairs +- Populated at startup and on config change events +- get_config_value_cached() returns const pointer to cached value (no allocation) +- Existing get_config_value() kept for dynamic/rare lookups +``` + +**Strategy B — Quick fix: Free at every call site** + +Add `free((char*)result)` after every `get_config_value()` call that doesn't already free. This is more error-prone but faster to implement. + +**Recommendation: Implement Strategy A for hot-path values (relay_pubkey, admin_pubkey, auth settings), Strategy B for cold-path values.** + +### Phase 2: Fix Structural Leaks + +1. **Fix inactive subscription skip in CLOSED handler** — Remove the `if (sub->active)` guard, or add a second pass that also collects inactive subscriptions for cleanup +2. **Verify `remove_ip_connection()` is called on all disconnect paths** +3. **Add defensive cleanup** — periodic sweep of orphaned subscriptions + +### Phase 3: Add Memory Monitoring + +1. **Add a `/stats` endpoint** that reports: + - Current RSS/VSZ from `/proc/self/status` + - Active subscription count + - Message queue depth across all sessions + - Config cache hit/miss ratio +2. **Add periodic memory logging** to the service loop +3. **Consider integrating jemalloc** for better allocation tracking in production + +### Phase 4: Reduce Stack Pressure + +1. Move `candidates_to_check` to heap allocation with `malloc`/`free` +2. Consider reducing `MAX_TOTAL_SUBSCRIPTIONS` or using a dynamic array + +--- + +## Verification Strategy + +1. **Build with AddressSanitizer** (`-fsanitize=address`) for development testing +2. **Run under Valgrind** with `--leak-check=full` for comprehensive leak detection +3. **Monitor RSS over time** after fixes — should plateau rather than grow linearly +4. **Load test** with `tests/load_tests.sh` while monitoring memory + +--- + +## Immediate Mitigation + +While fixes are being implemented: +1. **Raise `MemoryHigh`** to 2 GB in the systemd unit to prevent throttling +2. **Add a cron job** to restart the service every 12-24 hours +3. **Monitor with**: `watch -n 5 'cat /proc/$(pgrep c_relay)/status | grep -E "VmRSS|VmSwap"'` + +--- + +## Implementation Priority Order + +| Priority | Task | Impact | +|----------|------|--------| +| P0 | Fix `is_nip42_auth_required_for_kind()` leak in config.c:377 | Called on every event | +| P0 | Fix `ip_is_whitelisted()` leak in ip_ban.c:255 | Called on every connection check | +| P0 | Fix `is_authorized_admin_event()` leaks in main.c:1718,1742 | Called per admin event, leaks in loop | +| P1 | Fix websockets.c:1527,1549 auth bypass leaks | Called on every event with auth | +| P1 | Fix request_validator.c:304,320 leaks | Called on every event validation | +| P1 | Fix nip042.c:99 relay_url leak | Called on every NIP-42 auth | +| P2 | Implement config cache for hot-path values | Eliminates leak class entirely | +| P2 | Fix inactive subscription cleanup in CLOSED handler | Prevents slow subscription leak | +| P3 | Add memory monitoring endpoint | Ongoing visibility | +| P3 | Move large stack arrays to heap | Reduces stack pressure | diff --git a/src/config.c b/src/config.c index 175b5e1..22be7c1 100644 --- a/src/config.c +++ b/src/config.c @@ -378,22 +378,25 @@ int is_nip42_auth_required_for_kind(int event_kind) { if (!kinds_str) { return 0; // No authentication required if setting is missing } - + // Parse the kinds list int required_kinds[100]; // Support up to 100 different kinds int count = parse_auth_required_kinds(kinds_str, required_kinds, 100); - + if (count < 0) { + free((char*)kinds_str); return 0; // Parse error, default to no auth required } - + // Check if event_kind is in the list for (int i = 0; i < count; i++) { if (required_kinds[i] == event_kind) { + free((char*)kinds_str); return 1; // Authentication required for this kind } } - + + free((char*)kinds_str); return 0; // Authentication not required for this kind } diff --git a/src/ip_ban.c b/src/ip_ban.c index 905cbdd..dc608a2 100644 --- a/src/ip_ban.c +++ b/src/ip_ban.c @@ -253,23 +253,32 @@ void ip_ban_save_to_db(sqlite3* db) { // Check if an IP is in the idle_ban_whitelist config (comma-separated list) static int ip_is_whitelisted(const char* ip) { const char* whitelist = get_config_value("idle_ban_whitelist"); - if (!whitelist || whitelist[0] == '\0') return 0; + if (!whitelist || whitelist[0] == '\0') { + if (whitelist) free((char*)whitelist); + return 0; + } // Make a mutable copy to tokenize char buf[1024]; strncpy(buf, whitelist, sizeof(buf) - 1); buf[sizeof(buf) - 1] = '\0'; + int is_match = 0; char* token = strtok(buf, ","); while (token) { // Trim leading/trailing spaces while (*token == ' ') token++; char* end = token + strlen(token) - 1; while (end > token && *end == ' ') { *end = '\0'; end--; } - if (strcmp(token, ip) == 0) return 1; + if (strcmp(token, ip) == 0) { + is_match = 1; + break; + } token = strtok(NULL, ","); } - return 0; + + free((char*)whitelist); + return is_match; } int ip_ban_is_banned(const char* ip) { diff --git a/src/main.c b/src/main.c index 256e8e9..e4b4010 100644 --- a/src/main.c +++ b/src/main.c @@ -1704,6 +1704,7 @@ int is_authorized_admin_event(cJSON* event, char* error_buffer, size_t error_buf } int targets_this_relay = 0; + const char* relay_pubkey = get_config_value("relay_pubkey"); cJSON *tag; cJSON_ArrayForEach(tag, tags) { if (cJSON_IsArray(tag)) { @@ -1715,7 +1716,6 @@ int is_authorized_admin_event(cJSON* event, char* error_buffer, size_t error_buf strcmp(tag_name->valuestring, "p") == 0) { // Compare with our relay pubkey - const char* relay_pubkey = get_config_value("relay_pubkey"); if (relay_pubkey && strcmp(tag_value->valuestring, relay_pubkey) == 0) { targets_this_relay = 1; break; @@ -1723,6 +1723,7 @@ int is_authorized_admin_event(cJSON* event, char* error_buffer, size_t error_buf } } } + if (relay_pubkey) free((char*)relay_pubkey); if (!targets_this_relay) { // Admin event for different relay - not an error, just not for us @@ -1743,6 +1744,7 @@ int is_authorized_admin_event(cJSON* event, char* error_buffer, size_t error_buf if (!admin_pubkey || strlen(admin_pubkey) == 0) { DEBUG_WARN("Unauthorized admin event attempt: no admin pubkey configured"); snprintf(error_buffer, error_buffer_size, "Unauthorized admin event attempt: no admin configured"); + if (admin_pubkey) free((char*)admin_pubkey); return -1; } @@ -1755,6 +1757,7 @@ int is_authorized_admin_event(cJSON* event, char* error_buffer, size_t error_buf DEBUG_WARN(warning_msg); DEBUG_INFO("DEBUG: Pubkey comparison failed - event pubkey != admin pubkey"); snprintf(error_buffer, error_buffer_size, "Unauthorized admin event attempt: invalid admin pubkey"); + free((char*)admin_pubkey); return -1; } @@ -1763,9 +1766,11 @@ int is_authorized_admin_event(cJSON* event, char* error_buffer, size_t error_buf if (nostr_verify_event_signature(event) != 0) { DEBUG_WARN("Unauthorized admin event attempt: invalid signature"); snprintf(error_buffer, error_buffer_size, "Unauthorized admin event attempt: signature verification failed"); + free((char*)admin_pubkey); return -1; } + free((char*)admin_pubkey); // All checks passed - authorized admin event return 0; diff --git a/src/main.h b/src/main.h index d6ab0b6..9996146 100644 --- a/src/main.h +++ b/src/main.h @@ -13,8 +13,8 @@ // Using CRELAY_ prefix to avoid conflicts with nostr_core_lib VERSION macros #define CRELAY_VERSION_MAJOR 1 #define CRELAY_VERSION_MINOR 2 -#define CRELAY_VERSION_PATCH 52 -#define CRELAY_VERSION "v1.2.52" +#define CRELAY_VERSION_PATCH 53 +#define CRELAY_VERSION "v1.2.53" // Relay metadata (authoritative source for NIP-11 information) #define RELAY_NAME "C-Relay" diff --git a/src/request_validator.c b/src/request_validator.c index 8862758..439525c 100644 --- a/src/request_validator.c +++ b/src/request_validator.c @@ -304,9 +304,11 @@ int nostr_validate_unified_request(const char* json_string, size_t json_length) const char* admin_pubkey = get_config_value("admin_pubkey"); if (admin_pubkey && strcmp(event_pubkey, admin_pubkey) == 0) { // Valid admin event - bypass remaining validation + free((char*)admin_pubkey); cJSON_Delete(event); return NOSTR_SUCCESS; } + if (admin_pubkey) free((char*)admin_pubkey); // Not from admin - continue with normal validation } diff --git a/src/websockets.c b/src/websockets.c index f01818c..31e59e1 100644 --- a/src/websockets.c +++ b/src/websockets.c @@ -1540,6 +1540,7 @@ static int nostr_relay_callback(struct lws *wsi, enum lws_callback_reasons reaso } } } + free((char*)relay_pubkey); } } } @@ -1552,6 +1553,7 @@ static int nostr_relay_callback(struct lws *wsi, enum lws_callback_reasons reaso } else { DEBUG_INFO("DEBUG: Kind 23456 event but pubkey mismatch or no admin pubkey"); } + if (admin_pubkey) free((char*)admin_pubkey); } if (pss && auth_required && !pss->authenticated && !bypass_auth) { @@ -2724,6 +2726,7 @@ int process_dm_stats_command(cJSON* dm_event, char* error_message, size_t error_ } } } + free((char*)relay_pubkey); if (!addressed_to_relay) { // Not addressed to relay, allow normal processing @@ -2742,9 +2745,11 @@ int process_dm_stats_command(cJSON* dm_event, char* error_message, size_t error_ const char* admin_pubkey = get_config_value("admin_pubkey"); if (!admin_pubkey || strlen(admin_pubkey) == 0 || strcmp(sender_pubkey, admin_pubkey) != 0) { + if (admin_pubkey) free((char*)admin_pubkey); strncpy(error_message, "Unauthorized: not admin", error_size - 1); return -1; } + free((char*)admin_pubkey); // Get relay private key for decryption char* relay_privkey_hex = get_relay_private_key();