Skip to content

Commit 9fcff95

Browse files
matteiusclaude
matteius
and
claude
committed
Address Copilot review feedback on PR #354
Restore sanitize_stream_name() in all four start functions to prevent path traversal via crafted stream names. Restore cleanup_dead_recording() call in start_mp4_recording_with_trigger() to avoid leaking dead contexts. Remove large commented "OLD CODE" block and restore path_utils.h include. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1 parent fb8959e commit 9fcff95

1 file changed

Lines changed: 36 additions & 48 deletions

File tree

‎src/video/mp4_recording_core.c‎

Lines changed: 36 additions & 48 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,7 @@
2828
#include "core/logger.h"
2929
#include "core/config.h"
3030
#include "core/url_utils.h"
31+
#include "core/path_utils.h"
3132
#include "core/shutdown_coordinator.h"
3233
#include "video/stream_manager.h"
3334
#include "video/streams.h"
@@ -651,16 +652,20 @@ int start_mp4_recording(const char *stream_name) {
651652
const struct tm *tm_info = localtime_r(&now, &tm_buf);
652653
strftime(timestamp_str, sizeof(timestamp_str), "%Y%m%d_%H%M%S", tm_info);
653654

655+
// Sanitize the stream name so that names with spaces work correctly.
656+
char encoded_name[MAX_STREAM_NAME];
657+
sanitize_stream_name(stream_name, encoded_name, MAX_STREAM_NAME);
658+
654659
// Create MP4 directory path
655660
char mp4_dir[MAX_PATH_LENGTH];
656661
if (global_config->record_mp4_directly && global_config->mp4_storage_path[0] != '\0') {
657662
// Use configured MP4 storage path if available
658663
snprintf(mp4_dir, MAX_PATH_LENGTH, "%s/%s",
659-
global_config->mp4_storage_path, stream_name);
664+
global_config->mp4_storage_path, encoded_name);
660665
} else {
661666
// Use mp4 directory parallel to hls, NOT inside it
662667
snprintf(mp4_dir, MAX_PATH_LENGTH, "%s/mp4/%s",
663-
global_config->storage_path, stream_name);
668+
global_config->storage_path, encoded_name);
664669
}
665670

666671
// Create MP4 directory if it doesn't exist
@@ -818,16 +823,20 @@ int start_mp4_recording_with_url(const char *stream_name, const char *url) {
818823
const struct tm *tm_info = localtime_r(&now, &tm_buf);
819824
strftime(timestamp_str, sizeof(timestamp_str), "%Y%m%d_%H%M%S", tm_info);
820825

826+
// Sanitize the stream name so that names with spaces work correctly.
827+
char encoded_name[MAX_STREAM_NAME];
828+
sanitize_stream_name(stream_name, encoded_name, MAX_STREAM_NAME);
829+
821830
// Create MP4 directory path
822831
char mp4_dir[MAX_PATH_LENGTH];
823832
if (global_config->record_mp4_directly && global_config->mp4_storage_path[0] != '\0') {
824833
// Use configured MP4 storage path if available
825834
snprintf(mp4_dir, MAX_PATH_LENGTH, "%s/%s",
826-
global_config->mp4_storage_path, stream_name);
835+
global_config->mp4_storage_path, encoded_name);
827836
} else {
828837
// Use mp4 directory parallel to hls, NOT inside it
829838
snprintf(mp4_dir, MAX_PATH_LENGTH, "%s/mp4/%s",
830-
global_config->storage_path, stream_name);
839+
global_config->storage_path, encoded_name);
831840
}
832841

833842
// Create MP4 directory if it doesn't exist
@@ -972,61 +981,25 @@ int start_mp4_recording_with_trigger(const char *stream_name, const char *trigge
972981
}
973982

