Skip to content

Commit

Permalink
Ensure QuicChromiumPacketWriter isn't still tracked when deleted.
Browse files Browse the repository at this point in the history
Attempt to address increased crashes in the QuicChromiumPacketWriter
destructor.

The QuicChromiumPacketWriter destructor crashes seem to indicate that
there is a double free.  The destructor is called from both the
QuicConnection destructor and during QuicConnection::MigratePath.

A double free appears to only be possible after MigratePath is called
with the same writer as currently used, but on a connection that is
not connected.

The increase possibly means a race between a connection teardown and
migration was made more likely to occur as a result of recent
performance improvements.

This adds a check and pointer reset that should avoid having a stale
QuicChromiumPacketWriter pointer in the QuicConnection.

b/388316262
  • Loading branch information
jellefoks committed Jan 14, 2025
1 parent 729b59a commit 76c39ef
Showing 1 changed file with 15 additions and 5 deletions.
20 changes: 15 additions & 5 deletions net/third_party/quiche/src/quiche/quic/core/quic_connection.cc
Original file line number Diff line number Diff line change
Expand Up @@ -411,7 +411,7 @@ void QuicConnection::InstallInitialCrypters(QuicConnectionId connection_id) {

QuicConnection::~QuicConnection() {
QUICHE_DCHECK_GE(stats_.max_egress_mtu, long_term_mtu_);
if (owns_writer_) {
if (writer_ != nullptr && owns_writer_) {
delete writer_;
}
ClearQueuedPackets();
Expand Down Expand Up @@ -2439,7 +2439,7 @@ QuicPacketNumber QuicConnection::GetLeastUnacked() const {
}

bool QuicConnection::HandleWriteBlocked() {
if (!writer_->IsWriteBlocked()) {
if (writer_ != nullptr && !writer_->IsWriteBlocked()) {
return false;
}

Expand Down Expand Up @@ -2808,12 +2808,14 @@ void QuicConnection::ProcessUdpPacket(const QuicSocketAddress& self_address,
}

void QuicConnection::OnBlockedWriterCanWrite() {
writer_->SetWritable();
OnCanWrite();
if (writer_) {
writer_->SetWritable();
OnCanWrite();
}
}

void QuicConnection::OnCanWrite() {
if (!connected_) {
if (!connected_ || !writer_) {
return;
}
if (writer_->IsWriteBlocked()) {
Expand Down Expand Up @@ -6911,6 +6913,10 @@ bool QuicConnection::MigratePath(const QuicSocketAddress& self_address,
QUICHE_DCHECK(perspective_ == Perspective::IS_CLIENT);
if (!connected_) {
if (owns_writer) {
if (writer_ == writer) {
writer_ = nullptr;
owns_writer_ = false;
}
delete writer;
}
return false;
Expand All @@ -6921,6 +6927,10 @@ bool QuicConnection::MigratePath(const QuicSocketAddress& self_address,
if (connection_migration_use_new_cid_) {
if (!UpdateConnectionIdsOnMigration(self_address, peer_address)) {
if (owns_writer) {
if (writer_ == writer) {
writer_ = nullptr;
owns_writer_ = false;
}
delete writer;
}
return false;
Expand Down

0 comments on commit 76c39ef

Please sign in to comment.