From 3f4251cacf3ba1fa869da95c1de4057e0e622cd6 Mon Sep 17 00:00:00 2001 From: Sean Ferguson Date: Mon, 23 Mar 2026 04:01:00 -0400 Subject: [PATCH] MT#55283 Fix recording-file parameter ignores for pcap recording method Closes #2087 Change-Id: Ib25aeabecfcad78578f3d98162a4418c243a8dbe (cherry picked from commit b916bdc257dd50b9ea42dafb2b95504ba9c80a4d) (cherry picked from commit c61c40f81c2b5dcad3517b0fd536a4898aeeea6c) --- daemon/main.c | 4 +- daemon/recording.c | 72 +++++++--- pkg/deb/.gitignore | 1 + t/Makefile | 9 +- t/auto-daemon-tests-recording.pl | 231 +++++++++++++++++++++++++++++++ 5 files changed, 296 insertions(+), 21 deletions(-) create mode 100644 pkg/deb/.gitignore create mode 100644 t/auto-daemon-tests-recording.pl diff --git a/daemon/main.c b/daemon/main.c index c1f6793d6..4d182681a 100644 --- a/daemon/main.c +++ b/daemon/main.c @@ -1599,12 +1599,12 @@ int main(int argc, char **argv) { ilog(LOG_INFO, "Version %s shutting down", RTPENGINE_VERSION); - recording_fs_free(); - unfill_initial_rtpe_cfg(&initial_rtpe_config); call_free(); + recording_fs_free(); + jitter_buffer_init_free(); media_player_free(); call_interfaces_free(); diff --git a/daemon/recording.c b/daemon/recording.c index b3d56b18d..062d21009 100644 --- a/daemon/recording.c +++ b/daemon/recording.c @@ -36,6 +36,8 @@ struct rec_pcap_format { static int check_main_spool_dir(const char *spoolpath); static char *recording_setup_file(struct recording *recording, const str *); +static char *recording_setup_file_explicit(struct recording *recording, const char *path); +static char *recording_open_pcap_file(struct recording *recording, char *path); static char *meta_setup_file(struct recording *recording, const str *); static int append_meta_chunk(struct recording *recording, const char *buf, unsigned int buflen, const char *label_fmt, ...) @@ -486,7 +488,14 @@ static void rec_pcap_init(call_t *call) { meta_setup_file(recording, &call->recording_meta_prefix); // set up pcap file - char *pcap_path = recording_setup_file(recording, &call->recording_meta_prefix); + // If an explicit recording-file path was specified, use it directly; + // otherwise auto-generate a path from the meta prefix. + char *pcap_path; + if (call->recording_file.len) + pcap_path = recording_setup_file_explicit(recording, call->recording_file.s); + else + pcap_path = recording_setup_file(recording, &call->recording_meta_prefix); + if (pcap_path != NULL && recording->pcap.recording_pdumper != NULL && recording->pcap.meta_fp) { // Write the location of the PCAP file to the metadata file @@ -593,6 +602,12 @@ static void rec_pcap_meta_finish_file(call_t *call) { // Get the filename (in between its directory and the file extension) // and move it to the finished file location. // Rename extension to ".txt". + if (!spooldir) { + ilog(LOG_WARN, "Cannot move metadata file: spool directory not set"); + mutex_destroy(&recording->pcap.recording_lock); + g_clear_pointer(&recording->pcap.meta_filepath, g_free); + return; + } int fn_len; char *meta_filename = strrchr(recording->pcap.meta_filepath, '/'); char *meta_ext = NULL; @@ -640,32 +655,50 @@ static void rec_pcap_meta_discard_file(call_t *call) { g_clear_pointer(&recording->pcap.meta_filepath, free); } +/** + * Common helper: takes ownership of a g_malloc-allocated path string, opens + * the PCAP file there, and stores everything in the recording struct. + * Returns the path on success or failure (caller checks recording_pdumper). + */ +static char *recording_open_pcap_file(struct recording *recording, char *path) { + recording->pcap.recording_path = path; + + recording->pcap.recording_pd = pcap_open_dead(rec_pcap_format->linktype, 65535); + recording->pcap.recording_pdumper = pcap_dump_open(recording->pcap.recording_pd, path); + if (recording->pcap.recording_pdumper == NULL) { + pcap_close(recording->pcap.recording_pd); + recording->pcap.recording_pd = NULL; + ilog(LOG_INFO, "Failed to write recording file: %s", path); + } else { + ilog(LOG_INFO, "Writing recording file: %s", path); + } + + return path; +} + /** * Generate a random PCAP filepath to write recorded RTP stream. * Returns path to created file. */ static char *recording_setup_file(struct recording *recording, const str *meta_prefix) { - char *recording_path = NULL; - if (!spooldir) return NULL; if (recording->pcap.recording_pd || recording->pcap.recording_pdumper) return NULL; - recording_path = file_path_str(meta_prefix->s, "/pcaps/", ".pcap"); - recording->pcap.recording_path = recording_path; + return recording_open_pcap_file(recording, + file_path_str(meta_prefix->s, "/pcaps/", ".pcap")); +} - recording->pcap.recording_pd = pcap_open_dead(rec_pcap_format->linktype, 65535); - recording->pcap.recording_pdumper = pcap_dump_open(recording->pcap.recording_pd, recording_path); - if (recording->pcap.recording_pdumper == NULL) { - pcap_close(recording->pcap.recording_pd); - recording->pcap.recording_pd = NULL; - ilog(LOG_INFO, "Failed to write recording file: %s", recording_path); - } else { - ilog(LOG_INFO, "Writing recording file: %s", recording_path); - } +/** + * Open a PCAP file at an explicit absolute path supplied by the caller. + * No directory prefix or file extension is added. + */ +static char *recording_setup_file_explicit(struct recording *recording, const char *path) { + if (recording->pcap.recording_pd || recording->pcap.recording_pdumper) + return NULL; - return recording_path; + return recording_open_pcap_file(recording, g_strdup(path)); } /** @@ -675,11 +708,11 @@ static void rec_pcap_recording_finish_file(struct recording *recording) { if (recording->pcap.recording_pdumper != NULL) { pcap_dump_flush(recording->pcap.recording_pdumper); pcap_dump_close(recording->pcap.recording_pdumper); - g_clear_pointer(&recording->pcap.recording_path, free); } if (recording->pcap.recording_pd != NULL) { pcap_close(recording->pcap.recording_pd); } + g_clear_pointer(&recording->pcap.recording_path, free); } // "out" must be at least inp->len + MAX_PACKET_HEADER_LEN bytes @@ -784,9 +817,14 @@ void recording_finish(call_t *call, bool discard) { g_slice_free1(sizeof(*(recording)), recording); call->recording = NULL; - // clear the meta prefix to esure that pcaps for subsequent + // clear the meta prefix to ensure that pcaps for subsequent // start recordings dont overwrite previous ones call->recording_meta_prefix = STR_NULL; + // also clear per-recording path overrides so they are not + // inadvertently reused if the next start recording omits them + call->recording_file = STR_NULL; + call->recording_path = STR_NULL; + call->recording_pattern = STR_NULL; } diff --git a/pkg/deb/.gitignore b/pkg/deb/.gitignore new file mode 100644 index 000000000..fcf54f43d --- /dev/null +++ b/pkg/deb/.gitignore @@ -0,0 +1 @@ +debian/ diff --git a/t/Makefile b/t/Makefile index 695088cd5..c1dedef4e 100644 --- a/t/Makefile +++ b/t/Makefile @@ -105,7 +105,8 @@ include ../lib/common.Makefile daemon-tests-main daemon-tests-jb daemon-tests-dtx daemon-tests-dtx-cn daemon-tests-pubsub \ daemon-tests-intfs daemon-tests-stats daemon-tests-delay-buffer daemon-tests-delay-timing \ daemon-tests-evs daemon-tests-player-cache daemon-tests-redis daemon-tests-t38 daemon-tests-evs-dtx \ - daemon-tests-measure-rtp daemon-tests-heuristic daemon-tests-rtcp + daemon-tests-measure-rtp daemon-tests-heuristic daemon-tests-rtcp \ + daemon-tests-recording TESTS= test-bitstr aes-crypt aead-aes-crypt test-const_str_hash.strhash ifeq ($(with_transcoding),yes) @@ -144,7 +145,8 @@ daemon-tests: daemon-tests-main daemon-tests-jb daemon-tests-pubsub daemon-tests daemon-tests-audio-player daemon-tests-audio-player-play-media \ daemon-tests-record-egress-play-media \ daemon-tests-intfs daemon-tests-stats daemon-tests-player-cache daemon-tests-redis \ - daemon-tests-rtpp-flags daemon-tests-measure-rtp daemon-tests-heuristic daemon-tests-rtcp + daemon-tests-rtpp-flags daemon-tests-measure-rtp daemon-tests-heuristic daemon-tests-rtcp \ + daemon-tests-recording daemon-test-deps: tests-preload.so $(MAKE) -C ../daemon @@ -216,6 +218,9 @@ daemon-tests-t38: daemon-test-deps spandsp_recv_fax_pcm spandsp_recv_fax_t38 \ daemon-tests-rtcp: daemon-test-deps ./auto-test-helper "$@" perl -I../perl auto-daemon-tests-rtcp.pl +daemon-tests-recording: daemon-test-deps + ./auto-test-helper "$@" perl -I../perl auto-daemon-tests-recording.pl + test-bitstr: test-bitstr.o test-mix-buffer: test-mix-buffer.o $(COMMONOBJS) mix_buffer.o ssrc.o rtp.o crypto.o helpers.o \ diff --git a/t/auto-daemon-tests-recording.pl b/t/auto-daemon-tests-recording.pl new file mode 100644 index 000000000..e68cd0065 --- /dev/null +++ b/t/auto-daemon-tests-recording.pl @@ -0,0 +1,231 @@ +#!/usr/bin/perl + +use strict; +use warnings; +use NGCP::Rtpengine::Test; +use NGCP::Rtpengine::AutoTest; +use Test::More; +use POSIX; + + +my $spooldir = "/tmp/rtpengine-recording-test-$$"; +mkdir $spooldir or die "Cannot create spooldir '$spooldir': $!"; + +END { + system("rm", "-rf", $spooldir); +} + +autotest_start(qw(--config-file=none -t -1 -i 203.0.113.1 -i 2001:db8:4321::1 + -n 2223 -c 12345 -f -L 7 -E -u 2222 + --recording-method=pcap), + "--recording-dir=$spooldir") + or die; + +my ($sock_a, $sock_b, $port_a, $port_b, $resp); + + +# Test: recording-file parameter is honoured - pcap is written to the explicit path +{ + my $explicit_pcap = "$spooldir/explicit-test.pcap"; + + ($sock_a, $sock_b) = new_call([qw(198.51.100.1 2010)], [qw(198.51.100.3 2012)]); + + ($port_a) = offer('explicit recording-file', { ICE => 'remove' }, < 'remove' }, < $explicit_pcap }); + + snd($sock_a, $port_b, rtp(0, 1000, 3000, 0x1234, "\x00" x 160)); + rcv($sock_b, $port_a, rtpm(0, 1000, 3000, 0x1234, "\x00" x 160)); + + rtpe_req('stop recording', 'stop recording', {}); + + ok(-e $explicit_pcap, 'pcap file exists at explicit recording-file path'); + ok(!grep({ -f $_ } glob("$spooldir/pcaps/*.pcap")), + 'no pcap written to auto-generated spooldir/pcaps/ path'); + + rtpe_req('delete', 'delete call'); +} + + +# Test: auto-generated path is used when recording-file is not specified +{ + ($sock_a, $sock_b) = new_call([qw(198.51.100.1 2010)], [qw(198.51.100.3 2012)]); + + ($port_a) = offer('auto-generated recording path', { ICE => 'remove' }, < 'remove' }, < 0, 'pcap file created in auto-generated spooldir/pcaps/'); + + rtpe_req('delete', 'delete call'); +} + + +# Test: recording_file is cleared after stop recording so a subsequent start +# recording (without recording-file) uses an auto-generated path, not the +# previously specified one. +{ + my $first_explicit_pcap = "$spooldir/stale-path-test.pcap"; + + ($sock_a, $sock_b) = new_call([qw(198.51.100.1 2010)], [qw(198.51.100.3 2012)]); + + ($port_a) = offer('stale recording-file cleared after stop', { ICE => 'remove' }, < 'remove' }, < $first_explicit_pcap }); + + snd($sock_a, $port_b, rtp(0, 1000, 3000, 0x1234, "\x00" x 160)); + rcv($sock_b, $port_a, rtpm(0, 1000, 3000, 0x1234, "\x00" x 160)); + + rtpe_req('stop recording', 'stop first recording', {}); + my $size_after_first = -s $first_explicit_pcap; + ok(defined($size_after_first) && $size_after_first > 0, + 'first pcap file written to explicit path'); + + # Second recording on the same call: no recording-file specified. + # recording_file must have been cleared by recording_finish() so the + # second recording goes to the auto-generated path, not $first_explicit_pcap. + rtpe_req('start recording', 'start second recording without explicit path', {}); + + snd($sock_a, $port_b, rtp(0, 1001, 3160, 0x1234, "\x00" x 160)); + rcv($sock_b, $port_a, rtpm(0, 1001, 3160, 0x1234, "\x00" x 160)); + + rtpe_req('stop recording', 'stop second recording', {}); + + # The first explicit pcap must not have been extended by the second recording + my $size_after_second = -s $first_explicit_pcap; + is($size_after_second, $size_after_first, + 'first pcap not extended by second recording (stale path was cleared)'); + + # A new auto-generated pcap must exist in spooldir/pcaps/ + my @auto_pcaps = grep({ -f $_ } glob("$spooldir/pcaps/*.pcap")); + ok(scalar(@auto_pcaps) > 0, + 'second recording created a new auto-generated pcap in spooldir/pcaps/'); + + rtpe_req('delete', 'delete call'); +} + + +done_testing();