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).
This commit is contained in:
Isaac Connor committed 2026-06-01 11:54:32 -04:00
1 parent f7a3263c52
commit eee9c0f4ac
2 files changed
+10 -14

No files matched your search

+9 -7
View File
@@ -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<std::shared_ptr<ZMPacket>> packets_to_destroy;
{
std::lock_guard<std::mutex> lck(mutex);
deleting = true;
// Why are we notifying?
@@ -433,8 +439,6 @@ void PacketQueue::clear() {
while (!pktQueue.empty()) {
std::shared_ptr<ZMPacket> 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<std::mutex> lck(mutex);
Debug(2, "Incrementing %p, queue size %zu, end? %d, deleting %d", it, pktQueue.size(), ((*it) == pktQueue.end()), deleting);