From eee9c0f4acbe0cc5e8fdc329ddf8bfb64423aa39 Mon Sep 17 00:00:00 2001 From: Isaac Connor Date: Mon, 1 Jun 2026 11:54:32 -0400 Subject: [PATCH] fix(packetqueue): destroy packets after releasing mutex; drop dead code clear() was destroying ZMPackets while holding the queue mutex, unlike queuePacket() and clearPackets() which defer destruction to a stack vector that goes out of scope after the lock is released. ZMPacket teardown frees Image, AVFrame, and AVPacket data and is expensive, especially on PASSTHROUGH queues that may hold hundreds of MB. Doing it under the mutex risked stalling capture and analysis threads during shutdown or monitor reconfigure. Adopt the same deferred-destroy pattern. Also remove dead code identified in the audit: - PacketQueue::unlock(ZMPacketLock*): defined but no callers; would UAF if anyone passed a non-heap pointer. - monitor_ field and setMonitor(): set never read, no callers. - analysis_it field: declared, never used (Monitor::analysis_it is the live one). - get_stream_it(int) declaration: never defined. - max_video_packet_count comment: clarify 0 means unlimited (the setter clamps negatives to 0 and the limit check is > 0). --- src/zm_packetqueue.cpp | 16 +++++++++------- src/zm_packetqueue.h | 8 +------- 2 files changed, 10 insertions(+), 14 deletions(-) diff --git a/src/zm_packetqueue.cpp b/src/zm_packetqueue.cpp index 290dda7b3..145243e35 100644 --- a/src/zm_packetqueue.cpp +++ b/src/zm_packetqueue.cpp @@ -424,6 +424,12 @@ void PacketQueue::stop() { void PacketQueue::clear() { Debug(1, "Clearing packetqueue"); + // Move packets out under the lock, then let their destructors run after + // the mutex is released. Mirrors queuePacket()/clearPackets() so we don't + // stall other threads on expensive ZMPacket teardown (Image, AVPacket). + std::vector> packets_to_destroy; + + { std::lock_guard lck(mutex); deleting = true; // Why are we notifying? @@ -433,8 +439,6 @@ void PacketQueue::clear() { while (!pktQueue.empty()) { std::shared_ptr packet = pktQueue.front(); - // Someone might have this packet, but not for very long and since we have locked the queue they won't be able to get another one - // Deleting this packet, doesn't require a lock. We only need a lock if we are modifying the packet. Debug(1, "Deleting a packet with stream index:%d image_index:%d with keyframe:%d, video frames in queue:%d max: %d, queuesize:%zu", packet->packet->stream_index, @@ -445,6 +449,7 @@ void PacketQueue::clear() { pktQueue.size()); packet_counts[packet->packet->stream_index] -= 1; pktQueue.pop_front(); + packets_to_destroy.push_back(std::move(packet)); } Debug(1, "Packetqueue is clear, deleting iterators"); @@ -463,6 +468,8 @@ void PacketQueue::clear() { Debug(1, "Packetqueue is clear, notifying"); condition.notify_all(); + } // end scope for lock_guard — mutex released here + // packets_to_destroy goes out of scope here, destroying packets without holding the mutex } // end void PacketQueue::clear() unsigned int PacketQueue::size() { @@ -585,11 +592,6 @@ ZMPacketLock PacketQueue::get_packet_and_increment_it(packetqueue_iterator *it) return ZMPacketLock(); } // end ZMPacketLock *PacketQueue::get_packet_and_increment_it(it) -void PacketQueue::unlock(ZMPacketLock *lp) { - delete lp; - condition.notify_all(); -} - bool PacketQueue::increment_it(packetqueue_iterator *it, bool wait) { std::unique_lock lck(mutex); Debug(2, "Incrementing %p, queue size %zu, end? %d, deleting %d", it, pktQueue.size(), ((*it) == pktQueue.end()), deleting); diff --git a/src/zm_packetqueue.h b/src/zm_packetqueue.h index bd485b480..cbd945f66 100644 --- a/src/zm_packetqueue.h +++ b/src/zm_packetqueue.h @@ -28,7 +28,6 @@ #include #include -class Monitor; class ZMPacket; class ZMPacketLock; @@ -37,10 +36,9 @@ typedef std::list>::iterator packetqueue_iterator; class PacketQueue { private: // For now just to ease development std::list> pktQueue; - std::list>::iterator analysis_it; int video_stream_id; - int max_video_packet_count; // allow a negative value to someday mean unlimited + int max_video_packet_count; // 0 means unlimited // This is now a hard limit on the # of video packets to keep in the queue so that we can limit ram int pre_event_video_packet_count; // Was max_video_packet_count int max_stream_id; @@ -57,7 +55,6 @@ class PacketQueue { int frames_since_last_keyframe_; std::atomic clear_packets_pending_; uint64_t next_queue_index_; - Monitor *monitor_; public: PacketQueue(); @@ -69,7 +66,6 @@ class PacketQueue { void setMaxVideoPackets(int p); void setPreEventVideoPackets(int p); void setKeepKeyframes(bool k) { keep_keyframes = k; }; - void setMonitor(Monitor *m) { monitor_ = m; }; bool queuePacket(std::shared_ptr packet); void stop(); @@ -103,7 +99,6 @@ class PacketQueue { ZMPacketLock get_packet_no_wait(packetqueue_iterator *); ZMPacketLock get_packet_and_increment_it(packetqueue_iterator *); packetqueue_iterator *get_video_it(bool wait); - packetqueue_iterator *get_stream_it(int stream_id); void free_it(packetqueue_iterator *); packetqueue_iterator *get_event_start_packet_it( @@ -111,7 +106,6 @@ class PacketQueue { unsigned int pre_event_count ); bool is_there_an_iterator_pointing_to_packet(const std::shared_ptr zm_packet); - void unlock(ZMPacketLock *lp); void notify_all(); void wait(); void wait_for(Microseconds duration);