diff --git a/quiche/quic/core/quic_unacked_packet_map.cc b/quiche/quic/core/quic_unacked_packet_map.cc index 80401dd01..af65ee103 100644 --- a/quiche/quic/core/quic_unacked_packet_map.cc +++ b/quiche/quic/core/quic_unacked_packet_map.cc @@ -503,22 +503,21 @@ bool QuicUnackedPacketMap::NotifyFramesAcked(QuicPacketNumber packet_number, } bool new_data_acked = false; const bool is_retransmission = info->transmission_type != NOT_RETRANSMISSION; - const QuicFrames* frames = &info->retransmittable_frames; quiche::SimpleBufferAllocator allocator; - std::optional frames_copy; - const bool use_copied_frames = - !HasDatagramFrame(info->retransmittable_frames); - - if (use_copied_frames) { - frames = &frames_copy.emplace( - CopyQuicFrames(&allocator, info->retransmittable_frames)); - } + // SECURITY: OnFrameAcked can synchronously call AddSentPacket → push_back + // → Relocate, which frees both the deque slab (invalidating |info|) and + // each element's InlinedVector heap buffer (invalidating any iterator into + // info->retransmittable_frames). The DATAGRAM-frame copy bypass that was + // here is unsafe because the same reentrancy is reachable when a DATAGRAM + // frame coexists with a MAX_STREAMS frame in the same packet. Always copy. + QuicFrames frames_copy = + CopyQuicFrames(&allocator, info->retransmittable_frames); + const QuicFrames* frames = &frames_copy; absl::Cleanup cleanup = [&]() { - if (use_copied_frames) { - DeleteFrames(&frames_copy.value()); - info = GetMutableTransmissionInfo(packet_number); - } + DeleteFrames(&frames_copy); + // |info| may have been invalidated by deque relocation; re-fetch. + info = GetMutableTransmissionInfo(packet_number); }; for (const QuicFrame& frame : *frames) { @@ -548,22 +547,18 @@ void QuicUnackedPacketMap::MaybeAggregateAckedStreamFrame( if (session_notifier_ == nullptr) { return; } - const QuicFrames* frames = &info->retransmittable_frames; quiche::SimpleBufferAllocator allocator; - std::optional frames_copy; - const bool use_copied_frames = - !HasDatagramFrame(info->retransmittable_frames); - - if (use_copied_frames) { - frames = &frames_copy.emplace( - CopyQuicFrames(&allocator, info->retransmittable_frames)); - } + // SECURITY: same reentrancy hazard as NotifyFramesAcked. Always copy; + // always re-fetch |info|. The DATAGRAM-frame bypass that was here left + // both the iterator and the caller's |info| pointing into freed memory + // when OnFrameAcked → AddSentPacket → Relocate fires. + QuicFrames frames_copy = + CopyQuicFrames(&allocator, info->retransmittable_frames); + const QuicFrames* frames = &frames_copy; absl::Cleanup cleanup = [&]() { - if (use_copied_frames) { - DeleteFrames(&frames_copy.value()); - info = GetMutableTransmissionInfo(packet_number); - } + DeleteFrames(&frames_copy); + info = GetMutableTransmissionInfo(packet_number); }; for (const auto& frame : *frames) {