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;
