From 526c201211528778603e83385fe712750fc7b3c2 Mon Sep 17 00:00:00 2001 From: Davis Mosenkovs Date: Thu, 2 Oct 2025 20:11:58 +0300 Subject: [PATCH] Refactor NotificationManager::Notification Refactor NotificationManager::Notification to use constructors. This reduces risk of coding errors (incl. buffer overflows) when creating NotificationManager::Notification and copying text to it. And fix a latent bug in ImmediateAlertService (include null terminator in the bytes copied and properly set size) using the constructor. --- src/components/ble/AlertNotificationClient.cpp | 5 +---- src/components/ble/AlertNotificationService.cpp | 5 +---- src/components/ble/DfuService.cpp | 4 +--- src/components/ble/FSService.cpp | 4 +--- src/components/ble/ImmediateAlertService.cpp | 3 +-- src/components/ble/NotificationManager.cpp | 17 +++++++++++++++++ src/components/ble/NotificationManager.h | 16 ++++++++++++++-- 7 files changed, 36 insertions(+), 18 deletions(-) diff --git a/src/components/ble/AlertNotificationClient.cpp b/src/components/ble/AlertNotificationClient.cpp index e3bc924230..c50643e451 100644 --- a/src/components/ble/AlertNotificationClient.cpp +++ b/src/components/ble/AlertNotificationClient.cpp @@ -156,10 +156,7 @@ void AlertNotificationClient::OnNotification(ble_gap_event* event) { size_t bufferSize = std::min(packetLen + stringTerminatorSize, maxBufferSize); auto messageSize = std::min(maxMessageSize, (bufferSize - headerSize)); - NotificationManager::Notification notif; - os_mbuf_copydata(event->notify_rx.om, headerSize, messageSize - 1, notif.message.data()); - notif.message[messageSize - 1] = '\0'; - notif.size = messageSize; + NotificationManager::Notification notif(event->notify_rx.om, headerSize, messageSize); notif.category = Pinetime::Controllers::NotificationManager::Categories::SimpleAlert; notificationManager.Push(std::move(notif)); diff --git a/src/components/ble/AlertNotificationService.cpp b/src/components/ble/AlertNotificationService.cpp index d9f28698bc..7aaa3cca19 100644 --- a/src/components/ble/AlertNotificationService.cpp +++ b/src/components/ble/AlertNotificationService.cpp @@ -61,11 +61,8 @@ int AlertNotificationService::OnAlert(struct ble_gatt_access_ctxt* ctxt) { auto messageSize = std::min(maxMessageSize, (bufferSize - headerSize)); Categories category; - NotificationManager::Notification notif; - os_mbuf_copydata(ctxt->om, headerSize, messageSize - 1, notif.message.data()); + NotificationManager::Notification notif(ctxt->om, headerSize, messageSize); os_mbuf_copydata(ctxt->om, 0, 1, &category); - notif.message[messageSize - 1] = '\0'; - notif.size = messageSize; // TODO convert all ANS categories to NotificationController categories switch (category) { diff --git a/src/components/ble/DfuService.cpp b/src/components/ble/DfuService.cpp index ad9c99e9e0..a6affc854a 100644 --- a/src/components/ble/DfuService.cpp +++ b/src/components/ble/DfuService.cpp @@ -82,9 +82,7 @@ void DfuService::Init() { int DfuService::OnServiceData(uint16_t connectionHandle, uint16_t attributeHandle, ble_gatt_access_ctxt* context) { #ifndef PINETIME_IS_RECOVERY if (systemTask.GetSettings().GetDfuAndFsMode() == Pinetime::Controllers::Settings::DfuAndFsMode::Disabled) { - Pinetime::Controllers::NotificationManager::Notification notif; - memcpy(notif.message.data(), denyAlert, denyAlertLength); - notif.size = denyAlertLength; + Pinetime::Controllers::NotificationManager::Notification notif(denyAlert, denyAlertLength); notif.category = Pinetime::Controllers::NotificationManager::Categories::SimpleAlert; systemTask.GetNotificationManager().Push(std::move(notif)); systemTask.PushMessage(Pinetime::System::Messages::OnNewNotification); diff --git a/src/components/ble/FSService.cpp b/src/components/ble/FSService.cpp index 721ed297c5..82af3cd038 100644 --- a/src/components/ble/FSService.cpp +++ b/src/components/ble/FSService.cpp @@ -53,9 +53,7 @@ void FSService::Init() { int FSService::OnFSServiceRequested(uint16_t connectionHandle, uint16_t attributeHandle, ble_gatt_access_ctxt* context) { #ifndef PINETIME_IS_RECOVERY if (systemTask.GetSettings().GetDfuAndFsMode() == Pinetime::Controllers::Settings::DfuAndFsMode::Disabled) { - Pinetime::Controllers::NotificationManager::Notification notif; - memcpy(notif.message.data(), denyAlert, denyAlertLength); - notif.size = denyAlertLength; + Pinetime::Controllers::NotificationManager::Notification notif(denyAlert, denyAlertLength); notif.category = Pinetime::Controllers::NotificationManager::Categories::SimpleAlert; systemTask.GetNotificationManager().Push(std::move(notif)); systemTask.PushMessage(Pinetime::System::Messages::OnNewNotification); diff --git a/src/components/ble/ImmediateAlertService.cpp b/src/components/ble/ImmediateAlertService.cpp index b9de615aa0..19f32465f5 100644 --- a/src/components/ble/ImmediateAlertService.cpp +++ b/src/components/ble/ImmediateAlertService.cpp @@ -62,8 +62,7 @@ int ImmediateAlertService::OnAlertLevelChanged(uint16_t attributeHandle, ble_gat auto alertLevel = static_cast(context->om->om_data[0]); auto* alertString = ToString(alertLevel); - NotificationManager::Notification notif; - std::memcpy(notif.message.data(), alertString, strlen(alertString)); + NotificationManager::Notification notif(alertString, strlen(alertString) + 1); notif.category = Pinetime::Controllers::NotificationManager::Categories::SimpleAlert; notificationManager.Push(std::move(notif)); diff --git a/src/components/ble/NotificationManager.cpp b/src/components/ble/NotificationManager.cpp index 223f6221af..f73d726264 100644 --- a/src/components/ble/NotificationManager.cpp +++ b/src/components/ble/NotificationManager.cpp @@ -132,6 +132,23 @@ size_t NotificationManager::NbNotifications() const { return size; } +NotificationManager::Notification::Notification() { +} + +NotificationManager::Notification::Notification(const char* message, uint8_t size) { + uint8_t effectiveSize = std::min(std::max(size, (uint8_t) 1), NotificationManager::MessageSize); + memcpy(this->message.data(), message, effectiveSize - 1); + this->message[effectiveSize - 1] = '\0'; + this->size = effectiveSize; +} + +NotificationManager::Notification::Notification(const struct os_mbuf* om, int off, uint8_t size) { + uint8_t effectiveSize = std::min(std::max(size, (uint8_t) 1), NotificationManager::MessageSize); + os_mbuf_copydata(om, off, effectiveSize - 1, this->message.data()); + this->message[effectiveSize - 1] = '\0'; + this->size = effectiveSize; +} + const char* NotificationManager::Notification::Message() const { const char* itField = std::find(message.begin(), message.begin() + size - 1, '\0'); if (itField != message.begin() + size - 1) { diff --git a/src/components/ble/NotificationManager.h b/src/components/ble/NotificationManager.h index 57a9c715d2..cc90fb5018 100644 --- a/src/components/ble/NotificationManager.h +++ b/src/components/ble/NotificationManager.h @@ -5,6 +5,12 @@ #include #include +#define min // workaround: nimble's min/max macros conflict with libstdc++ +#define max +#include +#undef max +#undef min + namespace Pinetime { namespace Controllers { class NotificationManager { @@ -25,17 +31,23 @@ namespace Pinetime { static constexpr uint8_t MessageSize {100}; struct Notification { + public: using Id = uint8_t; using Idx = uint8_t; - std::array message{}; - uint8_t size; Categories category = Categories::Unknown; Id id = 0; bool valid = false; + Notification(); + Notification(const char* message, uint8_t size); + Notification(const struct os_mbuf* om, int off, uint8_t size); const char* Message() const; const char* Title() const; + + private: + std::array message {}; + uint8_t size; }; void Push(Notification&& notif);