Skip to content

Commit

Permalink
mavsdk_server: fix crash on stop/destruction (mavlink#2417)
Browse files Browse the repository at this point in the history
This fixes a segfault which happens when the mavsdk_server is stopped
and destroyed before every having discovered any autopilot.

What happened is that:
1. ConnectionInitiator would be cancelled.
2. ConnectionInitiator would be destructed.
3. And only now the connect thread would wake up from sleeping and read
   the _should_exit flag which at this point has been destroyed.
   At this point the connect function would try to access
   Mavsdk::systems() which is also destructed by that point and cause a
   segfault.

The fix is to wrap the ConnectionInitiator class in a shared ptr and
keep that one alive in the connect function to avoid destruction until
it has returned.
  • Loading branch information
julianoes authored and mjbcopland committed Feb 19, 2025
1 parent d7806a7 commit a80dea9
Show file tree
Hide file tree
Showing 2 changed files with 12 additions and 10 deletions.
15 changes: 8 additions & 7 deletions src/mavsdk_server/src/connection_initiator.h
Original file line number Diff line number Diff line change
@@ -1,25 +1,26 @@
#pragma once

#include <atomic>
#include <chrono>
#include <future>
#include <memory>
#include <mutex>
#include <thread>
#include <string>

#include "connection_result.h"
#include "log.h"
#include "mavsdk.h"

namespace mavsdk {
namespace mavsdk_server {

template<typename Mavsdk> class ConnectionInitiator {
class ConnectionInitiator : public std::enable_shared_from_this<ConnectionInitiator> {
public:
ConnectionInitiator() {}
~ConnectionInitiator() {}

bool connect(Mavsdk& mavsdk, const std::string& connection_url)
bool connect(mavsdk::Mavsdk& mavsdk, const std::string& connection_url)
{
LogInfo() << "Waiting to discover system on " << connection_url << "...";
// Keep this class alive while this function is running.
auto self = shared_from_this();

if (!add_any_connection(mavsdk, connection_url)) {
return false;
Expand Down Expand Up @@ -49,7 +50,7 @@ template<typename Mavsdk> class ConnectionInitiator {
void cancel() { _should_exit = true; }

private:
bool add_any_connection(Mavsdk& mavsdk, const std::string& connection_url)
bool add_any_connection(mavsdk::Mavsdk& mavsdk, const std::string& connection_url)
{
mavsdk::ConnectionResult connection_result = mavsdk.add_any_connection(connection_url);

Expand Down
7 changes: 4 additions & 3 deletions src/mavsdk_server/src/mavsdk_server.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,8 @@ class MavsdkServer::Impl {

bool connect(const std::string& connection_url)
{
return _connection_initiator.connect(_mavsdk, connection_url);
_connection_initiator = std::make_shared<ConnectionInitiator>();
return _connection_initiator->connect(_mavsdk, connection_url);
}

int startGrpcServer(const int port)
Expand All @@ -31,7 +32,7 @@ class MavsdkServer::Impl {

void stop()
{
_connection_initiator.cancel();
_connection_initiator->cancel();

if (_server != nullptr) {
_server->stop();
Expand All @@ -47,7 +48,7 @@ class MavsdkServer::Impl {

private:
mavsdk::Mavsdk _mavsdk;
ConnectionInitiator<mavsdk::Mavsdk> _connection_initiator;
std::shared_ptr<ConnectionInitiator> _connection_initiator;
std::unique_ptr<GrpcServer> _server;
int _grpc_port;
};
Expand Down

0 comments on commit a80dea9

Please sign in to comment.