Fix a race condition in QUIC TLS ticket processing QUIC's TlsServerHandshaker class holds a non-owning pointer to a DecryptCallback object. When shutting down, the handshaker makes a call through this non-owning pointer, which can race with other code which may be simultaneously deleting the callback, resulting in a use-after-free crash. This CL solves the problem by switching to shared ownership of the callback. This change is difficult to regression-test - the problem was uncovered by some in-progress changes to the LetoClient class which are not ready for review yet. Protected by Trivial change, not protected. PiperOrigin-RevId: 447795102
diff --git a/quiche/quic/core/crypto/proof_source.h b/quiche/quic/core/crypto/proof_source.h index ffa5fec..f91b572 100644 --- a/quiche/quic/core/crypto/proof_source.h +++ b/quiche/quic/core/crypto/proof_source.h
@@ -219,7 +219,7 @@ // |in|. If decryption fails, the callback is invoked with an empty // vector. virtual void Decrypt(absl::string_view in, - std::unique_ptr<DecryptCallback> callback) = 0; + std::shared_ptr<DecryptCallback> callback) = 0; }; // Returns the TicketCrypter used for encrypting and decrypting TLS
diff --git a/quiche/quic/core/tls_server_handshaker.cc b/quiche/quic/core/tls_server_handshaker.cc index 3274fa5..c9781cc 100644 --- a/quiche/quic/core/tls_server_handshaker.cc +++ b/quiche/quic/core/tls_server_handshaker.cc
@@ -742,9 +742,8 @@ } if (!ticket_decryption_callback_) { - ticket_decryption_callback_ = new DecryptCallback(this); - proof_source_->GetTicketCrypter()->Decrypt( - in, std::unique_ptr<DecryptCallback>(ticket_decryption_callback_)); + ticket_decryption_callback_ = std::make_shared<DecryptCallback>(this); + proof_source_->GetTicketCrypter()->Decrypt(in, ticket_decryption_callback_); // Decrypt can run the callback synchronously. In that case, the callback // will clear the ticket_decryption_callback_ pointer, and instead of
diff --git a/quiche/quic/core/tls_server_handshaker.h b/quiche/quic/core/tls_server_handshaker.h index 68abf73..05b63ae 100644 --- a/quiche/quic/core/tls_server_handshaker.h +++ b/quiche/quic/core/tls_server_handshaker.h
@@ -325,7 +325,7 @@ // |ticket_decryption_callback_| points to the non-owned callback that was // passed to ProofSource::TicketCrypter::Decrypt but hasn't finished running // yet. - DecryptCallback* ticket_decryption_callback_ = nullptr; + std::shared_ptr<DecryptCallback> ticket_decryption_callback_; // |decrypted_session_ticket_| contains the decrypted session ticket after the // callback has run but before it is passed to BoringSSL. std::vector<uint8_t> decrypted_session_ticket_;
diff --git a/quiche/quic/test_tools/test_ticket_crypter.cc b/quiche/quic/test_tools/test_ticket_crypter.cc index b886dde..dbdb2a9 100644 --- a/quiche/quic/test_tools/test_ticket_crypter.cc +++ b/quiche/quic/test_tools/test_ticket_crypter.cc
@@ -55,7 +55,7 @@ void TestTicketCrypter::Decrypt( absl::string_view in, - std::unique_ptr<ProofSource::DecryptCallback> callback) { + std::shared_ptr<ProofSource::DecryptCallback> callback) { auto decrypted_ticket = Decrypt(in); if (run_async_) { pending_callbacks_.push_back({std::move(callback), decrypted_ticket});
diff --git a/quiche/quic/test_tools/test_ticket_crypter.h b/quiche/quic/test_tools/test_ticket_crypter.h index 8443099..888bdef 100644 --- a/quiche/quic/test_tools/test_ticket_crypter.h +++ b/quiche/quic/test_tools/test_ticket_crypter.h
@@ -22,7 +22,7 @@ std::vector<uint8_t> Encrypt(absl::string_view in, absl::string_view encryption_key) override; void Decrypt(absl::string_view in, - std::unique_ptr<ProofSource::DecryptCallback> callback) override; + std::shared_ptr<ProofSource::DecryptCallback> callback) override; void SetRunCallbacksAsync(bool run_async); size_t NumPendingCallbacks(); @@ -36,7 +36,7 @@ std::vector<uint8_t> Decrypt(absl::string_view in); struct PendingCallback { - std::unique_ptr<ProofSource::DecryptCallback> callback; + std::shared_ptr<ProofSource::DecryptCallback> callback; std::vector<uint8_t> decrypted_ticket; };
diff --git a/quiche/quic/tools/simple_ticket_crypter.cc b/quiche/quic/tools/simple_ticket_crypter.cc index aa1e65e..ad9fea1 100644 --- a/quiche/quic/tools/simple_ticket_crypter.cc +++ b/quiche/quic/tools/simple_ticket_crypter.cc
@@ -87,7 +87,7 @@ void SimpleTicketCrypter::Decrypt( absl::string_view in, - std::unique_ptr<quic::ProofSource::DecryptCallback> callback) { + std::shared_ptr<quic::ProofSource::DecryptCallback> callback) { callback->Run(Decrypt(in)); }
diff --git a/quiche/quic/tools/simple_ticket_crypter.h b/quiche/quic/tools/simple_ticket_crypter.h index f0395f0..b5ad16d 100644 --- a/quiche/quic/tools/simple_ticket_crypter.h +++ b/quiche/quic/tools/simple_ticket_crypter.h
@@ -28,7 +28,7 @@ absl::string_view encryption_key) override; void Decrypt( absl::string_view in, - std::unique_ptr<quic::ProofSource::DecryptCallback> callback) override; + std::shared_ptr<quic::ProofSource::DecryptCallback> callback) override; private: std::vector<uint8_t> Decrypt(absl::string_view in);