From c1a8a5f753a5fcd939a6255370e6b35cb16d43cb Mon Sep 17 00:00:00 2001 From: Jacob Su Date: Wed, 27 Aug 2025 06:42:50 +0800 Subject: [PATCH] RTC: Fix null pointer crash in RTC2RTMP when start packet is missing. v6.0.175 (#4451) Try to fix #4450 ## Cause The SRS transcode rtp packets, whose sequence number in range [start, end], to one rtmp packet, but when the first rtp packet is empty, then this crash happens. check #4450 for details. ## Impact 5.0release and 6.0release branch. develop branch already has its own solution. So this PR is targeting to **6.0release**. ## Solution find the first not empty rtp packet in seq range [start, end]. --------- Co-authored-by: OSSRS-AI Co-authored-by: winlin --- trunk/auto/depends.sh | 5 ++ trunk/doc/CHANGELOG.md | 1 + trunk/src/app/srs_app_rtc_source.cpp | 19 ++++- trunk/src/core/srs_core_version6.hpp | 2 +- trunk/src/utest/srs_utest_rtc.cpp | 121 +++++++++++++++++++++++++++ 5 files changed, 144 insertions(+), 4 deletions(-) diff --git a/trunk/auto/depends.sh b/trunk/auto/depends.sh index 38d09bf83..e52e06c2b 100755 --- a/trunk/auto/depends.sh +++ b/trunk/auto/depends.sh @@ -671,6 +671,11 @@ fi if [[ $SRS_SRT == YES && $SRS_USE_SYS_SRT == NO ]]; then # Always disable c++11 for libsrt, because only the srt-app requres it. LIBSRT_OPTIONS="--enable-apps=0 --enable-static=1 --enable-c++11=0" + CMAKE_VERSION=$(cmake --version | head -n1 | cut -d' ' -f3) + CMAKE_MAJOR=$(echo $CMAKE_VERSION | cut -d'.' -f1) + if [[ $CMAKE_MAJOR -ge 4 ]]; then + LIBSRT_OPTIONS="$LIBSRT_OPTIONS --CMAKE_POLICY_VERSION_MINIMUM=3.5" + fi if [[ $SRS_SHARED_SRT == YES ]]; then LIBSRT_OPTIONS="$LIBSRT_OPTIONS --enable-shared=1" else diff --git a/trunk/doc/CHANGELOG.md b/trunk/doc/CHANGELOG.md index 5fa268b82..b4024b35f 100644 --- a/trunk/doc/CHANGELOG.md +++ b/trunk/doc/CHANGELOG.md @@ -7,6 +7,7 @@ The changelog for SRS. ## SRS 6.0 Changelog +* v6.0, 2025-08-26, Merge [#4451](https://github.com/ossrs/srs/pull/4451): RTC: Fix null pointer crash in RTC2RTMP when start packet is missing. v6.0.175 (#4451) * v6.0, 2025-08-16, Merge [#4441](https://github.com/ossrs/srs/pull/4441): fix err memory leak in rtc to rtmp bridge. v6.0.174 (#4441) * v6.0, 2025-08-14, Merge [#4161](https://github.com/ossrs/srs/pull/4161): fix hls & dash segments cleanup. v6.0.173 (#4161) * v6.0, 2025-08-12, Merge [#4230](https://github.com/ossrs/srs/pull/4230): MP4 DVR: Fix audio/video synchronization issues in WebRTC recordings. v6.0.172 (#4230) diff --git a/trunk/src/app/srs_app_rtc_source.cpp b/trunk/src/app/srs_app_rtc_source.cpp index 85a322130..6a2eb4880 100644 --- a/trunk/src/app/srs_app_rtc_source.cpp +++ b/trunk/src/app/srs_app_rtc_source.cpp @@ -1798,13 +1798,26 @@ srs_error_t SrsRtcFrameBuilder::packet_video_rtmp(const uint16_t start, const ui //type_codec1 + avc_type + composition time + nalu size + nalu nb_payload += 1 + 1 + 3; + // find the first not emtpy rtp packet, which is always exist in seq range [start, end], + // because nb_payload check above. + // @see https://github.com/ossrs/srs/issues/4450 + SrsRtpPacket* first_pkt = NULL; + for (uint16_t i = 0; i < (uint16_t)cnt; ++i) { + uint16_t index = cache_index(start + i); + SrsRtpPacket* pkt = cache_video_pkts_[index].pkt; + + if (pkt) { + first_pkt = pkt; + break; + } + } + SrsCommonMessage rtmp; - SrsRtpPacket* pkt = cache_video_pkts_[cache_index(start)].pkt; - rtmp.header.initialize_video(nb_payload, pkt->get_avsync_time(), 1); + rtmp.header.initialize_video(nb_payload, first_pkt->get_avsync_time(), 1); rtmp.create_payload(nb_payload); rtmp.size = nb_payload; SrsBuffer payload(rtmp.payload, rtmp.size); - if (pkt->is_keyframe()) { + if (first_pkt->is_keyframe()) { payload.write_1bytes(0x17); // type(4 bits): key frame; code(4bits): avc rtp_key_frame_ts_ = -1; } else { diff --git a/trunk/src/core/srs_core_version6.hpp b/trunk/src/core/srs_core_version6.hpp index ea7f3c7c6..37016a7b0 100644 --- a/trunk/src/core/srs_core_version6.hpp +++ b/trunk/src/core/srs_core_version6.hpp @@ -9,6 +9,6 @@ #define VERSION_MAJOR 6 #define VERSION_MINOR 0 -#define VERSION_REVISION 174 +#define VERSION_REVISION 175 #endif diff --git a/trunk/src/utest/srs_utest_rtc.cpp b/trunk/src/utest/srs_utest_rtc.cpp index b40679008..4b4056b0b 100644 --- a/trunk/src/utest/srs_utest_rtc.cpp +++ b/trunk/src/utest/srs_utest_rtc.cpp @@ -1365,3 +1365,124 @@ VOID TEST(KernelRTCTest, JitterSequence) EXPECT_EQ((uint32_t)11, jitter.correct(11)); } +// Helper function to create a test RTP packet +SrsRtpPacket *mock_create_test_rtp_packet(uint16_t sequence_number, uint32_t timestamp, bool marker = false) +{ + SrsRtpPacket *pkt = new SrsRtpPacket(); + pkt->header.set_sequence(sequence_number); + pkt->header.set_timestamp(timestamp); + pkt->header.set_marker(marker); + pkt->header.set_ssrc(12345); + return pkt; +} + +// Mock bridge for testing SrsRtcFrameBuilder +class MockStreamBridge : public ISrsStreamBridge +{ +public: + srs_error_t last_error; + int frame_count; + + MockStreamBridge() { + last_error = srs_success; + frame_count = 0; + } + + virtual ~MockStreamBridge() { + srs_freep(last_error); + } + + virtual srs_error_t initialize(SrsRequest *r) { + return srs_success; + } + + virtual srs_error_t on_publish() { + return srs_success; + } + + virtual srs_error_t on_frame(SrsSharedPtrMessage *frame) { + frame_count++; + return srs_success; + } + + virtual void on_unpublish() { + } +}; + +VOID TEST(KernelRTC2Test, SrsRtcFrameBuilderPacketVideoRtmpNullPointerCrash) +{ + srs_error_t err; + + // Test reproducing the null pointer crash from issue #4450 fixed by PR #4451 + // + // ISSUE BACKGROUND: + // Before PR 4451, the packet_video_rtmp() function assumed that the packet at the + // 'start' sequence number would always be available in the cache. However, due to + // network packet loss or reordering, the packet at the start position could be missing. + // + // THE CRASH: + // The original code did: pkt = cache_video_pkts_[cache_index(start)].pkt; + // When pkt was NULL, calling pkt->get_avsync_time() caused a segmentation fault. + // + // THE FIX: + // PR #4451 added a loop to find the first non-null packet in the sequence range + // instead of blindly using the packet at the start position. + // + // This test simulates the crash scenario: packets exist at positions 101, 102, 103 + // but the start packet (100) is missing. With the fix, the function should use + // packet 101 (first available) instead of crashing on the missing packet 100. + if (true) { + MockStreamBridge bridge; + SrsRtcFrameBuilder frame_builder(&bridge); + + // Skip initialization and directly set up the test scenario + // We only need to test the packet_video_rtmp function, not the full initialization + + // Manually populate the video cache to simulate the crash scenario + // We'll store packets at positions 101, 102, 103 but NOT at position 100 (start) + // This simulates network packet loss where the first packet is missing + + // Create test packets with payload to ensure nb_payload > 0 + SrsRtpPacket *pkt101 = mock_create_test_rtp_packet(101, 1000, false); + SrsRtpPacket *pkt102 = mock_create_test_rtp_packet(102, 1000, false); + SrsRtpPacket *pkt103 = mock_create_test_rtp_packet(103, 1000, true); // marker bit + + // Add some payload to ensure packets are not empty + char payload_data[] = "test_payload_data"; + SrsRtpRawPayload *payload101 = new SrsRtpRawPayload(); + payload101->payload = (char*)payload_data; + payload101->nn_payload = strlen(payload_data); + pkt101->set_payload(payload101, SrsRtspPacketPayloadTypeRaw); + + SrsRtpRawPayload *payload102 = new SrsRtpRawPayload(); + payload102->payload = (char*)payload_data; + payload102->nn_payload = strlen(payload_data); + pkt102->set_payload(payload102, SrsRtspPacketPayloadTypeRaw); + + SrsRtpRawPayload *payload103 = new SrsRtpRawPayload(); + payload103->payload = (char*)payload_data; + payload103->nn_payload = strlen(payload_data); + pkt103->set_payload(payload103, SrsRtspPacketPayloadTypeRaw); + + // Set the avsync time for the packets to avoid other null pointer issues + pkt101->set_avsync_time(1000); + pkt102->set_avsync_time(1000); + pkt103->set_avsync_time(1000); + + // Store packets in cache (but skip sequence 100 - this is the missing start packet) + frame_builder.cache_video_pkts_[frame_builder.cache_index(pkt101->header.get_sequence())].pkt = pkt101; + frame_builder.cache_video_pkts_[frame_builder.cache_index(pkt102->header.get_sequence())].pkt = pkt102; + frame_builder.cache_video_pkts_[frame_builder.cache_index(pkt103->header.get_sequence())].pkt = pkt103; + + // Before the fix in PR #4451, calling packet_video_rtmp(100, 103) would crash + // because it would try to access cache_video_pkts_[cache_index(100)].pkt which is NULL + // and then call pkt->get_avsync_time() causing a null pointer dereference + + // The fix ensures we find the first non-null packet (101) instead of assuming + // the start packet (100) exists + HELPER_EXPECT_SUCCESS(frame_builder.packet_video_rtmp(100, 103)); + + // Verify that a frame was successfully processed + EXPECT_EQ(1, bridge.frame_count); + } +}