Address Copilot review feedback on PR #353

- Validate clock_gettime() return and handle unexpected pthread_cond_timedwait()
  errors in pop_event() to prevent tight loops
- Restore sanitize_stream_name() in generate_recording_path() to prevent
  path traversal via crafted stream names
- Remove racy read of ctx->enabled under contexts_mutex (update_recording_state
  already checks it under the per-context mutex)
- Remove commented-out OLD CODE blocks

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Author: matteius

src/video/onvif_motion_recording.c

@@ -18,6 +18,7 @@
 #include "video/stream_manager.h"
 #include "core/logger.h"
 #include "core/config.h"
+#include "core/path_utils.h"
 #include "core/shutdown_coordinator.h"
 #include "database/database_manager.h"
 #include "database/db_recordings.h"
@@ -125,27 +126,6 @@ static int push_event(const motion_event_t *event) {
 //   0  = event successfully dequeued
 //  -1  = shutting down (no event)
 //  -2  = timeout (no event within 1 s), caller should tick the state machine
-//
-// FIX: recording stop bug - the old implementation used pthread_cond_wait
-// (blocking forever), so update_recording_state() was never called again
-// after a "no motion" event, and recordings never stopped.
-// The new implementation uses a 1-second timedwait so the event loop can
-// periodically tick the state machine even when no new events arrive.
-//
-// OLD CODE (kept for reference):
-// static int pop_event(motion_event_t *event) {
-//     if (!event) {
-//         return -1;
-//     }
-//     pthread_mutex_lock(&event_queue.mutex);
-//     // Wait for events if queue is empty
-//     while (event_queue.count == 0 && event_processor_running) {
-//         pthread_cond_wait(&event_queue.cond, &event_queue.mutex);  // <-- bug: blocks forever
-//     }
-//     if (event_queue.count == 0) {
-//         pthread_mutex_unlock(&event_queue.mutex);
-//         return -1;
-//     }
 static int pop_event(motion_event_t *event) {
     if (!event) {
         return -1;
@@ -158,13 +138,23 @@ static int pop_event(motion_event_t *event) {
     // RECORDING -> FINALIZING -> stop after a "no motion" event).
     while (event_queue.count == 0 && event_processor_running) {
         struct timespec ts;
-        clock_gettime(CLOCK_REALTIME, &ts);
+        if (clock_gettime(CLOCK_REALTIME, &ts) != 0) {
+            // clock_gettime failed — fall back to a short sleep
+            pthread_mutex_unlock(&event_queue.mutex);
+            return -2;
+        }
         ts.tv_sec += 1;
         int rc = pthread_cond_timedwait(&event_queue.cond, &event_queue.mutex, &ts);
         if (rc == ETIMEDOUT) {
             pthread_mutex_unlock(&event_queue.mutex);
             return -2; // periodic tick
         }
+        if (rc != 0 && rc != ETIMEDOUT) {
+            // Unexpected error (e.g. EINVAL) — treat as a tick to avoid tight loop
+            log_warn("pthread_cond_timedwait unexpected error: %d", rc);
+            pthread_mutex_unlock(&event_queue.mutex);
+            return -2;
+        }
     }
 
     if (event_queue.count == 0) {
@@ -270,31 +260,35 @@ static int generate_recording_path(const char *stream_name, char *path, size_t p
     char timestamp[32];
     strftime(timestamp, sizeof(timestamp), "%Y%m%d_%H%M%S", tm_info);
 
+    // Sanitize the stream name to prevent path traversal and handle spaces.
+    char sanitized_name[MAX_STREAM_NAME];
+    sanitize_stream_name(stream_name, sanitized_name, MAX_STREAM_NAME);
+
     // Create directory structure: /recordings/camera_name/YYYY/MM/DD/
     char year[8], month[4], day[4];
     strftime(year, sizeof(year), "%Y", tm_info);
     strftime(month, sizeof(month), "%m", tm_info);
     strftime(day, sizeof(day), "%d", tm_info);
-    
+
     char dir_path[MAX_PATH_LENGTH];
     snprintf(dir_path, sizeof(dir_path), "%s/%s/%s/%s/%s",
-             config->storage_path, stream_name, year, month, day);
-    
+             config->storage_path, sanitized_name, year, month, day);
+
     // Create directories if they don't exist
     char temp_path[MAX_PATH_LENGTH];
-    snprintf(temp_path, sizeof(temp_path), "%s/%s", config->storage_path, stream_name);
+    snprintf(temp_path, sizeof(temp_path), "%s/%s", config->storage_path, sanitized_name);
     mkdir(temp_path, 0755);
-    
-    snprintf(temp_path, sizeof(temp_path), "%s/%s/%s", config->storage_path, stream_name, year);
+
+    snprintf(temp_path, sizeof(temp_path), "%s/%s/%s", config->storage_path, sanitized_name, year);
     mkdir(temp_path, 0755);
-    
-    snprintf(temp_path, sizeof(temp_path), "%s/%s/%s/%s", config->storage_path, stream_name, year, month);
+
+    snprintf(temp_path, sizeof(temp_path), "%s/%s/%s/%s", config->storage_path, sanitized_name, year, month);
     mkdir(temp_path, 0755);
-    
+
     mkdir(dir_path, 0755);
-    
+
     // Generate full file path
-    snprintf(path, path_size, "%s/%s_%s_motion.mp4", dir_path, stream_name, timestamp);
+    snprintf(path, path_size, "%s/%s_%s_motion.mp4", dir_path, sanitized_name, timestamp);
 
     log_info("Generated recording path: %s", path);
     return 0;
@@ -497,14 +491,6 @@ static void* event_processor_thread_func(void *arg) {
         // as "shutting down" and continued/broke without ever ticking the
         // state machine for already-active recordings.
         //
-        // OLD CODE (kept for reference):
-        // if (pop_event(&event) != 0) {
-        //     // Queue is empty or we're shutting down
-        //     if (!event_processor_running) {
-        //         break;
-        //     }
-        //     continue;  // <-- bug: skips update_recording_state entirely
-        // }
         int pop_ret = pop_event(&event);
         if (pop_ret == -1) {
             // Shutting down
@@ -518,8 +504,9 @@ static void* event_processor_thread_func(void *arg) {
             time_t now = time(NULL);
             pthread_mutex_lock(&contexts_mutex);
             for (int i = 0; i < g_config.max_streams; i++) {
-                if (recording_contexts[i].active && recording_contexts[i].enabled) {
+                if (recording_contexts[i].active) {
                     pthread_mutex_unlock(&contexts_mutex);
+                    // update_recording_state() checks ctx->enabled under ctx->mutex
                     update_recording_state(&recording_contexts[i], now);
                     pthread_mutex_lock(&contexts_mutex);
                 }