From 40be98920ad3338dbdc9c554dd1fea6fc8d7ca93 Mon Sep 17 00:00:00 2001 From: Michal Kopczan Date: Fri, 16 Jan 2026 12:15:57 +0100 Subject: [PATCH 1/2] fix: Prevent segfault by correctly handling dbus restarts in mender-update and mender-auth Ticket: MEN-9144 Changelog: Prevent segfault by correctly handling dbus restarts in mender-update and mender-auth. Previously when systemctl stop dbus.socket or systemctl stop dbus was called, it resulted in mender-update segfault. Signed-off-by: Michal Kopczan (cherry picked from commit 43dd88881bb5253b0ff7f3f25ccd991b300b4675) --- src/api/auth.hpp | 1 - src/api/auth/platform/dbus/auth.cpp | 3 +- src/common/platform/dbus.hpp | 44 +++- .../platform/dbus/asio_libdbus/dbus.cpp | 237 +++++++++++++----- 4 files changed, 210 insertions(+), 75 deletions(-) diff --git a/src/api/auth.hpp b/src/api/auth.hpp index a1d48c27a..2c4f997f1 100644 --- a/src/api/auth.hpp +++ b/src/api/auth.hpp @@ -125,7 +125,6 @@ class AuthenticatorDBus : public Authenticator { error::Error FetchJwtToken() override; dbus::DBusClient dbus_client_; - bool watching_token_signal_ {false}; }; #endif diff --git a/src/api/auth/platform/dbus/auth.cpp b/src/api/auth/platform/dbus/auth.cpp index b68834ea9..77bdcb3ed 100644 --- a/src/api/auth/platform/dbus/auth.cpp +++ b/src/api/auth/platform/dbus/auth.cpp @@ -36,7 +36,7 @@ namespace mlog = mender::common::log; namespace expected = mender::common::expected; error::Error AuthenticatorDBus::StartWatchingTokenSignal() { - if (watching_token_signal_) { + if (dbus_client_.WatchingSignal("io.mender.Authentication1", "JwtTokenStateChange")) { return error::NoError; } @@ -47,7 +47,6 @@ error::Error AuthenticatorDBus::StartWatchingTokenSignal() { HandleReceivedToken(ex_auth_dbus_data, NoTokenAction::Finish); }); - watching_token_signal_ = (err == error::NoError); return err; } diff --git a/src/common/platform/dbus.hpp b/src/common/platform/dbus.hpp index caf2b287f..5a635ab00 100644 --- a/src/common/platform/dbus.hpp +++ b/src/common/platform/dbus.hpp @@ -41,6 +41,35 @@ namespace mender { namespace common { namespace dbus { +struct SignalSpec { + std::string iface_; + std::string signal_; + + bool operator==(const SignalSpec &other) const { + return iface_ == other.iface_ && signal_ == other.signal_; + } + + size_t hash() const { + return std::hash {}(iface_ + signal_); + } +}; +} // namespace dbus +} // namespace common +} // namespace mender + +namespace std { +template <> +struct hash { + size_t operator()(const mender::common::dbus::SignalSpec &s) const { + return s.hash(); + } +}; +} // namespace std + +namespace mender { +namespace common { +namespace dbus { + namespace error = mender::common::error; namespace expected = mender::common::expected; namespace events = mender::common::events; @@ -70,10 +99,6 @@ using DBusCallReplyHandler = function; template using DBusSignalHandler = function; -// Might need something like -// struct {string iface; string signal;} -// in the future. -using SignalSpec = string; using StringPair = std::pair; using ExpectedStringPair = expected::expected; @@ -131,7 +156,7 @@ class DBusClient : public DBusPeer { template error::Error RegisterSignalHandler( const string &iface, const string &signal, DBusSignalHandler handler); - void UnregisterSignalHandler(const string &iface, const string &signal); + error::Error UnregisterSignalHandler(const string &iface, const string &signal); #ifdef MENDER_USE_ASIO_LIBDBUS // see DBusPeer's friends for some details @@ -139,6 +164,9 @@ class DBusClient : public DBusPeer { DBusConnection *connection, DBusMessage *message, void *data); #endif // MENDER_USE_ASIO_LIBDBUS + bool WatchingSignal(const string &iface, const string &signal) const; + void CleanUp(); + private: unordered_map> signal_handlers_string_; unordered_map> signal_handlers_string_pair_; @@ -146,7 +174,7 @@ class DBusClient : public DBusPeer { error::Error InitializeConnection() override; template - void AddSignalHandler(const string &spec, DBusSignalHandler handler); + void AddSignalHandler(const SignalSpec &spec, DBusSignalHandler handler); template optional> GetSignalHandler(const SignalSpec &spec); @@ -213,10 +241,10 @@ class DBusServer : public DBusPeer { DBusConnection *connection, DBusMessage *message, void *data); private: + error::Error RegisterDBusName(); + string service_name_; vector objects_; - - error::Error InitializeConnection() override; }; } // namespace dbus diff --git a/src/common/platform/dbus/asio_libdbus/dbus.cpp b/src/common/platform/dbus/asio_libdbus/dbus.cpp index 14e7d789a..a8268f892 100644 --- a/src/common/platform/dbus/asio_libdbus/dbus.cpp +++ b/src/common/platform/dbus/asio_libdbus/dbus.cpp @@ -132,7 +132,7 @@ error::Error DBusClient::CallMethod( const string &iface, const string &method, DBusCallReplyHandler handler) { - if (!dbus_conn_) { + if (!dbus_conn_ || !dbus_connection_get_is_connected(dbus_conn_.get())) { auto err = InitializeConnection(); if (err != error::NoError) { return err; @@ -209,7 +209,7 @@ static inline string GetSignalMatchRule(const string &iface, const string &signa template error::Error DBusClient::RegisterSignalHandler( const string &iface, const string &signal, DBusSignalHandler handler) { - if (!dbus_conn_) { + if (!dbus_conn_ || !dbus_connection_get_is_connected(dbus_conn_.get())) { auto err = InitializeConnection(); if (err != error::NoError) { return err; @@ -231,7 +231,7 @@ error::Error DBusClient::RegisterSignalHandler( dbus_error_free(&dbus_error); return err; } - AddSignalHandler(match_rule, handler); + AddSignalHandler({iface, signal}, handler); return error::NoError; } @@ -241,13 +241,29 @@ template error::Error DBusClient::RegisterSignalHandler( template error::Error DBusClient::RegisterSignalHandler( const string &iface, const string &signal, DBusSignalHandler handler); -void DBusClient::UnregisterSignalHandler(const string &iface, const string &signal) { +error::Error DBusClient::UnregisterSignalHandler(const string &iface, const string &signal) { // we use the match rule as a unique string for the given signal - const string spec = GetSignalMatchRule(iface, signal); + const string match_rule = GetSignalMatchRule(iface, signal); // should be in at most one set, but erase() is a noop if not found - signal_handlers_string_.erase(spec); - signal_handlers_string_pair_.erase(spec); + // Remove handlers even if subsequent dbus_bus_remove_match fails. This will result in + // simply nothing being called when the signal comes. Besides, if remove_match fails, it most + // probably means that the match was already not there, due to e.g. connection reestablishment. + signal_handlers_string_.erase({iface, signal}); + signal_handlers_string_pair_.erase({iface, signal}); + + DBusError dbus_error; + dbus_error_init(&dbus_error); + dbus_bus_remove_match(dbus_conn_.get(), match_rule.c_str(), &dbus_error); + if (dbus_error_is_set(&dbus_error)) { + auto err = MakeError( + ConnectionError, + string("Failed to unregister signal reception: ") + dbus_error.message); + dbus_error_free(&dbus_error); + return err; + } + + return error::NoError; } void HandleDispatch(DBusConnection *conn, DBusDispatchStatus status, void *data) { @@ -257,12 +273,30 @@ void HandleDispatch(DBusConnection *conn, DBusDispatchStatus status, void *data) // dbus_connection_dispatch() below can cause this to be called again. client->loop_.Post([conn]() { while (dbus_connection_get_dispatch_status(conn) == DBUS_DISPATCH_DATA_REMAINS) { - dbus_connection_dispatch(conn); + // The data from the socket should be read by now into internal libdbus buffer, + // but just in case call dbus_connection_read_write_dispatch instead of + // dbus_connection_dispatch. + // Timeout is set to 0 to make sure we read only data that is ready to be read, we + // don't wait for anything - waiting is done in RepeatedWaitFunctor. + dbus_connection_read_write_dispatch(conn, 0); } }); } } + +struct WatchState { + std::unique_ptr sd_; + bool finished_; + DBusClient *client_; + + WatchState(std::unique_ptr sd, DBusClient *client) : + sd_ {std::move(sd)}, + finished_ {false}, + client_ {client} { + } +}; + dbus_bool_t AddDBusWatch(DBusWatch *w, void *data) { // libdbus adds watches in two steps -- using AddDBusWatch() with a disabled // watch which should allocate all the necessary data (and can fail) @@ -274,10 +308,12 @@ dbus_bool_t AddDBusWatch(DBusWatch *w, void *data) { } DBusClient *client = static_cast(data); - unique_ptr sd { + std::unique_ptr sd { new asio::posix::stream_descriptor(DBusClient::GetAsioIoContext(client->loop_))}; + auto watch_state = make_shared(std::move(sd), client); boost::system::error_code ec; - sd->assign(dbus_watch_get_unix_fd(w), ec); + + watch_state->sd_->assign(dbus_watch_get_unix_fd(w), ec); if (ec) { log::Error("Failed to assign DBus FD to ASIO stream descriptor"); return FALSE; @@ -286,12 +322,12 @@ dbus_bool_t AddDBusWatch(DBusWatch *w, void *data) { class RepeatedWaitFunctor { public: RepeatedWaitFunctor( - asio::posix::stream_descriptor *sd, + std::shared_ptr watch_state, asio::posix::stream_descriptor::wait_type type, DBusWatch *watch, DBusClient *client, unsigned int flags) : - sd_ {sd}, + watch_state_ {watch_state}, type_ {type}, watch_ {watch}, client_ {client}, @@ -302,15 +338,30 @@ dbus_bool_t AddDBusWatch(DBusWatch *w, void *data) { if (ec == boost::asio::error::operation_aborted) { return; } + // RepeatedWaitFunctor is called once by asio on completion - i.e. when dbus removes a + // watch, we don't have anything more to handle, but this function is called once more. + // We mark it with finished_ flag to avoid trying to handle an invalid (removed) watch. + if (watch_state_->finished_) { + return; + } if (!dbus_watch_handle(watch_, flags_)) { log::Error("Failed to handle watch"); } HandleDispatch(client_->dbus_conn_.get(), DBUS_DISPATCH_DATA_REMAINS, client_); - sd_->async_wait(type_, *this); + // Check again if we're finished, as dbus_watch_handle may have caused it in the + // meantime. + if (watch_state_->finished_) { + return; + } + + // In case we return before calling async_wait with *this in all functors, + // the last instance of WatchState will be unreferenced, and thus sd will be deleted + // too. + watch_state_->sd_->async_wait(type_, *this); } private: - asio::posix::stream_descriptor *sd_; + std::shared_ptr watch_state_; asio::posix::stream_descriptor::wait_type type_; DBusWatch *watch_; DBusClient *client_; @@ -320,32 +371,40 @@ dbus_bool_t AddDBusWatch(DBusWatch *w, void *data) { unsigned int flags {dbus_watch_get_flags(w)}; if (flags & DBUS_WATCH_READABLE) { RepeatedWaitFunctor read_ftor { - sd.get(), asio::posix::stream_descriptor::wait_read, w, client, flags}; - sd->async_wait(asio::posix::stream_descriptor::wait_read, read_ftor); + watch_state, asio::posix::stream_descriptor::wait_read, w, client, flags}; + watch_state->sd_->async_wait(asio::posix::stream_descriptor::wait_read, read_ftor); } if (flags & DBUS_WATCH_WRITABLE) { RepeatedWaitFunctor write_ftor { - sd.get(), asio::posix::stream_descriptor::wait_write, w, client, flags}; - sd->async_wait(asio::posix::stream_descriptor::wait_write, write_ftor); + watch_state, asio::posix::stream_descriptor::wait_write, w, client, flags}; + watch_state->sd_->async_wait(asio::posix::stream_descriptor::wait_write, write_ftor); } // Always watch for errors. RepeatedWaitFunctor error_ftor { - sd.get(), asio::posix::stream_descriptor::wait_error, w, client, DBUS_WATCH_ERROR}; - sd->async_wait(asio::posix::stream_descriptor::wait_error, error_ftor); - - // Assign the stream_descriptor so that we have access to it in - // RemoveDBusWatch() and we can delete it. - dbus_watch_set_data(w, sd.release(), NULL); + watch_state, asio::posix::stream_descriptor::wait_error, w, client, DBUS_WATCH_ERROR}; + watch_state->sd_->async_wait(asio::posix::stream_descriptor::wait_error, error_ftor); + + // Assign the watch_state so that we have access to it in + // RemoveDBusWatch() and we can mark it as finished. + // watch_state will exist until the watch is marked as finished. So there's no + // danger of dereferencing a deleted pointer. + dbus_watch_set_data(w, watch_state.get(), NULL); return TRUE; } static void RemoveDBusWatch(DBusWatch *w, void *data) { - asio::posix::stream_descriptor *sd = - static_cast(dbus_watch_get_data(w)); + WatchState *watch_state = static_cast(dbus_watch_get_data(w)); dbus_watch_set_data(w, NULL, NULL); - if (sd != nullptr) { - sd->cancel(); - delete sd; + if (nullptr == watch_state) { + // The watch was already removed (marked as nullptr with dbus_watch_set_data above). + // dbus calls this function twice, so we need to check for it. + // Nothing to do. + return; + } + + watch_state->finished_ = true; + if (watch_state->sd_ != nullptr) { + watch_state->sd_->cancel(); } } @@ -545,14 +604,21 @@ DBusHandlerResult MsgFilter(DBusConnection *connection, DBusMessage *message, vo DBusClient *client = static_cast(data); - // we use the match rule as a unique string for the given signal - const string spec = - GetSignalMatchRule(dbus_message_get_interface(message), dbus_message_get_member(message)); + // first check special case: disconnected message. In this case, clean up the connection state + // to mark that we're not watching any signals, so whoever wants them to be watched, needs to + // register them again. + if (dbus_message_is_signal(message, DBUS_INTERFACE_LOCAL, "Disconnected")) { + client->CleanUp(); + return DBUS_HANDLER_RESULT_HANDLED; + } + + const string iface = dbus_message_get_interface(message); + const string signal = dbus_message_get_member(message); const string signature {dbus_message_get_signature(message)}; - auto opt_string_handler = client->GetSignalHandler(spec); - auto opt_string_pair_handler = client->GetSignalHandler(spec); + auto opt_string_handler = client->GetSignalHandler({iface, signal}); + auto opt_string_pair_handler = client->GetSignalHandler({iface, signal}); // either no match or only one match assert( @@ -584,6 +650,34 @@ DBusHandlerResult MsgFilter(DBusConnection *connection, DBusMessage *message, vo } } +bool DBusClient::WatchingSignal(const string &iface, const string &signal) const { + SignalSpec spec {iface, signal}; + + if (signal_handlers_string_.find(spec) != signal_handlers_string_.end()) { + return true; + } + + if (signal_handlers_string_pair_.find(spec) != signal_handlers_string_pair_.end()) { + return true; + } + + return false; +} + +void DBusClient::CleanUp() { + std::vector keys; + for (const auto &pair : signal_handlers_string_) { + keys.push_back(pair.first); + } + for (const auto &pair : signal_handlers_string_pair_) { + keys.push_back(pair.first); + } + + for (const auto &key : keys) { + UnregisterSignalHandler(key.iface_, key.signal_); + } +} + static inline string GetMethodSpec(const string &interface, const string &method) { return interface + "." + method; } @@ -643,33 +737,6 @@ optional> DBusObject::GetMethodHandler } } -error::Error DBusServer::InitializeConnection() { - auto err = DBusPeer::InitializeConnection(); - if (err != error::NoError) { - return err; - } - - DBusError dbus_error; - dbus_error_init(&dbus_error); - - // We could also do DBUS_NAME_FLAG_ALLOW_REPLACEMENT for cases where two of - // processes request the same name, but it would require handling of the - // NameLost signal and triggering termination. - if (dbus_bus_request_name( - dbus_conn_.get(), service_name_.c_str(), DBUS_NAME_FLAG_DO_NOT_QUEUE, &dbus_error) - == -1) { - dbus_conn_.reset(); - auto err = MakeError( - ConnectionError, - (string("Failed to register DBus name: ") + dbus_error.message + " [" + dbus_error.name - + "]")); - dbus_error_free(&dbus_error); - return err; - } - - return error::NoError; -} - DBusServer::~DBusServer() { if (!dbus_conn_) { // nothing to do without a DBus connection @@ -818,7 +885,7 @@ DBusHandlerResult HandleMethodCall(DBusConnection *connection, DBusMessage *mess static DBusObjectPathVTable dbus_vtable = {.message_function = HandleMethodCall}; error::Error DBusServer::AdvertiseObject(DBusObjectPtr obj) { - if (!dbus_conn_) { + if (!dbus_conn_ || !dbus_connection_get_is_connected(dbus_conn_.get())) { auto err = InitializeConnection(); if (err != error::NoError) { return err; @@ -838,21 +905,42 @@ error::Error DBusServer::AdvertiseObject(DBusObjectPtr obj) { dbus_error_free(&dbus_error); return err; } - objects_.push_back(obj); + + // We must register dbus name AFTER we already registered object paths. + // If our server was started due to a message directed at our name configured in a .service + // file, the messages aimed at the name will be delivered immediately after we successuly + // register the name. If we registered the name before registering object paths, the messages + // would be discared silently. + auto err = RegisterDBusName(); + if (err != error::NoError) { + return err; + } + + // If at this point we already have messages in the buffer (i.e. this service was started by + // dbus because a message was sent to our registered name), we must call HandleDispatch manually + // once to make sure that all messages are handled. We cannot rely on + // dbus_connection_set_dispatch_status_function from InitializeConnection to call it for us at + // this point. + HandleDispatch(dbus_conn_.get(), DBUS_DISPATCH_DATA_REMAINS, this); return error::NoError; } template error::Error DBusServer::EmitSignal( const string &path, const string &iface, const string &signal, SignalValueType value) { - if (!dbus_conn_) { + if (!dbus_conn_ || !dbus_connection_get_is_connected(dbus_conn_.get())) { auto err = InitializeConnection(); if (err != error::NoError) { return err; } } + auto err = RegisterDBusName(); + if (err != error::NoError) { + return err; + } + unique_ptr signal_msg { dbus_message_new_signal(path.c_str(), iface.c_str(), signal.c_str()), dbus_message_unref}; if (!signal_msg) { @@ -877,6 +965,27 @@ template error::Error DBusServer::EmitSignal( template error::Error DBusServer::EmitSignal( const string &path, const string &iface, const string &signal, StringPair value); +error::Error DBusServer::RegisterDBusName() { + // We could also do DBUS_NAME_FLAG_ALLOW_REPLACEMENT for cases where two of + // processes request the same name, but it would require handling of the + // NameLost signal and triggering termination. + DBusError dbus_error; + dbus_error_init(&dbus_error); + + if (dbus_bus_request_name( + dbus_conn_.get(), service_name_.c_str(), DBUS_NAME_FLAG_DO_NOT_QUEUE, &dbus_error) + == -1) { + dbus_conn_.reset(); + auto err = MakeError( + ConnectionError, + (string("Failed to register DBus name: ") + dbus_error.message + " [" + dbus_error.name + + "]")); + dbus_error_free(&dbus_error); + return err; + } + + return error::NoError; +} } // namespace dbus } // namespace common } // namespace mender From 5c4fff44b80dec2ae940979ab848dcf69f861ddd Mon Sep 17 00:00:00 2001 From: Michal Kopczan Date: Wed, 21 Jan 2026 14:00:51 +0100 Subject: [PATCH 2/2] fix: Schedule next deployment poll if current one failed early causing no handler to be called Ticket: MEN-9144 Changelog: Fix a hang when polling for deployment failed early causing no handler of API response to be called. Added handler call for this case, causing the deployment polling to continue. Signed-off-by: Michal Kopczan (cherry picked from commit 6aa4c776107ae5b038ced51ecdeee32a8f6b9ee5) --- src/mender-update/daemon/states.cpp | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/mender-update/daemon/states.cpp b/src/mender-update/daemon/states.cpp index 3ccc93e5d..4345a373d 100644 --- a/src/mender-update/daemon/states.cpp +++ b/src/mender-update/daemon/states.cpp @@ -282,6 +282,12 @@ void PollForDeploymentState::OnEnter(Context &ctx, sm::EventPoster & if (err != error::NoError) { log::Error("Error when trying to poll for deployment: " + err.String()); + // If we're here, no handler will be called, so we need to manually schedule the next + // deployment poll. + // Posting Failure correctly exits the PollForDeploymentState, but does not schedule the + // next poll. Thus, we need to also call HandlePollingError that adds a timer that will + // cause DeploymentPollingTriggered to be posted to the state machine after a defined time. + HandlePollingError(ctx, poster); poster.PostEvent(StateEvent::Failure); } }