From d2f593e4d9f92f2eaff3686e88b65cc640526bd5 Mon Sep 17 00:00:00 2001
From: Simon Morlat <simon.morlat@linphone.org>
Date: Fri, 24 Feb 2023 13:08:49 +0100
Subject: [PATCH] Fix leak of nat_policy_X section in configuration file, and
add code to automatically cleanup orphan sections.
---
coreapi/linphonecore.c | 9 +++----
coreapi/lpconfig.c | 2 ++
coreapi/proxy.c | 2 ++
src/account/account-params.cpp | 22 ++++++----------
src/account/account-params.h | 9 +++----
src/account/account.cpp | 2 +-
src/account/account.h | 2 +-
src/c-wrapper/api/c-account-params.cpp | 9 ++++---
src/core/core-p.h | 3 +++
src/core/core.cpp | 24 ++++++++++++++++++
src/nat/nat-policy.cpp | 35 ++++++++++++--------------
src/nat/nat-policy.h | 4 +--
tester/tester.c | 21 +++++++++++++++-
13 files changed, 93 insertions(+), 51 deletions(-)
diff --git a/coreapi/linphonecore.c b/coreapi/linphonecore.c
index 7ee7180dde..775e785a7a 100644
--- a/coreapi/linphonecore.c
+++ b/coreapi/linphonecore.c
@@ -6007,9 +6007,8 @@ void linphone_core_enable_mic(LinphoneCore *lc, bool_t enable) {
const bctbx_list_t *list;
const bctbx_list_t *elem;
- ms_message("linphone_core_enable_mic(): new state is [%s], current state is [%s]",
- enable ? "enabled" : "disabled",
- lc->sound_conf.mic_enabled ? "enabled" : "disabled");
+ ms_message("linphone_core_enable_mic(): new state is [%s], current state is [%s]", enable ? "enabled" : "disabled",
+ lc->sound_conf.mic_enabled ? "enabled" : "disabled");
lc->sound_conf.mic_enabled = enable; /* this is a global switch read everywhere the microphone is used. */
/* apply to conference and calls */
if (linphone_core_is_in_conference(lc)) {
@@ -6056,7 +6055,7 @@ void linphone_core_send_dtmf(LinphoneCore *lc, char dtmf) {
void linphone_core_set_stun_server(LinphoneCore *lc, const char *server) {
if (lc->nat_policy != NULL) {
linphone_nat_policy_set_stun_server(lc->nat_policy, server);
- NatPolicy::toCpp(lc->nat_policy)->saveToConfig();
+ L_GET_PRIVATE_FROM_C_OBJECT(lc)->writeNatPolicyConfigurations();
} else {
linphone_config_set_string(lc->config, "net", "stun_server", server);
}
@@ -6175,7 +6174,7 @@ void linphone_core_set_nat_policy(LinphoneCore *lc, LinphoneNatPolicy *policy) {
/*start an immediate (but asynchronous) resolution.*/
linphone_nat_policy_resolve_stun_server(policy);
linphone_config_set_string(lc->config, "net", "nat_policy_ref", NatPolicy::toCpp(policy)->getRef().c_str());
- NatPolicy::toCpp(policy)->saveToConfig();
+ L_GET_PRIVATE_FROM_C_OBJECT(lc)->writeNatPolicyConfigurations();
}
lc->sal->enableNatHelper(!!linphone_config_get_int(lc->config, "net", "enable_nat_helper", 1));
diff --git a/coreapi/lpconfig.c b/coreapi/lpconfig.c
index 77872e7884..bfa6e5824f 100644
--- a/coreapi/lpconfig.c
+++ b/coreapi/lpconfig.c
@@ -112,8 +112,10 @@ struct _LpConfig {
BELLE_SIP_DECLARE_NO_IMPLEMENTED_INTERFACES(LinphoneConfig);
BELLE_SIP_DECLARE_VPTR_NO_EXPORT(LinphoneConfig);
+#ifndef _MSC_VER
#pragma GCC diagnostic push
#pragma GCC diagnostic ignored "-Wunused-parameter"
+#endif
char *lp_realpath(const char *file, char *name) {
#if defined(_WIN32) || defined(__QNX__) || defined(__ANDROID__)
diff --git a/coreapi/proxy.c b/coreapi/proxy.c
index 674b902fba..f7b4692b69 100644
--- a/coreapi/proxy.c
+++ b/coreapi/proxy.c
@@ -44,6 +44,7 @@
#include "mediastreamer2/mediastream.h"
+#include "core/core-p.h"
#include "core/core.h"
#include "enum.h"
#include "private.h"
@@ -81,6 +82,7 @@ void linphone_proxy_config_write_all_to_config_file(LinphoneCore *lc) {
/*to ensure removed configs are erased:*/
linphone_proxy_config_write_to_config_file(lc->config, NULL, i);
linphone_config_set_int(lc->config, "sip", "default_proxy", linphone_core_get_default_proxy_config_index(lc));
+ L_GET_PRIVATE_FROM_C_OBJECT(lc)->writeNatPolicyConfigurations();
}
static void linphone_proxy_config_init(LinphoneCore *lc, LinphoneProxyConfig *cfg) {
diff --git a/src/account/account-params.cpp b/src/account/account-params.cpp
index d070572070..e1a594181a 100644
--- a/src/account/account-params.cpp
+++ b/src/account/account-params.cpp
@@ -108,8 +108,7 @@ AccountParams::AccountParams(LinphoneCore *lc) {
natPolicyRef);
}
if (policy) {
- setNatPolicy(policy->toC());
- policy->unref();
+ setNatPolicy(policy->toSharedPtr());
} else {
lError() << "Cannot create default nat policy with ref [" << natPolicyRef << "] for account [" << this
<< "]";
@@ -234,18 +233,19 @@ AccountParams::AccountParams(LinphoneCore *lc, int index) : AccountParams(lc) {
const char *nat_policy_ref = linphone_config_get_string(config, key, "nat_policy_ref", NULL);
if (nat_policy_ref != NULL) {
- if (mNatPolicy) linphone_nat_policy_unref(mNatPolicy);
/* CAUTION: the nat_policy_ref meaning in default values is different than in usual [nat_policy_%i] section.
* This is not consistent and error-prone.
* Normally, the nat_policy_ref refers to a "ref" entry within a [nat_policy_%i] section.
*/
+ LinphoneNatPolicy *natPolicy;
if (linphone_config_has_section(config, nat_policy_ref)) {
/* Odd method - to be deprecated, inconsistent */
- mNatPolicy = linphone_core_create_nat_policy_from_config(lc, nat_policy_ref);
+ natPolicy = linphone_core_create_nat_policy_from_config(lc, nat_policy_ref);
} else {
/* Usual method */
- mNatPolicy = linphone_core_create_nat_policy_from_ref(lc, nat_policy_ref);
+ natPolicy = linphone_core_create_nat_policy_from_ref(lc, nat_policy_ref);
}
+ mNatPolicy = NatPolicy::toCpp(natPolicy)->toSharedPtr();
}
mConferenceFactoryUri =
@@ -323,7 +323,6 @@ AccountParams::~AccountParams() {
if (mProxyAddress) linphone_address_unref(mProxyAddress);
if (mRoutes) bctbx_list_free_with_data(mRoutes, (bctbx_list_free_func)linphone_address_unref);
if (mRoutesString) bctbx_list_free_with_data(mRoutesString, (bctbx_list_free_func)bctbx_free);
- if (mNatPolicy) linphone_nat_policy_unref(mNatPolicy);
if (mPushNotificationConfig) mPushNotificationConfig->unref();
if (mAudioVideoConferenceFactoryAddress) linphone_address_unref(mAudioVideoConferenceFactoryAddress);
if (mCustomContact) linphone_address_unref(mCustomContact);
@@ -557,11 +556,7 @@ void AccountParams::setAvpfMode(LinphoneAVPFMode avpfMode) {
mAvpfMode = avpfMode;
}
-void AccountParams::setNatPolicy(LinphoneNatPolicy *natPolicy) {
- if (natPolicy != nullptr) {
- linphone_nat_policy_ref(natPolicy); /* Prevent object destruction if the same policy is used */
- }
- if (mNatPolicy != nullptr) linphone_nat_policy_unref(mNatPolicy);
+void AccountParams::setNatPolicy(const shared_ptr<NatPolicy> &natPolicy) {
mNatPolicy = natPolicy;
}
@@ -750,7 +745,7 @@ LinphoneAVPFMode AccountParams::getAvpfMode() const {
return mAvpfMode;
}
-LinphoneNatPolicy *AccountParams::getNatPolicy() const {
+shared_ptr<NatPolicy> AccountParams::getNatPolicy() const {
return mNatPolicy;
}
@@ -916,8 +911,7 @@ void AccountParams::writeToConfigFile(LinphoneConfig *config, int index) {
linphone_config_set_int(config, key, "publish_expires", mPublishExpires);
if (mNatPolicy != NULL) {
- linphone_config_set_string(config, key, "nat_policy_ref", NatPolicy::toCpp(mNatPolicy)->getRef().c_str());
- NatPolicy::toCpp(mNatPolicy)->saveToConfig();
+ linphone_config_set_string(config, key, "nat_policy_ref", mNatPolicy->getRef().c_str());
}
linphone_config_set_string(config, key, "conference_factory_uri", mConferenceFactoryUri.c_str());
diff --git a/src/account/account-params.h b/src/account/account-params.h
index 3bb7104e27..64a02dd0aa 100644
--- a/src/account/account-params.h
+++ b/src/account/account-params.h
@@ -32,6 +32,7 @@
LINPHONE_BEGIN_NAMESPACE
class PushNotificationConfig;
+class NatPolicy;
class AccountParams : public bellesip::HybridObject<LinphoneAccountParams, AccountParams>, public CustomParams {
friend class Account;
@@ -44,7 +45,6 @@ public:
AccountParams *clone() const override;
- // Setters
void setExpires(int expires);
void setQualityReportingInterval(int qualityReportingInterval);
void setPublishExpires(int publishExpires);
@@ -72,7 +72,7 @@ public:
void setFileTranferServer(const std::string &fileTransferServer);
void setPrivacy(LinphonePrivacyMask privacy);
void setAvpfMode(LinphoneAVPFMode avpfMode);
- void setNatPolicy(LinphoneNatPolicy *natPolicy);
+ void setNatPolicy(const std::shared_ptr<NatPolicy> &natPolicy);
void setPushNotificationConfig(PushNotificationConfig *pushNotificationConfig);
LinphoneStatus setIdentityAddress(const LinphoneAddress *identityAddress);
LinphoneStatus setRoutes(const bctbx_list_t *routes);
@@ -83,7 +83,6 @@ public:
void setCustomContact(const LinphoneAddress *contact);
void setLimeServerUrl(const std::string &url);
- // Getters
int getExpires() const;
int getQualityReportingInterval() const;
int getPublishExpires() const;
@@ -117,7 +116,7 @@ public:
LinphonePrivacyMask getPrivacy() const;
LinphoneAddress *getIdentityAddress() const;
LinphoneAVPFMode getAvpfMode() const;
- LinphoneNatPolicy *getNatPolicy() const;
+ std::shared_ptr<NatPolicy> getNatPolicy() const;
PushNotificationConfig *getPushNotificationConfig() const;
const LinphoneAddress *getAudioVideoConferenceFactoryAddress() const;
bool rtpBundleEnabled() const;
@@ -182,7 +181,7 @@ private:
LinphoneAVPFMode mAvpfMode;
- LinphoneNatPolicy *mNatPolicy = nullptr;
+ std::shared_ptr<NatPolicy> mNatPolicy = nullptr;
PushNotificationConfig *mPushNotificationConfig;
diff --git a/src/account/account.cpp b/src/account/account.cpp
index c4e4959261..1f14b7eb6b 100644
--- a/src/account/account.cpp
+++ b/src/account/account.cpp
@@ -1216,7 +1216,7 @@ void Account::onAudioVideoConferenceFactoryAddressChanged(const LinphoneAddress
}
}
-void Account::onNatPolicyChanged(BCTBX_UNUSED(LinphoneNatPolicy *policy)) {
+void Account::onNatPolicyChanged(BCTBX_UNUSED(const std::shared_ptr<NatPolicy> &policy)) {
}
LinphoneProxyConfig *Account::getConfig() const {
diff --git a/src/account/account.h b/src/account/account.h
index eddb76d7b7..4c7905a4cf 100644
--- a/src/account/account.h
+++ b/src/account/account.h
@@ -141,7 +141,7 @@ private:
void onInternationalPrefixChanged();
void onConferenceFactoryUriChanged(const std::string &conferenceFactoryUri);
void onAudioVideoConferenceFactoryAddressChanged(const LinphoneAddress *audioVideoConferenceFactoryAddress);
- void onNatPolicyChanged(LinphoneNatPolicy *policy);
+ void onNatPolicyChanged(const std::shared_ptr<NatPolicy> &policy);
void onLimeServerUrlChanged(const std::string &limeServerUrl);
bool customContactChanged();
std::list<SalAddress *> getOtherContacts();
diff --git a/src/c-wrapper/api/c-account-params.cpp b/src/c-wrapper/api/c-account-params.cpp
index 244c84720f..24f6e34229 100644
--- a/src/c-wrapper/api/c-account-params.cpp
+++ b/src/c-wrapper/api/c-account-params.cpp
@@ -20,11 +20,13 @@
#include "linphone/api/c-account-params.h"
#include "account/account-params.h"
+
#include "c-wrapper/c-wrapper.h"
#include "c-wrapper/internal/c-tools.h"
#include "linphone/api/c-address.h"
#include "linphone/core.h"
#include "linphone/lpconfig.h"
+#include "nat/nat-policy.h"
#include "push-notification/push-notification-config.h"
// =============================================================================
@@ -295,11 +297,12 @@ void linphone_account_params_set_idkey(LinphoneAccountParams *params, const char
}
LinphoneNatPolicy *linphone_account_params_get_nat_policy(const LinphoneAccountParams *params) {
- return AccountParams::toCpp(params)->getNatPolicy();
+ auto pol = AccountParams::toCpp(params)->getNatPolicy();
+ return pol ? pol->toC() : nullptr;
}
void linphone_account_params_set_nat_policy(LinphoneAccountParams *params, LinphoneNatPolicy *policy) {
- AccountParams::toCpp(params)->setNatPolicy(policy);
+ AccountParams::toCpp(params)->setNatPolicy(policy ? NatPolicy::toCpp(policy)->getSharedFromThis() : nullptr);
}
void linphone_account_params_set_conference_factory_uri(LinphoneAccountParams *params, const char *uri) {
@@ -421,4 +424,4 @@ void linphone_account_params_set_lime_server_url(LinphoneAccountParams *params,
const char *linphone_account_params_get_lime_server_url(const LinphoneAccountParams *params) {
return L_STRING_TO_C(AccountParams::toCpp(params)->getLimeServerUrl());
-}
\ No newline at end of file
+}
diff --git a/src/core/core-p.h b/src/core/core-p.h
index b851ab7e58..0f105478af 100644
--- a/src/core/core-p.h
+++ b/src/core/core-p.h
@@ -180,6 +180,9 @@ public:
/* called by linphone_core_set_video_device() to update the video device in the running call or conference.*/
void updateVideoDevice();
+ /* centralized method to write down all NatPolicy used by Accounts or Core */
+ void writeNatPolicyConfigurations();
+
static const Utils::Version conferenceProtocolVersion;
static const Utils::Version groupChatProtocolVersion;
static const Utils::Version ephemeralProtocolVersion;
diff --git a/src/core/core.cpp b/src/core/core.cpp
index d55b15a8ac..b5fb316d39 100644
--- a/src/core/core.cpp
+++ b/src/core/core.cpp
@@ -190,6 +190,30 @@ void CorePrivate::unregisterListener(CoreListener *listener) {
listeners.remove(listener);
}
+void CorePrivate::writeNatPolicyConfigurations() {
+ int index = 0;
+ LinphoneCore *lc = getCCore();
+
+ if (!linphone_core_ready(lc)) return;
+
+ LinphoneConfig *config = linphone_core_get_config(lc);
+
+ if (lc->nat_policy) {
+ NatPolicy::toCpp(lc->nat_policy)->saveToConfig(config, index);
+ ++index;
+ }
+ const bctbx_list_t *elem;
+ for (elem = linphone_core_get_account_list(lc); elem != nullptr; elem = elem->next) {
+ LinphoneAccount *account = (LinphoneAccount *)elem->data;
+ auto natPolicy = Account::toCpp(account)->getAccountParams()->getNatPolicy();
+ if (natPolicy) {
+ natPolicy->saveToConfig(config, index);
+ ++index;
+ }
+ }
+ NatPolicy::clearConfigFromIndex(config, index);
+}
+
void Core::onStopAsyncBackgroundTaskStarted() {
L_D();
d->stopAsyncEndEnabled = false;
diff --git a/src/nat/nat-policy.cpp b/src/nat/nat-policy.cpp
index 621a30be67..ee26778f1f 100644
--- a/src/nat/nat-policy.cpp
+++ b/src/nat/nat-policy.cpp
@@ -121,25 +121,22 @@ void NatPolicy::saveToConfig(LinphoneConfig *config, int index) const {
bctbx_list_free(l);
}
-void NatPolicy::saveToConfig() {
- LinphoneConfig *config = linphone_core_get_config(getCore()->getCCore());
- char *section;
- int index;
- bool_t finished = FALSE;
-
- for (index = 0; finished != TRUE; index++) {
- section = belle_sip_strdup_printf("nat_policy_%i", index);
- if (linphone_config_has_section(config, section)) {
- const char *config_ref = linphone_config_get_string(config, section, "ref", NULL);
- if ((config_ref != NULL) && (strcmp(config_ref, mRef.c_str()) == 0)) {
- saveToConfig(config, index);
- finished = TRUE;
- }
- } else {
- saveToConfig(config, index);
- finished = TRUE;
- }
- belle_sip_free(section);
+void NatPolicy::clearConfigFromIndex(LinphoneConfig *config, int index) {
+ int purged = 0;
+ while (true) {
+ std::ostringstream ostr;
+ ostr << "nat_policy_" << index;
+ if (linphone_config_has_section(config, ostr.str().c_str())) {
+ linphone_config_clean_section(config, ostr.str().c_str());
+ ++purged;
+ } else break;
+ ++index;
+ }
+ /* Warn about suspicious number of orphan NatPolicy sections.
+ * A bug occured December 2022 caused a massive leak of nat_policy_X sections in configuration file.
+ */
+ if (purged > 5) {
+ lWarning() << "Purged [" << purged << "] unused NatPolicy sections from config file.";
}
}
diff --git a/src/nat/nat-policy.h b/src/nat/nat-policy.h
index 1bf003f410..b5ea4d076c 100644
--- a/src/nat/nat-policy.h
+++ b/src/nat/nat-policy.h
@@ -111,11 +111,11 @@ public:
void clear();
void release();
bool stunServerActivated() const;
- void saveToConfig();
void resolveStunServer();
+ void saveToConfig(LinphoneConfig *config, int index) const;
+ static void clearConfigFromIndex(LinphoneConfig *config, int index);
private:
- void saveToConfig(LinphoneConfig *config, int index) const;
void initFromSection(const LinphoneConfig *config, const char *section);
static void sStunServerResolved(void *data, belle_sip_resolver_results_t *results);
void stunServerResolved(belle_sip_resolver_results_t *results);
diff --git a/tester/tester.c b/tester/tester.c
index 157c69681d..ab702adce6 100644
--- a/tester/tester.c
+++ b/tester/tester.c
@@ -2690,6 +2690,25 @@ LinphoneCoreManager *linphone_core_manager_create_shared(const char *rc_file,
return manager;
}
+static void check_orphan_nat_policy_section(LinphoneCoreManager *mgr) {
+ bctbx_list_t *l = linphone_config_get_sections_names_list(linphone_core_get_config(mgr->lc));
+ bctbx_list_t *elem;
+ int nat_policy_count = 0;
+ int proxy_count = 0;
+
+ for (elem = l; elem != NULL; elem = elem->next) {
+ const char *name = (const char *)elem->data;
+ if (strstr(name, "nat_policy_") == name) {
+ nat_policy_count++;
+ } else if (strstr(name, "proxy_") == name) {
+ proxy_count++;
+ }
+ }
+ /* There can't be more nat_policy_ section than proxy_? section + 1 */
+ BC_ASSERT_LOWER(nat_policy_count, proxy_count + 1, int, "%i");
+ bctbx_list_free(l);
+}
+
void linphone_core_manager_stop(LinphoneCoreManager *mgr) {
if (mgr->lc) {
const char *record_file = linphone_core_get_record_file(mgr->lc);
@@ -2702,7 +2721,7 @@ void linphone_core_manager_stop(LinphoneCoreManager *mgr) {
}
linphone_core_stop(mgr->lc);
-
+ check_orphan_nat_policy_section(mgr);
linphone_core_unref(mgr->lc);
mgr->lc = NULL;
}
--
GitLab