v1.2.53 - Fix P0 config-value memory leaks in hot paths and add investigation plan

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 |

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
 }
 

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) {

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;

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"

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
   }
 

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();