From 2ef5b66c07b67af2193da48d54a00af38b7b425d Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Fran=C3=A7ois-Simon=20Fauteux-Chapleau?=
<francois-simon.fauteux-chapleau@savoirfairelinux.com>
Date: Wed, 27 Mar 2024 12:30:26 -0400
Subject: [PATCH] ice/turn transport: make sure PJSIP is initialized before
it's used
Using PJSIP functions without calling pj_init first can lead to subtle
non-deterministic bugs.
GitLab: #18
Change-Id: I9364fd247165c0ce19a8d0d42575fb66651b54a3
---
include/ice_transport_factory.h | 4 +++
include/pj_init_lock.h | 59 +++++++++++++++++++++++++++++++++
src/ice_transport.cpp | 7 +++-
src/turn/turn_transport.cpp | 3 +-
src/turn/turn_transport.h | 2 ++
5 files changed, 73 insertions(+), 2 deletions(-)
create mode 100644 include/pj_init_lock.h
diff --git a/include/ice_transport_factory.h b/include/ice_transport_factory.h
index df3367c..3e4c931 100644
--- a/include/ice_transport_factory.h
+++ b/include/ice_transport_factory.h
@@ -19,6 +19,7 @@
#include "ice_options.h"
#include "ice_transport.h"
#include "ip_utils.h"
+#include "pj_init_lock.h"
#include <functional>
#include <memory>
@@ -50,6 +51,9 @@ public:
std::shared_ptr<pj_caching_pool> getPoolCaching() { return cp_; }
private:
+ // Declaring pjInitLock_ before cp_ because its constructor needs to be called
+ // first (see constructor implementation for a comment with more information).
+ PjInitLock pjInitLock_;
std::shared_ptr<pj_caching_pool> cp_;
pj_ice_strans_cfg ice_cfg_;
std::shared_ptr<Logger> logger_ {};
diff --git a/include/pj_init_lock.h b/include/pj_init_lock.h
new file mode 100644
index 0000000..c521a8a
--- /dev/null
+++ b/include/pj_init_lock.h
@@ -0,0 +1,59 @@
+/*
+ * Copyright (C) 2024 Savoir-faire Linux Inc.
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation, either version 3 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program. If not, see <https://www.gnu.org/licenses/>.
+ */
+#pragma once
+
+#include <fmt/core.h>
+#include <mutex>
+#include <pj/errno.h>
+#include <pj/types.h>
+
+namespace dhtnet {
+
+// PJSIP expects the number of calls to pj_shutdown to match the number of calls
+// to pj_init (https://docs.pjsip.org/en/latest/specific-guides/develop/init_shutdown_thread.html).
+// The intended behavior seems to be the following:
+// - The first call to pj_init actually initializes the library; subsequent calls do nothing.
+// - All calls to pj_shutdown do nothing, except the last one which actually performs the shutdown.
+// Unfortunately, the way this logic is implemented in PJSIP is not thread-safe, so we're
+// responsible for making sure that these functions can't be called by two threads at the same time.
+class PjInitLock
+{
+private:
+ inline static std::mutex mutex_;
+
+public:
+ PjInitLock()
+ {
+ std::lock_guard lk(mutex_);
+ pj_status_t status = pj_init();
+
+ if (status != PJ_SUCCESS) {
+ char errorMessage[PJ_ERR_MSG_SIZE];
+ pj_strerror(status, errorMessage, sizeof(errorMessage));
+ throw std::runtime_error(
+ fmt::format("pj_init failed: {}", errorMessage));
+ }
+ }
+
+ ~PjInitLock()
+ {
+ std::lock_guard lk(mutex_);
+ pj_shutdown();
+ }
+};
+
+}
diff --git a/src/ice_transport.cpp b/src/ice_transport.cpp
index d9beef8..1edf00c 100644
--- a/src/ice_transport.cpp
+++ b/src/ice_transport.cpp
@@ -1799,7 +1799,12 @@ IceTransport::link() const
//==============================================================================
IceTransportFactory::IceTransportFactory(const std::shared_ptr<Logger>& logger)
- : cp_(new pj_caching_pool(),
+ : pjInitLock_()
+ // Warning: pj_caching_pool_destroy will segfault if it's called before
+ // pj_caching_pool_init. Hence, any member which appears in the initializer
+ // list and whose constructor can fail (such as pjInitLock_) must be constructed
+ // before cp_ (which means it must be declared before cp_ in the class definition).
+ , cp_(new pj_caching_pool(),
[](pj_caching_pool* p) {
pj_caching_pool_destroy(p);
delete p;
diff --git a/src/turn/turn_transport.cpp b/src/turn/turn_transport.cpp
index 3ed4015..75525ff 100644
--- a/src/turn/turn_transport.cpp
+++ b/src/turn/turn_transport.cpp
@@ -147,7 +147,8 @@ TurnTransport::Impl::ioJob()
}
TurnTransport::TurnTransport(const TurnTransportParams& params, std::function<void(bool)>&& cb, const std::shared_ptr<Logger>& logger)
- : pimpl_ {new Impl(std::move(cb), logger)}
+ : pjInitLock_()
+ , pimpl_ {new Impl(std::move(cb), logger)}
{
auto server = params.server;
if (!server.getPort())
diff --git a/src/turn/turn_transport.h b/src/turn/turn_transport.h
index b872454..37626b8 100644
--- a/src/turn/turn_transport.h
+++ b/src/turn/turn_transport.h
@@ -17,6 +17,7 @@
#pragma once
#include "ip_utils.h"
+#include "pj_init_lock.h"
#include "turn_params.h"
#include <opendht/logger.h>
@@ -48,6 +49,7 @@ public:
private:
TurnTransport() = delete;
+ PjInitLock pjInitLock_;
class Impl;
std::unique_ptr<Impl> pimpl_;
};
--
GitLab