974983
// Check if already running — also verify the recording is actually healthy.
975-
//
976-
// OLD CODE (kept for reference):
977-
//
978-
// mp4_recording_ctx_t *dead_ctx = NULL;
979-
// pthread_mutex_lock(&recording_contexts_mutex);
980-
// for (int i = 0; i < g_config.max_streams; i++) {
981-
// if (recording_contexts[i] && strcmp(recording_contexts[i]->config.name, stream_name) == 0) {
982-
// mp4_writer_t *writer = recording_contexts[i]->mp4_writer;
983-
// if (writer && mp4_writer_is_recording(writer)) {
984-
// pthread_mutex_unlock(&recording_contexts_mutex);
985-
// return 0; // Already running and healthy
986-
// }
987-
// // <-- bug: writer==NULL was treated as dead and the slot was cleared,
988-
// // allowing a second recording instance to start for the same stream.
989-
// // writer is set asynchronously inside mp4_recording_thread (after
990-
// // RTSP connect + avformat_find_stream_info, which can take several
991-
// // seconds). During that window every call here saw writer==NULL,
992-
// // concluded the recording was dead, and spawned a new thread —
993-
// // producing multiple parallel recordings with overlapping timestamps.
994-
// dead_ctx = recording_contexts[i];
995-
// dead_ctx->running = 0;
996-
// recording_contexts[i] = NULL;
997-
// break;
998-
// }
999-
// }
1000-
// pthread_mutex_unlock(&recording_contexts_mutex);
1001-
//
1002-
// FIX: distinguish three states for an existing slot:
1003-
// 1. writer != NULL && mp4_writer_is_recording() → healthy, return early
1004-
// 2. writer == NULL && ctx->running == 1 → initializing, return early
1005-
// 3. everything else → dead, clean up and restart
1006984
mp4_recording_ctx_t *dead_ctx = NULL;
1007985
pthread_mutex_lock(&recording_contexts_mutex);
1008986
for (int i = 0; i < g_config.max_streams; i++) {
1009987
if (recording_contexts[i] && strcmp(recording_contexts[i]->config.name, stream_name) == 0) {
1010988
mp4_writer_t *writer = recording_contexts[i]->mp4_writer;
1011-
1012989
if (writer && mp4_writer_is_recording(writer)) {
1013-
// State 1: fully up and healthy
1014990
pthread_mutex_unlock(&recording_contexts_mutex);
1015991
log_info("MP4 recording for stream %s already running and healthy", stream_name);
1016-
return 0;
992+
return 0; // Already running and healthy
1017993
}
1018-
1019994
if (!writer && recording_contexts[i]->running) {
1020-
// State 2: thread started but mp4_writer not yet assigned —
995+
// Still initializing — mp4_writer not yet assigned by the thread.
1021996
// RTSP connect / avformat_find_stream_info still in progress.
1022-
// This is the race window; do NOT spawn a second instance.
1023997
pthread_mutex_unlock(&recording_contexts_mutex);
1024-
log_info("MP4 recording for stream %s is initializing (writer not yet ready), skipping duplicate start",
1025-
stream_name);
998+
log_info("MP4 recording for stream %s is initializing, skipping duplicate start", stream_name);
1026999
return 0;
10271000
}
10281001

1029-
// State 3: dead — extract under the lock, join outside
1002+
// Dead — extract from slot under the lock, join outside
10301003
log_warn("MP4 recording for stream %s exists but is dead, cleaning up before restart", stream_name);
10311004
dead_ctx = recording_contexts[i];
10321005
dead_ctx->running = 0;
@@ -1036,6 +1009,13 @@ int start_mp4_recording_with_trigger(const char *stream_name, const char *trigge
10361009
}
10371010
pthread_mutex_unlock(&recording_contexts_mutex);
10381011

1012+
// Join the dead thread outside the mutex (can block up to 15 s)
1013+
if (dead_ctx) {
1014+
cleanup_dead_recording(dead_ctx, stream_name);
1015+
}
1016+
1017+
log_info("Using standalone recording thread for stream %s with trigger_type: %s", stream_name, trigger_type);
1018+
10391019
// Find empty slot (under lock)
10401020
pthread_mutex_lock(&recording_contexts_mutex);
10411021
int slot = -1;
@@ -1083,14 +1063,18 @@ int start_mp4_recording_with_trigger(const char *stream_name, const char *trigge
10831063
const struct tm *tm_info = localtime_r(&now, &tm_buf);
10841064
strftime(timestamp_str, sizeof(timestamp_str), "%Y%m%d_%H%M%S", tm_info);
10851065

1066+
// Sanitize the stream name so that names with spaces work correctly.
1067+
char encoded_name[MAX_STREAM_NAME];
1068+
sanitize_stream_name(stream_name, encoded_name, MAX_STREAM_NAME);
1069+
10861070
// Create MP4 directory path
10871071
char mp4_dir[MAX_PATH_LENGTH];
10881072
if (global_config->record_mp4_directly && global_config->mp4_storage_path[0] != '\0') {
10891073
snprintf(mp4_dir, MAX_PATH_LENGTH, "%s/%s",
1090-
global_config->mp4_storage_path, stream_name);
1074+
global_config->mp4_storage_path, encoded_name);
10911075
} else {
10921076
snprintf(mp4_dir, MAX_PATH_LENGTH, "%s/mp4/%s",
1093-
global_config->storage_path, stream_name);
1077+
global_config->storage_path, encoded_name);
10941078
}
10951079

10961080
// Create MP4 directory if it doesn't exist
@@ -1236,14 +1220,18 @@ int start_mp4_recording_with_url_and_trigger(const char *stream_name, const char
12361220
const struct tm *tm_info = localtime_r(&now, &tm_buf);
12371221
strftime(timestamp_str, sizeof(timestamp_str), "%Y%m%d_%H%M%S", tm_info);
12381222

1223+
// Sanitize the stream name so that names with spaces work correctly.
1224+
char encoded_name[MAX_STREAM_NAME];
1225+
sanitize_stream_name(stream_name, encoded_name, MAX_STREAM_NAME);
1226+
12391227
// Create MP4 directory path
12401228
char mp4_dir[MAX_PATH_LENGTH];
12411229
if (global_config->record_mp4_directly && global_config->mp4_storage_path[0] != '\0') {
12421230
snprintf(mp4_dir, MAX_PATH_LENGTH, "%s/%s",
1243-
global_config->mp4_storage_path, stream_name);
1231+
global_config->mp4_storage_path, encoded_name);
12441232
} else {
12451233
snprintf(mp4_dir, MAX_PATH_LENGTH, "%s/mp4/%s",
1246-
global_config->storage_path, stream_name);
1234+
global_config->storage_path, encoded_name);
12471235
}
12481236

12491237
// Create MP4 directory if it doesn't exist

0 commit comments

Comments
 (0)