Fix QUIC insecure randomness on Windows 7
These tests were disabled recently due to a rare crash on Windows 7. We tracked the crash down to the fact that taking the address of a static thread_local uint64_t[4] is apparently buggy on Windows 7. This change instead initializes that variable by assignment to sidestep the issue.
We've confirmed that this change fixes the Windows 7 Chrome crasher:
https://chromium-swarm.appspot.com/task?id=54e82650dc386a10
Note that this CL is not flag-protected because that would require two separate implementations of Xoshiro which would double the amount of thread_local storage required. That could cause issues on some platforms.
PiperOrigin-RevId: 386354470
diff --git a/quic/core/crypto/quic_random.cc b/quic/core/crypto/quic_random.cc
index 56657b5..d3190c6 100644
--- a/quic/core/crypto/quic_random.cc
+++ b/quic/core/crypto/quic_random.cc
@@ -19,17 +19,22 @@
// xoshiro256++ 1.0 based on code in the public domain from
// <http://prng.di.unimi.it/xoshiro256plusplus.c>.
+inline uint64_t Xoshiro256InitializeRngStateMember() {
+ uint64_t result;
+ RAND_bytes(reinterpret_cast<uint8_t*>(&result), sizeof(result));
+ return result;
+}
+
inline uint64_t Xoshiro256PlusPlusRotLeft(uint64_t x, int k) {
return (x << k) | (x >> (64 - k));
}
uint64_t Xoshiro256PlusPlus() {
- static thread_local uint64_t rng_state[4];
- static thread_local bool rng_state_initialized = false;
- if (QUIC_PREDICT_FALSE(!rng_state_initialized)) {
- RAND_bytes(reinterpret_cast<uint8_t*>(&rng_state), sizeof(rng_state));
- rng_state_initialized = true;
- }
+ static thread_local uint64_t rng_state[4] = {
+ Xoshiro256InitializeRngStateMember(),
+ Xoshiro256InitializeRngStateMember(),
+ Xoshiro256InitializeRngStateMember(),
+ Xoshiro256InitializeRngStateMember()};
const uint64_t result =
Xoshiro256PlusPlusRotLeft(rng_state[0] + rng_state[3], 23) + rng_state[0];
const uint64_t t = rng_state[1] << 17;
diff --git a/quic/core/crypto/quic_random_test.cc b/quic/core/crypto/quic_random_test.cc
index 67963ce..ab27918 100644
--- a/quic/core/crypto/quic_random_test.cc
+++ b/quic/core/crypto/quic_random_test.cc
@@ -30,8 +30,7 @@
EXPECT_NE(value1, value2);
}
-// TODO(b/194177024): re-enable this test.
-TEST_F(QuicRandomTest, QUIC_TEST_DISABLED_IN_CHROME(InsecureRandBytes)) {
+TEST_F(QuicRandomTest, InsecureRandBytes) {
unsigned char buf1[16];
unsigned char buf2[16];
memset(buf1, 0xaf, sizeof(buf1));
@@ -43,8 +42,7 @@
EXPECT_NE(0, memcmp(buf1, buf2, sizeof(buf1)));
}
-// TODO(b/194177024): re-enable this test.
-TEST_F(QuicRandomTest, QUIC_TEST_DISABLED_IN_CHROME(InsecureRandUint64)) {
+TEST_F(QuicRandomTest, InsecureRandUint64) {
QuicRandom* rng = QuicRandom::GetInstance();
uint64_t value1 = rng->InsecureRandUint64();
uint64_t value2 = rng->InsecureRandUint64();
diff --git a/quic/core/quic_connection_test.cc b/quic/core/quic_connection_test.cc
index 9aa3c2c..301639b 100644
--- a/quic/core/quic_connection_test.cc
+++ b/quic/core/quic_connection_test.cc
@@ -7026,8 +7026,7 @@
IsError(QUIC_PUBLIC_RESET));
}
-// TODO(b/194177024): re-enable this test.
-TEST_P(QuicConnectionTest, QUIC_TEST_DISABLED_IN_CHROME(IetfStatelessReset)) {
+TEST_P(QuicConnectionTest, IetfStatelessReset) {
if (!GetParam().version.HasIetfInvariantHeader()) {
return;
}
@@ -12028,9 +12027,7 @@
EXPECT_EQ(2u, writer_->packets_write_attempts());
}
-// TODO(b/194177024): re-enable this test.
-TEST_P(QuicConnectionTest,
- QUIC_TEST_DISABLED_IN_CHROME(PathValidationReceivesStatelessReset)) {
+TEST_P(QuicConnectionTest, PathValidationReceivesStatelessReset) {
if (!VersionHasIetfQuicFrames(connection_.version().transport_version) ||
!connection_.use_path_validator()) {
return;
diff --git a/quic/core/quic_framer_test.cc b/quic/core/quic_framer_test.cc
index 6237bc6..f36bf5c 100644
--- a/quic/core/quic_framer_test.cc
+++ b/quic/core/quic_framer_test.cc
@@ -9237,9 +9237,7 @@
}
}
-// TODO(b/194177024): re-enable this test.
-TEST_P(QuicFramerTest,
- QUIC_TEST_DISABLED_IN_CHROME(BuildIetfStatelessResetPacket)) {
+TEST_P(QuicFramerTest, BuildIetfStatelessResetPacket) {
if (GetQuicRestartFlag(quic_fix_stateless_reset2)) {
// clang-format off
unsigned char packet[] = {
diff --git a/quic/core/quic_time_wait_list_manager_test.cc b/quic/core/quic_time_wait_list_manager_test.cc
index c31d289..f47bdf2 100644
--- a/quic/core/quic_time_wait_list_manager_test.cc
+++ b/quic/core/quic_time_wait_list_manager_test.cc
@@ -358,9 +358,7 @@
ProcessPacket(connection_id_);
}
-// TODO(b/194177024): re-enable this test.
-TEST_F(QuicTimeWaitListManagerTest,
- QUIC_TEST_DISABLED_IN_CHROME(SendPublicReset)) {
+TEST_F(QuicTimeWaitListManagerTest, SendPublicReset) {
EXPECT_CALL(visitor_, OnConnectionAddedToTimeWaitList(connection_id_));
AddConnectionId(connection_id_,
QuicTimeWaitListManager::SEND_STATELESS_RESET);
