From f585c33b7afacce8fa10c33c44bab37e935b68d4 Mon Sep 17 00:00:00 2001 From: Lefucjusz Date: Thu, 6 Jun 2024 13:19:57 +0200 Subject: [PATCH] [BH-2002] Fix crash when connected with broken USB cable Fixes of the issues that would cause device crash when connected with broken USB cable: * fixed memory leaks in VCOM init and deinit functions; * guarded accessing desktopWorker object in message handlers, as messages are asynchronous and might arrive after worker has already been destroyed. Not the cleanest solution, but does the trick --- harmony_changelog.md | 5 +- .../service-desktop/ServiceDesktop.cpp | 47 ++++++++++--------- .../service-desktop/WorkerDesktop.cpp | 6 +-- .../service-desktop/WorkerDesktop.hpp | 2 + .../service-desktop/ServiceDesktop.hpp | 30 ++++++------ third-party/usb_stack | 2 +- 6 files changed, 49 insertions(+), 43 deletions(-) diff --git a/harmony_changelog.md b/harmony_changelog.md index 4cb6dacc2a78ab8f90070bcef61a4c2238e3f244..893a6af685ce32c857dc5714bfa4e9a3e2bb5236 100644 --- a/harmony_changelog.md +++ b/harmony_changelog.md @@ -12,8 +12,9 @@ ### Changed / Improved * Changed the refresh rate of the progress bar screen -* Extended range of supported chargers. -* Changed order of options in Settings menu. +* Extended range of supported chargers +* Changed order of options in Settings menu +* Improved device stability with low quality USB cables ## [2.7.0 2024-05-20] diff --git a/module-services/service-desktop/ServiceDesktop.cpp b/module-services/service-desktop/ServiceDesktop.cpp index 4c7c06c9db9e8a57e50ad0daa42c71a31c5a0f01..b144962371e60212316636aef0e9710e1eff1955 100644 --- a/module-services/service-desktop/ServiceDesktop.cpp +++ b/module-services/service-desktop/ServiceDesktop.cpp @@ -1,4 +1,4 @@ -// Copyright (c) 2017-2023, Mudita Sp. z.o.o. All rights reserved. +// Copyright (c) 2017-2024, Mudita Sp. z.o.o. All rights reserved. // For licensing, see https://github.com/mudita/MuditaOS/LICENSE.md #include @@ -27,11 +27,9 @@ ServiceDesktop::ServiceDesktop(const std::filesystem::path &mtpRootPath) bus.channels.push_back(sys::BusChannel::USBNotifications); } -ServiceDesktop::~ServiceDesktop() -{ -} +ServiceDesktop::~ServiceDesktop() = default; -sys::ReturnCodes ServiceDesktop::InitHandler() +auto ServiceDesktop::InitHandler() -> sys::ReturnCodes { settings = std::make_unique(); settings->init(service::ServiceProxy(shared_from_this())); @@ -57,18 +55,19 @@ sys::ReturnCodes ServiceDesktop::InitHandler() return sys::ReturnCodes::Success; } -sys::ReturnCodes ServiceDesktop::DeinitHandler() +auto ServiceDesktop::DeinitHandler() -> sys::ReturnCodes { LOG_INFO("Deinitialized"); return usbWorkerDeinit(); } -sys::ReturnCodes ServiceDesktop::SwitchPowerModeHandler(const sys::ServicePowerMode /*mode*/) +auto ServiceDesktop::SwitchPowerModeHandler([[maybe_unused]] const sys::ServicePowerMode mode) -> sys::ReturnCodes { return sys::ReturnCodes::Success; } -sys::MessagePointer ServiceDesktop::DataReceivedHandler(sys::DataMessage * /*msg*/, sys::ResponseMessage *resp) +auto ServiceDesktop::DataReceivedHandler([[maybe_unused]] sys::DataMessage *msg, sys::ResponseMessage *resp) + -> sys::MessagePointer { auto response = std::make_shared(); if (resp == nullptr) { @@ -92,7 +91,7 @@ sys::MessagePointer ServiceDesktop::DataReceivedHandler(sys::DataMessage * /*msg return response; } -void ServiceDesktop::prepareSyncData() +auto ServiceDesktop::prepareSyncData() -> void { syncStatus.state = Sync::OperationState::Stopped; syncStatus.tempDir = purefs::dir::getTemporaryPath() / "sync"; @@ -102,7 +101,7 @@ auto ServiceDesktop::requestLogsFlush() -> void { int response = 0; auto ret = bus.sendUnicastSync( - std::make_shared(), service::name::evt_manager, DefaultLogFlushTimeoutInMs); + std::make_shared(), service::name::evt_manager, defaultLogFlushTimeoutMs); if (ret.first == sys::ReturnCodes::Success) { auto responseMsg = std::dynamic_pointer_cast(ret.second); @@ -145,7 +144,7 @@ auto ServiceDesktop::getNotificationEntries() const -> std::vector &uidsOfNotificationsToBeRemoved) +auto ServiceDesktop::removeNotificationEntries(const std::vector &uidsOfNotificationsToBeRemoved) -> void { outboxNotifications.removeNotificationEntries(uidsOfNotificationsToBeRemoved); } @@ -217,12 +216,12 @@ auto ServiceDesktop::usbWorkerDeinit() -> sys::ReturnCodes return sys::ReturnCodes::Success; } -void ServiceDesktop::restartConnectionActiveTimer() +auto ServiceDesktop::restartConnectionActiveTimer() -> void { connectionActiveTimer.restart(sdesktop::connectionActiveTimerDelayMs); } -void ServiceDesktop::checkChargingCondition() +auto ServiceDesktop::checkChargingCondition() -> void { if (Store::Battery::get().state == Store::Battery::State::Discharging) { usbWorkerDeinit(); @@ -243,7 +242,7 @@ auto ServiceDesktop::handle(db::NotificationMessage *msg) -> std::shared_ptr std::shared_ptr +auto ServiceDesktop::handle([[maybe_unused]] locks::UnlockedPhone *msg) -> std::shared_ptr { LOG_INFO("Phone unlocked."); usbSecurityModel->setPhoneUnlocked(); @@ -252,13 +251,15 @@ auto ServiceDesktop::handle(locks::UnlockedPhone * /*msg*/) -> std::shared_ptr(sys::phone_modes::Tethering::On), service::name::system_manager); isPlugEventUnhandled = false; - desktopWorker->notify(WorkerDesktop::Signal::unlockMTP); + if (desktopWorker != nullptr) { + desktopWorker->notify(WorkerDesktop::Signal::unlockMTP); + } } return sys::MessageNone{}; } -auto ServiceDesktop::handle(locks::LockedPhone * /*msg*/) -> std::shared_ptr +auto ServiceDesktop::handle([[maybe_unused]] locks::LockedPhone *msg) -> std::shared_ptr { LOG_INFO("Phone locked."); usbSecurityModel->setPhoneLocked(); @@ -300,7 +301,7 @@ auto ServiceDesktop::handle(sdesktop::developerMode::DeveloperModeRequest *msg) return sys::MessageNone{}; } -auto ServiceDesktop::handle(sdesktop::SyncMessage * /*msg*/) -> std::shared_ptr +auto ServiceDesktop::handle([[maybe_unused]] sdesktop::SyncMessage *msg) -> std::shared_ptr { syncStatus.state = Sync::OperationState::Running; syncStatus.completionCode = Sync::PrepareSyncPackage(this, syncStatus.tempDir); @@ -317,7 +318,7 @@ auto ServiceDesktop::handle(sdesktop::SyncMessage * /*msg*/) -> std::shared_ptr< return sys::MessageNone{}; } -auto ServiceDesktop::handle(sdesktop::FactoryMessage * /*msg*/) -> std::shared_ptr +auto ServiceDesktop::handle([[maybe_unused]] sdesktop::FactoryMessage *msg) -> std::shared_ptr { LOG_DEBUG("ServiceDesktop: FactoryMessage received"); sys::SystemManagerCommon::FactoryReset(this); @@ -325,7 +326,7 @@ auto ServiceDesktop::handle(sdesktop::FactoryMessage * /*msg*/) -> std::shared_p return sys::MessageNone{}; } -auto ServiceDesktop::handle(sdesktop::usb::USBConfigured *msg) -> std::shared_ptr +auto ServiceDesktop::handle([[maybe_unused]] sdesktop::usb::USBConfigured *msg) -> std::shared_ptr { isUsbConfigured = true; if (usbSecurityModel->isSecurityEnabled()) { @@ -336,13 +337,15 @@ auto ServiceDesktop::handle(sdesktop::usb::USBConfigured *msg) -> std::shared_pt bus.sendUnicast(std::make_shared(sys::phone_modes::Tethering::On), service::name::system_manager); isPlugEventUnhandled = false; - desktopWorker->notify(WorkerDesktop::Signal::unlockMTP); + if (desktopWorker != nullptr) { + desktopWorker->notify(WorkerDesktop::Signal::unlockMTP); + } } return sys::MessageNone{}; } -auto ServiceDesktop::handle(sdesktop::usb::USBDisconnected * /*msg*/) -> std::shared_ptr +auto ServiceDesktop::handle([[maybe_unused]] sdesktop::usb::USBDisconnected *msg) -> std::shared_ptr { LOG_INFO("USB disconnected"); if (usbSecurityModel->isSecurityEnabled()) { @@ -378,7 +381,7 @@ auto ServiceDesktop::getOnboardingState() const -> sdesktop::endpoints::Onboardi settings->getValue(settings::SystemProperties::onboardingDone, settings::SettingsScope::Global))); } -void ServiceDesktop::cleanFileSystemEndpointUndeliveredTransfers() +auto ServiceDesktop::cleanFileSystemEndpointUndeliveredTransfers() -> void { FileOperations::instance().cleanUpUndeliveredTransfers(); } diff --git a/module-services/service-desktop/WorkerDesktop.cpp b/module-services/service-desktop/WorkerDesktop.cpp index 11119cb660b270c99172e149fe23b28df1235f94..0b902c79345e397b4b0f6ca20de61205e1fc06f6 100644 --- a/module-services/service-desktop/WorkerDesktop.cpp +++ b/module-services/service-desktop/WorkerDesktop.cpp @@ -41,7 +41,7 @@ bool WorkerDesktop::init(std::list queues) receiveQueue = Worker::getQueueHandleByName(sdesktop::cdcReceiveQueueName); sdesktop::endpoints::sender::setSendQueueHandle(Worker::getQueueHandleByName(sdesktop::cdcSendQueueName)); - cpuSentinel = std::make_shared("WorkerDesktop", ownerService); + cpuSentinel = std::make_shared(cpuSentinelName, ownerService); auto sentinelRegistrationMsg = std::make_shared(cpuSentinel); ownerService->bus.sendUnicast(std::move(sentinelRegistrationMsg), service::name::system_manager); @@ -79,8 +79,8 @@ void WorkerDesktop::closeWorker() close(); cpuSentinel->ReleaseMinimumFrequency(); - auto sentinelRemoveMsg = std::make_shared("WorkerDesktop"); - auto result = ownerService->bus.sendUnicastSync(sentinelRemoveMsg, service::name::system_manager, 1000); + auto sentinelRemoveMsg = std::make_shared(cpuSentinelName); + auto result = ownerService->bus.sendUnicastSync(std::move(sentinelRemoveMsg), service::name::system_manager, 1000); if (result.first != sys::ReturnCodes::Success) { LOG_ERROR("Sentinel %s could not be removed!", cpuSentinel->GetName().c_str()); } diff --git a/module-services/service-desktop/WorkerDesktop.hpp b/module-services/service-desktop/WorkerDesktop.hpp index 8f5794e2543f89cd68e670f1e8c931b5cacaa7d4..728225d7e49ec3083ff17c2a2350008b2f2aa642 100644 --- a/module-services/service-desktop/WorkerDesktop.hpp +++ b/module-services/service-desktop/WorkerDesktop.hpp @@ -62,6 +62,8 @@ class WorkerDesktop : public sys::Worker sys::Service *ownerService = nullptr; sdesktop::endpoints::StateMachine parser; + static constexpr auto cpuSentinelName = "WorkerDesktop"; std::shared_ptr cpuSentinel; + std::function messageProcessedCallback; }; diff --git a/module-services/service-desktop/include/service-desktop/ServiceDesktop.hpp b/module-services/service-desktop/include/service-desktop/ServiceDesktop.hpp index 8af01c34cb7331ad656588d3707eb5613dc9ce8b..dc1477e91509ba2644946fb988ac365df930b689 100644 --- a/module-services/service-desktop/include/service-desktop/ServiceDesktop.hpp +++ b/module-services/service-desktop/include/service-desktop/ServiceDesktop.hpp @@ -1,4 +1,4 @@ -// Copyright (c) 2017-2023, Mudita Sp. z.o.o. All rights reserved. +// Copyright (c) 2017-2024, Mudita Sp. z.o.o. All rights reserved. // For licensing, see https://github.com/mudita/MuditaOS/LICENSE.md #pragma once @@ -71,17 +71,17 @@ class ServiceDesktop : public sys::Service std::unique_ptr desktopWorker; Sync::OperationStatus syncStatus; - sys::ReturnCodes InitHandler() override; - sys::ReturnCodes DeinitHandler() override; - sys::ReturnCodes SwitchPowerModeHandler(sys::ServicePowerMode mode) override; - sys::MessagePointer DataReceivedHandler(sys::DataMessage *msg, sys::ResponseMessage *resp) override; + auto InitHandler() -> sys::ReturnCodes override; + auto DeinitHandler() -> sys::ReturnCodes override; + auto SwitchPowerModeHandler(sys::ServicePowerMode mode) -> sys::ReturnCodes override; + auto DataReceivedHandler(sys::DataMessage *msg, sys::ResponseMessage *resp) -> sys::MessagePointer override; - void prepareSyncData(); - const Sync::OperationStatus getSyncStatus() + auto prepareSyncData() -> void; + auto getSyncStatus() const -> Sync::OperationStatus { return syncStatus; } - const sdesktop::USBSecurityModel *getSecurity() + auto getSecurity() const -> const sdesktop::USBSecurityModel * { return usbSecurityModel.get(); } @@ -93,7 +93,7 @@ class ServiceDesktop : public sys::Service auto getDeviceToken() -> std::string; auto getNotificationEntries() const -> std::vector; - void removeNotificationEntries(const std::vector &); + auto removeNotificationEntries(const std::vector &uidsOfNotificationsToBeRemoved) -> void; auto getMtpPath() const noexcept -> std::filesystem::path; auto getOnboardingState() const -> sdesktop::endpoints::OnboardingState; @@ -103,24 +103,24 @@ class ServiceDesktop : public sys::Service std::unique_ptr btMsgHandler; OutboxNotifications outboxNotifications; sys::TimerHandle connectionActiveTimer; - static constexpr unsigned int DefaultLogFlushTimeoutInMs = 1000U; + static constexpr unsigned int defaultLogFlushTimeoutMs = 1000U; bool initialized = false; bool isPlugEventUnhandled = false; bool isUsbConfigured = false; std::filesystem::path mtpRootPath; - void generateDeviceUniqueId(); + auto generateDeviceUniqueId() -> void; auto getDeviceUniqueId() const -> std::string; - void setDeviceUniqueId(const std::string &token); + auto setDeviceUniqueId(const std::string &token) -> void; auto usbWorkerInit() -> sys::ReturnCodes; auto usbWorkerDeinit() -> sys::ReturnCodes; - void restartConnectionActiveTimer(); + auto restartConnectionActiveTimer() -> void; - void checkChargingCondition(); + auto checkChargingCondition() -> void; - void cleanFileSystemEndpointUndeliveredTransfers(); + auto cleanFileSystemEndpointUndeliveredTransfers() -> void; template auto connectHandler() -> bool diff --git a/third-party/usb_stack b/third-party/usb_stack index 02d682190ee2cf1efaf60b9e6ea123960fd83e43..2818d5b6b8bb6e66d2e356e07f2fc5431c9df641 160000 --- a/third-party/usb_stack +++ b/third-party/usb_stack @@ -1 +1 @@ -Subproject commit 02d682190ee2cf1efaf60b9e6ea123960fd83e43 +Subproject commit 2818d5b6b8bb6e66d2e356e07f2fc5431c9df641