From e593b561987540b6d309a33eafcdc5ab65b71e73 Mon Sep 17 00:00:00 2001 From: Jeff Elsloo Date: Mon, 8 Nov 2021 11:52:46 -0700 Subject: [PATCH 1/4] * Fixes segfault scenario when unknown option(s) are encountered * Changes behavior to ignore unknown options rather than rejecting the entire config and refusing to load --- plugins/cache_promote/configs.cc | 14 +++++++++----- plugins/cache_promote/policy_manager.cc | 10 +++++----- 2 files changed, 14 insertions(+), 10 deletions(-) diff --git a/plugins/cache_promote/configs.cc b/plugins/cache_promote/configs.cc index 30015b54e0b..867484ddae6 100644 --- a/plugins/cache_promote/configs.cc +++ b/plugins/cache_promote/configs.cc @@ -42,7 +42,9 @@ static const struct option longopt[] = { // The destructor is responsible for returning the policy to the PolicyManager. PromotionConfig::~PromotionConfig() { - _manager->releasePolicy(_policy); + if (_policy) { + _manager->releasePolicy(_policy); + } } // Parse the command line arguments to the plugin, and instantiate the appropriate policy @@ -86,10 +88,8 @@ PromotionConfig::factory(int argc, char *argv[]) TSDebug(PLUGIN_NAME, "internal_enabled set to true"); } else { if (!_policy->parseOption(opt, optarg)) { - TSError("[%s] The specified policy (%s) does not support the -%c option", PLUGIN_NAME, _policy->policyName(), opt); - delete _policy; - _policy = nullptr; - return false; + TSError("[%s] The specified policy (%s) does not support the -%c option; skipping this argument", PLUGIN_NAME, + _policy->policyName(), opt); } } } else { @@ -99,6 +99,10 @@ PromotionConfig::factory(int argc, char *argv[]) } } + if (!_policy) { + return false; + } + // Coalesce any LRU policies via the LRU manager. This is a little ugly, but it makes configuration // easier, and order of options doesn't matter. diff --git a/plugins/cache_promote/policy_manager.cc b/plugins/cache_promote/policy_manager.cc index de3ecb0f82b..b098d1b97b9 100644 --- a/plugins/cache_promote/policy_manager.cc +++ b/plugins/cache_promote/policy_manager.cc @@ -48,7 +48,7 @@ PolicyManager::releasePolicy(PromotionPolicy *policy) { std::string tag = policy->id(); - if (tag.size() != 0) { + if (tag.size() != 0) { // this is always the case for instances of LRUPolicy auto res = _policies.find(tag); if (res != _policies.end()) { @@ -58,10 +58,10 @@ PolicyManager::releasePolicy(PromotionPolicy *policy) _policies.erase(res); } } else { - TSAssert(!"Trying to release a policy which was not acquired via PolicyManager"); + TSDebug(PLUGIN_NAME, "Tried to release a policy which was not properly initialized nor acquired via PolicyManager"); } - } else { - // Not managed by the policy manager, so just nuke it. - delete policy; } + + // Not managed by the policy manager, so just nuke it. + delete policy; } From 945f3ca1c885ba5e5350ce6af2204324f977a39b Mon Sep 17 00:00:00 2001 From: Jeff Elsloo Date: Tue, 9 Nov 2021 09:33:15 -0700 Subject: [PATCH 2/4] Fixed a double free that leads to a segfault on config reload. --- plugins/cache_promote/policy_manager.cc | 1 + 1 file changed, 1 insertion(+) diff --git a/plugins/cache_promote/policy_manager.cc b/plugins/cache_promote/policy_manager.cc index b098d1b97b9..171878841fa 100644 --- a/plugins/cache_promote/policy_manager.cc +++ b/plugins/cache_promote/policy_manager.cc @@ -56,6 +56,7 @@ PolicyManager::releasePolicy(PromotionPolicy *policy) TSDebug(PLUGIN_NAME, "releasing unused PromotionPolicy"); delete res->second.first; _policies.erase(res); + return; } } else { TSDebug(PLUGIN_NAME, "Tried to release a policy which was not properly initialized nor acquired via PolicyManager"); From c77f1e23148932b6265826818c7dd99066ae679b Mon Sep 17 00:00:00 2001 From: Jeff Elsloo Date: Tue, 9 Nov 2021 10:56:32 -0700 Subject: [PATCH 3/4] Modified null checks to explicitly compare against nullptr. --- plugins/cache_promote/configs.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/plugins/cache_promote/configs.cc b/plugins/cache_promote/configs.cc index 867484ddae6..d38d963db89 100644 --- a/plugins/cache_promote/configs.cc +++ b/plugins/cache_promote/configs.cc @@ -42,7 +42,7 @@ static const struct option longopt[] = { // The destructor is responsible for returning the policy to the PolicyManager. PromotionConfig::~PromotionConfig() { - if (_policy) { + if (_policy != nullptr) { _manager->releasePolicy(_policy); } } @@ -99,7 +99,7 @@ PromotionConfig::factory(int argc, char *argv[]) } } - if (!_policy) { + if (_policy == nullptr) { return false; } From ec5c0f1c446a9751da12cda07df3c74fc215918c Mon Sep 17 00:00:00 2001 From: Jeff Elsloo Date: Wed, 10 Nov 2021 10:52:06 -0700 Subject: [PATCH 4/4] * Fixes a crash introduced by this PR caused by freeing a repurposed policy --- plugins/cache_promote/policy_manager.cc | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/plugins/cache_promote/policy_manager.cc b/plugins/cache_promote/policy_manager.cc index 171878841fa..2b30adb2c99 100644 --- a/plugins/cache_promote/policy_manager.cc +++ b/plugins/cache_promote/policy_manager.cc @@ -56,8 +56,9 @@ PolicyManager::releasePolicy(PromotionPolicy *policy) TSDebug(PLUGIN_NAME, "releasing unused PromotionPolicy"); delete res->second.first; _policies.erase(res); - return; } + + return; } else { TSDebug(PLUGIN_NAME, "Tried to release a policy which was not properly initialized nor acquired via PolicyManager"); }