Ensure some QuicAlarms are properly cancelled to prevent use-after-free issues when alarm fires. Also added error messages in QuicAlarm to detect similar issues in production. Protected by FLAGS_quic_restart_flag_quic_alarm_add_permanent_cancel. PiperOrigin-RevId: 388316558
diff --git a/quic/core/quic_alarm.cc b/quic/core/quic_alarm.cc index bfef316..75f7dc5 100644 --- a/quic/core/quic_alarm.cc +++ b/quic/core/quic_alarm.cc
@@ -4,30 +4,70 @@ #include "quic/core/quic_alarm.h" +#include "quic/platform/api/quic_bug_tracker.h" +#include "quic/platform/api/quic_flags.h" +#include "quic/platform/api/quic_stack_trace.h" + namespace quic { QuicAlarm::QuicAlarm(QuicArenaScopedPtr<Delegate> delegate) : delegate_(std::move(delegate)), deadline_(QuicTime::Zero()) {} -QuicAlarm::~QuicAlarm() {} +QuicAlarm::~QuicAlarm() { + if (IsSet()) { + QUIC_LOG_EVERY_N_SEC(ERROR, 10 * 60) + << "QuicAlarm not cancelled at destruction. This message is rate " + "limited to once every 10 minutes. " + << QuicStackTrace(); + } +} void QuicAlarm::Set(QuicTime new_deadline) { QUICHE_DCHECK(!IsSet()); QUICHE_DCHECK(new_deadline.IsInitialized()); + + if (IsPermanentlyCancelled()) { + QUIC_BUG(quic_alarm_illegal_set) + << "Set called after alarm is permanently cancelled. new_deadline:" + << new_deadline; + return; + } + deadline_ = new_deadline; SetImpl(); } -void QuicAlarm::Cancel() { - if (!IsSet()) { - // Don't try to cancel an alarm that hasn't been set. +void QuicAlarm::CancelInternal(bool permanent) { + if (!GetQuicRestartFlag(quic_alarm_add_permanent_cancel)) { + if (!IsSet()) { + // Don't try to cancel an alarm that hasn't been set. + return; + } + deadline_ = QuicTime::Zero(); + CancelImpl(); return; } - deadline_ = QuicTime::Zero(); - CancelImpl(); + + if (IsSet()) { + deadline_ = QuicTime::Zero(); + CancelImpl(); + } + + if (permanent) { + delegate_.reset(); + } } +bool QuicAlarm::IsPermanentlyCancelled() const { return delegate_ == nullptr; } + void QuicAlarm::Update(QuicTime new_deadline, QuicTime::Delta granularity) { + if (IsPermanentlyCancelled()) { + QUIC_BUG(quic_alarm_illegal_update) + << "Update called after alarm is permanently cancelled. new_deadline:" + << new_deadline << ", granularity:" << granularity; + return; + } + if (!new_deadline.IsInitialized()) { Cancel(); return; @@ -55,7 +95,9 @@ } deadline_ = QuicTime::Zero(); - delegate_->OnAlarm(); + if (!IsPermanentlyCancelled()) { + delegate_->OnAlarm(); + } } void QuicAlarm::UpdateImpl() {
diff --git a/quic/core/quic_alarm.h b/quic/core/quic_alarm.h index 527d2e3..5080fd4 100644 --- a/quic/core/quic_alarm.h +++ b/quic/core/quic_alarm.h
@@ -36,11 +36,18 @@ // then Set(). void Set(QuicTime new_deadline); - // Cancels the alarm. May be called repeatedly. Does not - // guarantee that the underlying scheduling system will remove - // the alarm's associated task, but guarantees that the - // delegates OnAlarm method will not be called. - void Cancel(); + // Both PermanentCancel() and Cancel() can cancel the alarm. If permanent, + // future calls to Set() and Update() will become no-op except emitting an + // error log. + // + // Both may be called repeatedly. Does not guarantee that the underlying + // scheduling system will remove the alarm's associated task, but guarantees + // that the delegates OnAlarm method will not be called. + void PermanentCancel() { CancelInternal(true); } + void Cancel() { CancelInternal(false); } + + // Return true if PermanentCancel() has been called. + bool IsPermanentlyCancelled() const; // Cancels and sets the alarm if the |deadline| is farther from the current // deadline than |granularity|, and otherwise does nothing. If |deadline| is @@ -77,6 +84,8 @@ void Fire(); private: + void CancelInternal(bool permanent); + QuicArenaScopedPtr<Delegate> delegate_; QuicTime deadline_; };
diff --git a/quic/core/quic_alarm_test.cc b/quic/core/quic_alarm_test.cc index 8f7296c..ab07002 100644 --- a/quic/core/quic_alarm_test.cc +++ b/quic/core/quic_alarm_test.cc
@@ -4,6 +4,7 @@ #include "quic/core/quic_alarm.h" +#include "quic/platform/api/quic_expect_bug.h" #include "quic/platform/api/quic_test.h" using testing::Invoke; @@ -111,6 +112,48 @@ EXPECT_EQ(QuicTime::Zero(), alarm_.deadline()); } +TEST_F(QuicAlarmTest, PermanentCancel) { + QuicTime deadline = QuicTime::Zero() + QuicTime::Delta::FromSeconds(7); + alarm_.Set(deadline); + alarm_.PermanentCancel(); + EXPECT_FALSE(alarm_.IsSet()); + EXPECT_FALSE(alarm_.scheduled()); + EXPECT_EQ(QuicTime::Zero(), alarm_.deadline()); + + if (!GetQuicRestartFlag(quic_alarm_add_permanent_cancel)) { + alarm_.Set(deadline); + // When flag is false, PermanentCancel should work like a normal Cancel. + EXPECT_TRUE(alarm_.IsSet()); + EXPECT_TRUE(alarm_.scheduled()); + EXPECT_EQ(deadline, alarm_.deadline()); + EXPECT_FALSE(alarm_.IsPermanentlyCancelled()); + alarm_.PermanentCancel(); + } else { + EXPECT_QUIC_BUG(alarm_.Set(deadline), + "Set called after alarm is permanently cancelled"); + EXPECT_TRUE(alarm_.IsPermanentlyCancelled()); + } + EXPECT_FALSE(alarm_.IsSet()); + EXPECT_FALSE(alarm_.scheduled()); + EXPECT_EQ(QuicTime::Zero(), alarm_.deadline()); + + if (!GetQuicRestartFlag(quic_alarm_add_permanent_cancel)) { + alarm_.Update(deadline, QuicTime::Delta::Zero()); + EXPECT_TRUE(alarm_.IsSet()); + EXPECT_TRUE(alarm_.scheduled()); + EXPECT_EQ(deadline, alarm_.deadline()); + EXPECT_FALSE(alarm_.IsPermanentlyCancelled()); + alarm_.PermanentCancel(); + } else { + EXPECT_QUIC_BUG(alarm_.Update(deadline, QuicTime::Delta::Zero()), + "Update called after alarm is permanently cancelled"); + EXPECT_TRUE(alarm_.IsPermanentlyCancelled()); + } + EXPECT_FALSE(alarm_.IsSet()); + EXPECT_FALSE(alarm_.scheduled()); + EXPECT_EQ(QuicTime::Zero(), alarm_.deadline()); +} + TEST_F(QuicAlarmTest, Update) { QuicTime deadline = QuicTime::Zero() + QuicTime::Delta::FromSeconds(7); alarm_.Set(deadline);
diff --git a/quic/core/quic_buffered_packet_store.cc b/quic/core/quic_buffered_packet_store.cc index 078e222..9312ea5 100644 --- a/quic/core/quic_buffered_packet_store.cc +++ b/quic/core/quic_buffered_packet_store.cc
@@ -76,7 +76,12 @@ expiration_alarm_( alarm_factory->CreateAlarm(new ConnectionExpireAlarm(this))) {} -QuicBufferedPacketStore::~QuicBufferedPacketStore() {} +QuicBufferedPacketStore::~QuicBufferedPacketStore() { + if (GetQuicRestartFlag(quic_alarm_add_permanent_cancel) && + expiration_alarm_ != nullptr) { + expiration_alarm_->PermanentCancel(); + } +} EnqueuePacketResult QuicBufferedPacketStore::EnqueuePacket( QuicConnectionId connection_id,
diff --git a/quic/core/quic_connection.cc b/quic/core/quic_connection.cc index d3b3e82..bbdaf9b 100644 --- a/quic/core/quic_connection.cc +++ b/quic/core/quic_connection.cc
@@ -4251,7 +4251,7 @@ blackhole_detector_.IsDetectionInProgress()) { // Stop detection in quiescence. QUICHE_DCHECK_EQ(QuicSentPacketManager::LOSS_MODE, retransmission_mode); - blackhole_detector_.StopDetection(); + blackhole_detector_.StopDetection(/*permanent=*/false); } WriteIfNotBlocked(); @@ -4766,15 +4766,15 @@ void QuicConnection::CancelAllAlarms() { QUIC_DVLOG(1) << "Cancelling all QuicConnection alarms."; - ack_alarm_->Cancel(); - ping_alarm_->Cancel(); - retransmission_alarm_->Cancel(); - send_alarm_->Cancel(); - mtu_discovery_alarm_->Cancel(); - process_undecryptable_packets_alarm_->Cancel(); - discard_previous_one_rtt_keys_alarm_->Cancel(); - discard_zero_rtt_decryption_keys_alarm_->Cancel(); - blackhole_detector_.StopDetection(); + ack_alarm_->PermanentCancel(); + ping_alarm_->PermanentCancel(); + retransmission_alarm_->PermanentCancel(); + send_alarm_->PermanentCancel(); + mtu_discovery_alarm_->PermanentCancel(); + process_undecryptable_packets_alarm_->PermanentCancel(); + discard_previous_one_rtt_keys_alarm_->PermanentCancel(); + discard_zero_rtt_decryption_keys_alarm_->PermanentCancel(); + blackhole_detector_.StopDetection(/*permanent=*/true); idle_network_detector_.StopDetection(); } @@ -4808,6 +4808,9 @@ } void QuicConnection::SetPingAlarm() { + if (!connected_) { + return; + } if (perspective_ == Perspective::IS_SERVER && initial_retransmittable_on_wire_timeout_.IsInfinite()) { // The PING alarm exists to support two features: @@ -5806,7 +5809,7 @@ // In case no new packets get acknowledged, it is possible packets are // detected lost because of time based loss detection. Cancel blackhole // detection if there is no packets in flight. - blackhole_detector_.StopDetection(); + blackhole_detector_.StopDetection(/*permanent=*/false); } if (send_stop_waiting) { @@ -6186,6 +6189,9 @@ } void QuicConnection::OnForwardProgressMade() { + if (GetQuicRestartFlag(quic_alarm_add_permanent_cancel) && !connected_) { + return; + } if (is_path_degrading_) { visitor_->OnForwardProgressMadeAfterPathDegrading(); is_path_degrading_ = false; @@ -6197,7 +6203,7 @@ GetPathMtuReductionDeadline()); } else { // Stop detections in quiecense. - blackhole_detector_.StopDetection(); + blackhole_detector_.StopDetection(/*permanent=*/false); } QUIC_BUG_IF(quic_bug_12714_35, default_enable_5rto_blackhole_detection_ && @@ -7159,7 +7165,7 @@ // re-arm it. SetRetransmissionAlarm(); // Stop detections in quiecense. - blackhole_detector_.StopDetection(); + blackhole_detector_.StopDetection(/*permanent=*/false); return old_send_algorithm; }
diff --git a/quic/core/quic_dispatcher.cc b/quic/core/quic_dispatcher.cc index 6518754..11f4bcd 100644 --- a/quic/core/quic_dispatcher.cc +++ b/quic/core/quic_dispatcher.cc
@@ -340,9 +340,14 @@ << "Trying to create dispatcher without any supported versions"; QUIC_DLOG(INFO) << "Created QuicDispatcher with versions: " << ParsedQuicVersionVectorToString(GetSupportedVersions()); + QUIC_RESTART_FLAG_COUNT(quic_alarm_add_permanent_cancel); } QuicDispatcher::~QuicDispatcher() { + if (GetQuicRestartFlag(quic_alarm_add_permanent_cancel) && + delete_sessions_alarm_ != nullptr) { + delete_sessions_alarm_->PermanentCancel(); + } reference_counted_session_map_.clear(); closed_ref_counted_session_list_.clear(); if (support_multiple_cid_per_connection_) {
diff --git a/quic/core/quic_flags_list.h b/quic/core/quic_flags_list.h index 84f50ee..2089704 100644 --- a/quic/core/quic_flags_list.h +++ b/quic/core/quic_flags_list.h
@@ -33,6 +33,8 @@ QUIC_FLAG(FLAGS_quic_reloadable_flag_quic_can_send_ack_frequency, true) // If true, add missing MaybeUpdateAckTimeout for ack-eliciting frames. QUIC_FLAG(FLAGS_quic_reloadable_flag_quic_add_missing_update_ack_timeout, true) +// If true, allow QuicAlarm to be permanently cancelled. +QUIC_FLAG(FLAGS_quic_restart_flag_quic_alarm_add_permanent_cancel, true) // If true, allow client to enable BBRv2 on server via connection option \'B2ON\'. QUIC_FLAG(FLAGS_quic_reloadable_flag_quic_allow_client_enabled_bbr_v2, false) // If true, allow ticket open to be ignored in TlsServerHandshaker. Also fixes TlsServerHandshaker::ResumptionAttempted when handshake hints is used.
diff --git a/quic/core/quic_idle_network_detector.cc b/quic/core/quic_idle_network_detector.cc index ba81601..ac6799e 100644 --- a/quic/core/quic_idle_network_detector.cc +++ b/quic/core/quic_idle_network_detector.cc
@@ -70,7 +70,7 @@ } void QuicIdleNetworkDetector::StopDetection() { - alarm_->Cancel(); + alarm_->PermanentCancel(); handshake_timeout_ = QuicTime::Delta::Infinite(); idle_network_timeout_ = QuicTime::Delta::Infinite(); stopped_ = true;
diff --git a/quic/core/quic_network_blackhole_detector.cc b/quic/core/quic_network_blackhole_detector.cc index 5281ffb..4dca508 100644 --- a/quic/core/quic_network_blackhole_detector.cc +++ b/quic/core/quic_network_blackhole_detector.cc
@@ -64,8 +64,12 @@ UpdateAlarm(); } -void QuicNetworkBlackholeDetector::StopDetection() { - alarm_->Cancel(); +void QuicNetworkBlackholeDetector::StopDetection(bool permanent) { + if (permanent) { + alarm_->PermanentCancel(); + } else { + alarm_->Cancel(); + } path_degrading_deadline_ = QuicTime::Zero(); blackhole_deadline_ = QuicTime::Zero(); path_mtu_reduction_deadline_ = QuicTime::Zero(); @@ -108,6 +112,12 @@ } void QuicNetworkBlackholeDetector::UpdateAlarm() const { + // If called after OnBlackholeDetected(), the alarm may have been permanently + // cancelled and is not safe to be armed again. + if (alarm_->IsPermanentlyCancelled()) { + return; + } + QuicTime next_deadline = GetEarliestDeadline(); QUIC_DVLOG(1) << "Updating alarm. next_deadline:" << next_deadline
diff --git a/quic/core/quic_network_blackhole_detector.h b/quic/core/quic_network_blackhole_detector.h index 7010eb5..82753f9 100644 --- a/quic/core/quic_network_blackhole_detector.h +++ b/quic/core/quic_network_blackhole_detector.h
@@ -44,8 +44,9 @@ QuicConnectionArena* arena, QuicAlarmFactory* alarm_factory); - // Called to stop all detections. - void StopDetection(); + // Called to stop all detections. If |permanent|, the alarm will be cancelled + // permanently and future calls to RestartDetection will be no-op. + void StopDetection(bool permanent); // Called to restart path degrading, path mtu reduction and blackhole // detections. Please note, if |blackhole_deadline| is set, it must be the
diff --git a/quic/core/quic_network_blackhole_detector_test.cc b/quic/core/quic_network_blackhole_detector_test.cc index 3bc6ad7..66bebfd 100644 --- a/quic/core/quic_network_blackhole_detector_test.cc +++ b/quic/core/quic_network_blackhole_detector_test.cc
@@ -106,7 +106,7 @@ RestartDetection(); EXPECT_EQ(clock_.Now() + path_degrading_delay_, alarm_->deadline()); - detector_.StopDetection(); + detector_.StopDetection(/*permanent=*/false); EXPECT_FALSE(detector_.IsDetectionInProgress()); }
diff --git a/quic/core/quic_session.cc b/quic/core/quic_session.cc index df32cc5..aef3fc2 100644 --- a/quic/core/quic_session.cc +++ b/quic/core/quic_session.cc
@@ -166,7 +166,12 @@ GetMutableCryptoStream()->id()); } -QuicSession::~QuicSession() {} +QuicSession::~QuicSession() { + if (GetQuicRestartFlag(quic_alarm_add_permanent_cancel) && + closed_streams_clean_up_alarm_ != nullptr) { + closed_streams_clean_up_alarm_->PermanentCancel(); + } +} void QuicSession::PendingStreamOnStreamFrame(const QuicStreamFrame& frame) { QUICHE_DCHECK(VersionUsesHttp3(transport_version()));
diff --git a/quic/test_tools/packet_dropping_test_writer.cc b/quic/test_tools/packet_dropping_test_writer.cc index 35cc978..76dd778 100644 --- a/quic/test_tools/packet_dropping_test_writer.cc +++ b/quic/test_tools/packet_dropping_test_writer.cc
@@ -68,7 +68,14 @@ simple_random_.set_seed(seed); } -PacketDroppingTestWriter::~PacketDroppingTestWriter() = default; +PacketDroppingTestWriter::~PacketDroppingTestWriter() { + if (write_unblocked_alarm_ != nullptr) { + write_unblocked_alarm_->PermanentCancel(); + } + if (delay_alarm_ != nullptr) { + delay_alarm_->PermanentCancel(); + } +} void PacketDroppingTestWriter::Initialize( QuicConnectionHelperInterface* helper,
diff --git a/quic/test_tools/simulator/queue.cc b/quic/test_tools/simulator/queue.cc index c286bf6..c7e23cd 100644 --- a/quic/test_tools/simulator/queue.cc +++ b/quic/test_tools/simulator/queue.cc
@@ -26,7 +26,7 @@ new AggregationAlarmDelegate(this))); } -Queue::~Queue() {} +Queue::~Queue() { aggregation_timeout_alarm_->PermanentCancel(); } void Queue::set_tx_port(ConstrainedPortInterface* port) { tx_port_ = port;