[trafficserver] branch quic-latest updated: Erase "ack_only" lost packet correctly

[ma...@apache.org]

This is an automated email from the ASF dual-hosted git repository.

masaori pushed a commit to branch quic-latest
in repository https://gitbox.apache.org/repos/asf/trafficserver.git


The following commit(s) were added to refs/heads/quic-latest by this push:
     new fad804a  Erase "ack_only" lost packet correctly
fad804a is described below

commit fad804a84d1a45f7ac4f24baea0d5838e15476db
Author: Masaori Koshiba <ma...@apache.org>
AuthorDate: Wed Aug 8 16:48:51 2018 +0900

    Erase "ack_only" lost packet correctly
    
    To avoid heap-use-after-free caused by erasing elements in the loop
---
 iocore/net/quic/QUICLossDetector.cc | 48 ++++++++++++++++++++++++-------------
 iocore/net/quic/QUICLossDetector.h  |  3 +++
 2 files changed, 35 insertions(+), 16 deletions(-)

diff --git a/iocore/net/quic/QUICLossDetector.cc b/iocore/net/quic/QUICLossDetector.cc
index c6a5014..2773d80 100644
--- a/iocore/net/quic/QUICLossDetector.cc
+++ b/iocore/net/quic/QUICLossDetector.cc
@@ -427,36 +427,39 @@ QUICLossDetector::_detect_lost_packets(QUICPacketNumber largest_acked_packet_num
     delay_until_lost = 5.0 / 4.0 * std::max(this->_latest_rtt, this->_smoothed_rtt);
   }
 
-  for (auto &unacked : this->_sent_packets) {
-    if (unacked.first >= largest_acked_packet_number) {
+  for (auto it = this->_sent_packets.begin(); it != this->_sent_packets.end();) {
+    if (it->first >= largest_acked_packet_number) {
       break;
     }
-    ink_hrtime time_since_sent = Thread::get_hrtime() - unacked.second->time;
-    uint64_t delta             = largest_acked_packet_number - unacked.second->packet_number;
+    ink_hrtime time_since_sent = Thread::get_hrtime() - it->second->time;
+    uint64_t delta             = largest_acked_packet_number - it->second->packet_number;
     if (time_since_sent > delay_until_lost || delta > this->_reordering_threshold) {
       if (time_since_sent > delay_until_lost) {
         QUICLDDebug("Lost: time since sent is too long (PN=%" PRId64 " sent=%" PRId64 ", delay=%lf, fraction=%lf, lrtt=%" PRId64
                     ", srtt=%" PRId64 ")",
-                    unacked.first, time_since_sent, delay_until_lost, this->_time_reordering_fraction, this->_latest_rtt,
+                    it->first, time_since_sent, delay_until_lost, this->_time_reordering_fraction, this->_latest_rtt,
                     this->_smoothed_rtt);
       } else {
         QUICLDDebug("Lost: packet delta is too large (PN=%" PRId64 " largest=%" PRId64 " unacked=%" PRId64 " threshold=%" PRId32
                     ")",
-                    unacked.first, largest_acked_packet_number, unacked.second->packet_number, this->_reordering_threshold);
+                    it->first, largest_acked_packet_number, it->second->packet_number, this->_reordering_threshold);
       }
 
-      if (!unacked.second->ack_only) {
-        lost_packets.insert({unacked.first, unacked.second.get()});
+      if (!it->second->ack_only) {
+        lost_packets.insert({it->first, it->second.get()});
       } else {
         // -- ADDITIONAL CODE --
         // We remove only "ack-only" packets for life time management of packets.
         // Packets in lost_packets will be removed from sent_packet later.
-        this->_remove_from_sent_packet_list(unacked.first);
+        it = this->_remove_from_sent_packet_list(it);
+        continue;
         // -- END OF ADDITIONAL CODE --
       }
     } else if (this->_loss_time == 0 && delay_until_lost != INFINITY) {
       this->_loss_time = Thread::get_hrtime() + delay_until_lost - time_since_sent;
     }
+
+    ++it;
   }
 
   // Inform the congestion controller of lost packets and
@@ -582,21 +585,34 @@ QUICLossDetector::_remove_from_sent_packet_list(QUICPacketNumber packet_number)
 {
   SCOPED_MUTEX_LOCK(lock, this->_loss_detection_mutex, this_ethread());
 
-  // Decrement counters
   auto ite = this->_sent_packets.find(packet_number);
-  if (ite != this->_sent_packets.end()) {
-    if (ite->second->handshake) {
+  this->_decrement_outstanding_counters(ite);
+  this->_sent_packets.erase(packet_number);
+}
+
+std::map<QUICPacketNumber, PacketInfoUPtr>::iterator
+QUICLossDetector::_remove_from_sent_packet_list(std::map<QUICPacketNumber, PacketInfoUPtr>::iterator it)
+{
+  SCOPED_MUTEX_LOCK(lock, this->_loss_detection_mutex, this_ethread());
+
+  this->_decrement_outstanding_counters(it);
+  return this->_sent_packets.erase(it);
+}
+
+void
+QUICLossDetector::_decrement_outstanding_counters(std::map<QUICPacketNumber, PacketInfoUPtr>::iterator it)
+{
+  if (it != this->_sent_packets.end()) {
+    // Decrement counters
+    if (it->second->handshake) {
       ink_assert(this->_handshake_outstanding.load() > 0);
       --this->_handshake_outstanding;
     }
-    if (!ite->second->ack_only) {
+    if (!it->second->ack_only) {
       ink_assert(this->_retransmittable_outstanding.load() > 0);
       --this->_retransmittable_outstanding;
     }
   }
-
-  // Remove from the list
-  this->_sent_packets.erase(packet_number);
 }
 
 ink_hrtime
diff --git a/iocore/net/quic/QUICLossDetector.h b/iocore/net/quic/QUICLossDetector.h
index 48249a1..4bb26ec 100644
--- a/iocore/net/quic/QUICLossDetector.h
+++ b/iocore/net/quic/QUICLossDetector.h
@@ -163,6 +163,9 @@ private:
   std::atomic<uint32_t> _retransmittable_outstanding;
   void _add_to_sent_packet_list(QUICPacketNumber packet_number, std::unique_ptr<PacketInfo> packet_info);
   void _remove_from_sent_packet_list(QUICPacketNumber packet_number);
+  std::map<QUICPacketNumber, PacketInfoUPtr>::iterator _remove_from_sent_packet_list(
+    std::map<QUICPacketNumber, PacketInfoUPtr>::iterator it);
+  void _decrement_outstanding_counters(std::map<QUICPacketNumber, PacketInfoUPtr>::iterator it);
 
   /*
    * Because this alarm will be reset on every packet transmission, to reduce number of events,