Skip to content

Commit

Permalink
server: fixed nullptr crash in server properties cache (#35268)
Browse files Browse the repository at this point in the history 
Signed-off-by: Christoph Pakulski <[email protected]>
  • Loading branch information
@cpakulski
cpakulski authored Jul 19, 2024
1 parent 189dd47 commit 1abf5e1
Show file tree
Hide file tree
Showing 2 changed files with 75 additions and 56 deletions.
4 changes: 3 additions & 1 deletion source/common/http/http_server_properties_cache_impl.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -234,7 +234,9 @@ HttpServerPropertiesCacheImpl::addOriginData(const Origin& origin, OriginData&&
ASSERT(protocols_.find(origin) == protocols_.end());
while (protocols_.size() >= max_entries_) {
auto iter = protocols_.begin();
key_value_store_->remove(originToString(iter->first));
if (key_value_store_) {
key_value_store_->remove(originToString(iter->first));
}
protocols_.erase(iter);
}
protocols_[origin] = std::move(origin_data);
Expand Down
127 changes: 72 additions & 55 deletions test/common/http/http_server_properties_cache_impl_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ namespace Http {
namespace {

static const absl::optional<std::chrono::seconds> kNoTtl = absl::nullopt;
class HttpServerPropertiesCacheImplTest : public testing::Test {
class HttpServerPropertiesCacheImplTest : public testing::TestWithParam<Envoy::MockKeyValueStore*> {
public:
HttpServerPropertiesCacheImplTest()
: dispatcher_([]() {
Expand All @@ -24,8 +24,7 @@ class HttpServerPropertiesCacheImplTest : public testing::Test {
[]() { return std::make_unique<Event::SimulatedTimeSystemHelper>(); });
return NiceMock<Event::MockDispatcher>();
}()),
store_(new NiceMock<MockKeyValueStore>()),
expiration1_(dispatcher_.timeSource().monotonicTime() + Seconds(5)),
store_(GetParam()), expiration1_(dispatcher_.timeSource().monotonicTime() + Seconds(5)),
expiration2_(dispatcher_.timeSource().monotonicTime() + Seconds(10)),
protocol1_(alpn1_, hostname1_, port1_, expiration1_),
protocol2_(alpn2_, hostname2_, port2_, expiration2_), protocols1_({protocol1_}),
Expand Down Expand Up @@ -70,64 +69,69 @@ class HttpServerPropertiesCacheImplTest : public testing::Test {
std::vector<HttpServerPropertiesCacheImpl::AlternateProtocol> protocols2_;
};

TEST_F(HttpServerPropertiesCacheImplTest, Init) {
#define EXPECT_CALL_WHEN_STORE_VALID(method) \
if (store_) { \
EXPECT_CALL(*store_, method); \
}

TEST_P(HttpServerPropertiesCacheImplTest, Init) {
initialize();
EXPECT_EQ(0, protocols_->size());
}

TEST_F(HttpServerPropertiesCacheImplTest, SetAlternativesThenSrtt) {
TEST_P(HttpServerPropertiesCacheImplTest, SetAlternativesThenSrtt) {
initialize();
EXPECT_EQ(0, protocols_->size());
EXPECT_EQ(std::chrono::microseconds(0), protocols_->getSrtt(origin1_));
EXPECT_CALL(*store_,
addOrUpdate("https://hostname1:1", "alpn1=\"hostname1:1\"; ma=5|0|0", kNoTtl));
EXPECT_CALL_WHEN_STORE_VALID(
addOrUpdate("https://hostname1:1", "alpn1=\"hostname1:1\"; ma=5|0|0", kNoTtl));
protocols_->setAlternatives(origin1_, protocols1_);
EXPECT_CALL(*store_,
addOrUpdate("https://hostname1:1", "alpn1=\"hostname1:1\"; ma=5|5|0", kNoTtl));
EXPECT_CALL_WHEN_STORE_VALID(
addOrUpdate("https://hostname1:1", "alpn1=\"hostname1:1\"; ma=5|5|0", kNoTtl));
protocols_->setSrtt(origin1_, std::chrono::microseconds(5));
EXPECT_EQ(1, protocols_->size());
EXPECT_EQ(std::chrono::microseconds(5), protocols_->getSrtt(origin1_));
}

TEST_F(HttpServerPropertiesCacheImplTest, SetSrttThenAlternatives) {
TEST_P(HttpServerPropertiesCacheImplTest, SetSrttThenAlternatives) {
initialize();
EXPECT_EQ(0, protocols_->size());
EXPECT_CALL(*store_, addOrUpdate("https://hostname1:1", "clear|5|0", kNoTtl));
EXPECT_CALL_WHEN_STORE_VALID(addOrUpdate("https://hostname1:1", "clear|5|0", kNoTtl));
protocols_->setSrtt(origin1_, std::chrono::microseconds(5));
EXPECT_EQ(1, protocols_->size());
EXPECT_EQ(std::chrono::microseconds(5), protocols_->getSrtt(origin1_));
EXPECT_CALL(*store_,
addOrUpdate("https://hostname1:1", "alpn1=\"hostname1:1\"; ma=5|5|0", kNoTtl));
EXPECT_CALL_WHEN_STORE_VALID(
addOrUpdate("https://hostname1:1", "alpn1=\"hostname1:1\"; ma=5|5|0", kNoTtl));
protocols_->setAlternatives(origin1_, protocols1_);
EXPECT_EQ(std::chrono::microseconds(5), protocols_->getSrtt(origin1_));
}

TEST_F(HttpServerPropertiesCacheImplTest, SetConcurrency) {
TEST_P(HttpServerPropertiesCacheImplTest, SetConcurrency) {
initialize();
EXPECT_EQ(0, protocols_->size());
EXPECT_CALL(*store_, addOrUpdate("https://hostname1:1", "clear|0|5", kNoTtl));
EXPECT_CALL_WHEN_STORE_VALID(addOrUpdate("https://hostname1:1", "clear|0|5", kNoTtl));
protocols_->setConcurrentStreams(origin1_, 5);
EXPECT_EQ(5, protocols_->getConcurrentStreams(origin1_));
}

TEST_F(HttpServerPropertiesCacheImplTest, FindAlternatives) {
TEST_P(HttpServerPropertiesCacheImplTest, FindAlternatives) {
initialize();
EXPECT_CALL(*store_,
addOrUpdate("https://hostname1:1", "alpn1=\"hostname1:1\"; ma=5|0|0", kNoTtl));
EXPECT_CALL_WHEN_STORE_VALID(
addOrUpdate("https://hostname1:1", "alpn1=\"hostname1:1\"; ma=5|0|0", kNoTtl));
protocols_->setAlternatives(origin1_, protocols1_);
OptRef<const std::vector<HttpServerPropertiesCacheImpl::AlternateProtocol>> protocols =
protocols_->findAlternatives(origin1_);
ASSERT_TRUE(protocols.has_value());
EXPECT_EQ(protocols1_, protocols.ref());
}

TEST_F(HttpServerPropertiesCacheImplTest, FindAlternativesAfterReplacement) {
TEST_P(HttpServerPropertiesCacheImplTest, FindAlternativesAfterReplacement) {
initialize();
EXPECT_CALL(*store_,
addOrUpdate("https://hostname1:1", "alpn1=\"hostname1:1\"; ma=5|0|0", kNoTtl));
EXPECT_CALL_WHEN_STORE_VALID(
addOrUpdate("https://hostname1:1", "alpn1=\"hostname1:1\"; ma=5|0|0", kNoTtl));
protocols_->setAlternatives(origin1_, protocols1_);
EXPECT_CALL(*store_,
addOrUpdate("https://hostname1:1", "alpn2=\"hostname2:2\"; ma=10|0|0", kNoTtl));
EXPECT_CALL_WHEN_STORE_VALID(
addOrUpdate("https://hostname1:1", "alpn2=\"hostname2:2\"; ma=10|0|0", kNoTtl));
protocols_->setAlternatives(origin1_, protocols2_);
OptRef<const std::vector<HttpServerPropertiesCacheImpl::AlternateProtocol>> protocols =
protocols_->findAlternatives(origin1_);
Expand All @@ -136,13 +140,13 @@ TEST_F(HttpServerPropertiesCacheImplTest, FindAlternativesAfterReplacement) {
EXPECT_NE(protocols1_, protocols.ref());
}

TEST_F(HttpServerPropertiesCacheImplTest, FindAlternativesForMultipleOrigins) {
TEST_P(HttpServerPropertiesCacheImplTest, FindAlternativesForMultipleOrigins) {
initialize();
EXPECT_CALL(*store_,
addOrUpdate("https://hostname1:1", "alpn1=\"hostname1:1\"; ma=5|0|0", kNoTtl));
EXPECT_CALL_WHEN_STORE_VALID(
addOrUpdate("https://hostname1:1", "alpn1=\"hostname1:1\"; ma=5|0|0", kNoTtl));
protocols_->setAlternatives(origin1_, protocols1_);
EXPECT_CALL(*store_,
addOrUpdate("https://hostname2:2", "alpn2=\"hostname2:2\"; ma=10|0|0", kNoTtl));
EXPECT_CALL_WHEN_STORE_VALID(
addOrUpdate("https://hostname2:2", "alpn2=\"hostname2:2\"; ma=10|0|0", kNoTtl));
protocols_->setAlternatives(origin2_, protocols2_);
OptRef<const std::vector<HttpServerPropertiesCacheImpl::AlternateProtocol>> protocols =
protocols_->findAlternatives(origin1_);
Expand All @@ -153,37 +157,37 @@ TEST_F(HttpServerPropertiesCacheImplTest, FindAlternativesForMultipleOrigins) {
EXPECT_EQ(protocols2_, protocols.ref());
}

TEST_F(HttpServerPropertiesCacheImplTest, FindAlternativesAfterExpiration) {
TEST_P(HttpServerPropertiesCacheImplTest, FindAlternativesAfterExpiration) {
initialize();
EXPECT_CALL(*store_,
addOrUpdate("https://hostname1:1", "alpn1=\"hostname1:1\"; ma=5|0|0", kNoTtl));
EXPECT_CALL_WHEN_STORE_VALID(
addOrUpdate("https://hostname1:1", "alpn1=\"hostname1:1\"; ma=5|0|0", kNoTtl));
protocols_->setAlternatives(origin1_, protocols1_);
dispatcher_.globalTimeSystem().advanceTimeWait(Seconds(6));
EXPECT_CALL(*store_, remove("https://hostname1:1"));
EXPECT_CALL_WHEN_STORE_VALID(remove("https://hostname1:1"));
OptRef<const std::vector<HttpServerPropertiesCacheImpl::AlternateProtocol>> protocols =
protocols_->findAlternatives(origin1_);
ASSERT_FALSE(protocols.has_value());
EXPECT_EQ(1u, protocols_->size());
}

TEST_F(HttpServerPropertiesCacheImplTest, FindAlternativesAfterPartialExpiration) {
TEST_P(HttpServerPropertiesCacheImplTest, FindAlternativesAfterPartialExpiration) {
initialize();
EXPECT_CALL(*store_,
addOrUpdate("https://hostname1:1",
"alpn1=\"hostname1:1\"; ma=5,alpn2=\"hostname2:2\"; ma=10|0|0", kNoTtl));
EXPECT_CALL_WHEN_STORE_VALID(
addOrUpdate("https://hostname1:1",
"alpn1=\"hostname1:1\"; ma=5,alpn2=\"hostname2:2\"; ma=10|0|0", kNoTtl));
std::vector<HttpServerPropertiesCacheImpl::AlternateProtocol> both = {protocol1_, protocol2_};
protocols_->setAlternatives(origin1_, both);
dispatcher_.globalTimeSystem().advanceTimeWait(Seconds(6));
EXPECT_CALL(*store_,
addOrUpdate("https://hostname1:1", "alpn2=\"hostname2:2\"; ma=10|0|0", kNoTtl));
EXPECT_CALL_WHEN_STORE_VALID(
addOrUpdate("https://hostname1:1", "alpn2=\"hostname2:2\"; ma=10|0|0", kNoTtl));
OptRef<const std::vector<HttpServerPropertiesCacheImpl::AlternateProtocol>> protocols =
protocols_->findAlternatives(origin1_);
ASSERT_TRUE(protocols.has_value());
EXPECT_EQ(protocols2_.size(), protocols->size());
EXPECT_EQ(protocols2_, protocols.ref());
}

TEST_F(HttpServerPropertiesCacheImplTest, FindAlternativesAfterTruncation) {
TEST_P(HttpServerPropertiesCacheImplTest, FindAlternativesAfterTruncation) {
initialize();
std::vector<HttpServerPropertiesCacheImpl::AlternateProtocol> expected_protocols;
for (size_t i = 0; i < 10; ++i) {
Expand All @@ -203,7 +207,7 @@ TEST_F(HttpServerPropertiesCacheImplTest, FindAlternativesAfterTruncation) {
EXPECT_EQ(expected_protocols, protocols.ref());
}

TEST_F(HttpServerPropertiesCacheImplTest, ToAndFromOriginString) {
TEST_P(HttpServerPropertiesCacheImplTest, ToAndFromOriginString) {
initialize();
std::string origin_str = "https://hostname1:1";
absl::optional<HttpServerPropertiesCache::Origin> origin =
Expand Down Expand Up @@ -233,8 +237,10 @@ TEST_F(HttpServerPropertiesCacheImplTest, ToAndFromOriginString) {
// Negative port.
EXPECT_TRUE(!HttpServerPropertiesCacheImpl::stringToOrigin("https://:-1").has_value());
}

TEST_F(HttpServerPropertiesCacheImplTest, MaxEntries) {
TEST_P(HttpServerPropertiesCacheImplTest, MaxEntries) {
// This test requires store. Do not execute it when store is not present.
if (store_ == nullptr)
return;
initialize();
EXPECT_EQ(0, protocols_->size());
const std::string hostname = "hostname";
Expand All @@ -251,7 +257,7 @@ TEST_F(HttpServerPropertiesCacheImplTest, MaxEntries) {
}
}

TEST_F(HttpServerPropertiesCacheImplTest, ToAndFromString) {
TEST_P(HttpServerPropertiesCacheImplTest, ToAndFromString) {
initialize();
auto testAltSvc = [&](const std::string& original_alt_svc,
const std::string& expected_alt_svc) -> void {
Expand Down Expand Up @@ -295,7 +301,7 @@ TEST_F(HttpServerPropertiesCacheImplTest, ToAndFromString) {
testAltSvc("h3-29=\":443\"; ma=86460|2000|0", "h3-29=\":443\"; ma=86460|2000|0");
}

TEST_F(HttpServerPropertiesCacheImplTest, InvalidString) {
TEST_P(HttpServerPropertiesCacheImplTest, InvalidString) {
initialize();
// Too many numbers
EXPECT_FALSE(
Expand All @@ -319,7 +325,10 @@ TEST_F(HttpServerPropertiesCacheImplTest, InvalidString) {
.has_value());
}

TEST_F(HttpServerPropertiesCacheImplTest, CacheLoad) {
TEST_P(HttpServerPropertiesCacheImplTest, CacheLoad) {
// This test requires store. Do not execute it when store is not present.
if (store_ == nullptr)
return;
EXPECT_CALL(*store_, iterate(_)).WillOnce(Invoke([&](KeyValueStore::ConstIterateCb fn) {
fn("foo", "bar");
fn("https://hostname1:1", "alpn1=\"hostname1:1\"; ma=5|2|3");
Expand All @@ -339,7 +348,10 @@ TEST_F(HttpServerPropertiesCacheImplTest, CacheLoad) {
EXPECT_EQ(3, protocols_->getConcurrentStreams(origin1_));
}

TEST_F(HttpServerPropertiesCacheImplTest, CacheLoadSrttOnly) {
TEST_P(HttpServerPropertiesCacheImplTest, CacheLoadSrttOnly) {
// This test requires store. Do not execute it when store is not present.
if (store_ == nullptr)
return;
EXPECT_CALL(*store_, iterate(_)).WillOnce(Invoke([&](KeyValueStore::ConstIterateCb fn) {
fn("https://hostname1:1", "clear|5|0");
}));
Expand All @@ -350,15 +362,17 @@ TEST_F(HttpServerPropertiesCacheImplTest, CacheLoadSrttOnly) {
EXPECT_EQ(std::chrono::microseconds(5), protocols_->getSrtt(origin1_));
}

TEST_F(HttpServerPropertiesCacheImplTest, ShouldNotUpdateStoreOnCacheLoad) {
EXPECT_CALL(*store_, addOrUpdate(_, _, _)).Times(0);
EXPECT_CALL(*store_, iterate(_)).WillOnce(Invoke([&](KeyValueStore::ConstIterateCb fn) {
fn("https://hostname1:1", "alpn1=\"hostname1:1\"; ma=5|0|0");
}));
TEST_P(HttpServerPropertiesCacheImplTest, ShouldNotUpdateStoreOnCacheLoad) {
if (store_ != nullptr) {
EXPECT_CALL(*store_, addOrUpdate(_, _, _)).Times(0);
EXPECT_CALL(*store_, iterate(_)).WillOnce(Invoke([&](KeyValueStore::ConstIterateCb fn) {
fn("https://hostname1:1", "alpn1=\"hostname1:1\"; ma=5|0|0");
}));
}
initialize();
}

TEST_F(HttpServerPropertiesCacheImplTest, GetOrCreateHttp3StatusTracker) {
TEST_P(HttpServerPropertiesCacheImplTest, GetOrCreateHttp3StatusTracker) {
max_entries_ = 1u;
initialize();
EXPECT_EQ(0u, protocols_->size());
Expand All @@ -373,7 +387,7 @@ TEST_F(HttpServerPropertiesCacheImplTest, GetOrCreateHttp3StatusTracker) {
EXPECT_FALSE(protocols_->getOrCreateHttp3StatusTracker(origin1_).isHttp3Broken());
}

TEST_F(HttpServerPropertiesCacheImplTest, CanonicalSuffix) {
TEST_P(HttpServerPropertiesCacheImplTest, CanonicalSuffix) {
std::string suffix = ".example.com";
std::string host1 = "first.example.com";
std::string host2 = "www.second.example.com";
Expand All @@ -390,7 +404,7 @@ TEST_F(HttpServerPropertiesCacheImplTest, CanonicalSuffix) {
EXPECT_EQ(protocols1_, protocols.ref());
}

TEST_F(HttpServerPropertiesCacheImplTest, CanonicalSuffixNoMatch) {
TEST_P(HttpServerPropertiesCacheImplTest, CanonicalSuffixNoMatch) {
std::string suffix = ".example.com";
std::string host1 = "www.example.com";
std::string host2 = "www.other.com";
Expand All @@ -406,7 +420,7 @@ TEST_F(HttpServerPropertiesCacheImplTest, CanonicalSuffixNoMatch) {
ASSERT_FALSE(protocols.has_value());
}

TEST_F(HttpServerPropertiesCacheImplTest, ExplicitAlternativeTakesPriorityOverCanonicalSuffix) {
TEST_P(HttpServerPropertiesCacheImplTest, ExplicitAlternativeTakesPriorityOverCanonicalSuffix) {
std::string suffix = ".example.com";
std::string host1 = "first.example.com";
std::string host2 = "second.example.com";
Expand All @@ -424,6 +438,9 @@ TEST_F(HttpServerPropertiesCacheImplTest, ExplicitAlternativeTakesPriorityOverCa
EXPECT_EQ(protocols2_, protocols.ref());
}

// Execute all tests when key value store is nullptr and when it is valid.
INSTANTIATE_TEST_SUITE_P(HttpServerPropertiesCacheImplTestSuite, HttpServerPropertiesCacheImplTest,
testing::Values(nullptr, new NiceMock<MockKeyValueStore>()));
} // namespace
} // namespace Http
} // namespace Envoy

0 comments on commit 1abf5e1

Please sign in to comment.