Use explicit storage for ActionReturnStatus::c_str (#34746)

* Use explicit storage for ActionReturnStatus::c_str Allocating some global storage for this is both not thread-safe and wastes RAM. Use the stack for the tiny amount of storage (relatively ... same as 4 integers) that a return status string needs. * Add unit tests and fix concatenation bug * Restyle * Undo header addition that seems wrong * Update comment to justify chosen storage size * Restyle * Fix unit test --------- Co-authored-by: Andrei Litvin <[email protected]>
project-chip · Aug 7, 2024 · 3a5e3db · 3a5e3db
1 parent 8bdc3c2
commit 3a5e3db
Show file tree

Hide file tree

Showing 6 changed files with 78 additions and 25 deletions.
diff --git a/src/app/data-model-provider/ActionReturnStatus.cpp b/src/app/data-model-provider/ActionReturnStatus.cpp
@@ -149,45 +149,42 @@ bool ActionReturnStatus::IsOutOfSpaceEncodingResponse() const
     return false;
 }
 
-const char * ActionReturnStatus::c_str() const
+const char * ActionReturnStatus::c_str(ActionReturnStatus::StringStorage & storage) const
 {
-
-    // Generally size should be sufficient.
-    // len("Status<123>, Code 255") == 21 (and then 22 for null terminator. We have slack.)
-    static chip::StringBuilder<32> sFormatBuffer;
-
     if (const CHIP_ERROR * err = std::get_if<CHIP_ERROR>(&mReturnStatus))
     {
 #if CHIP_CONFIG_ERROR_FORMAT_AS_STRING
         return err->Format(); // any length
 #else
-        sFormatBuffer.Reset().AddFormat("%" CHIP_ERROR_FORMAT, err->Format());
-        return sFormatBuffer.c_str();
+        storage.formatBuffer.Reset().AddFormat("%" CHIP_ERROR_FORMAT, err->Format());
+        return storage.formatBuffer.c_str();
 #endif
     }
 
     if (const ClusterStatusCode * status = std::get_if<ClusterStatusCode>(&mReturnStatus))
     {
+        storage.formatBuffer.Reset();
+
 #if CHIP_CONFIG_IM_STATUS_CODE_VERBOSE_FORMAT
-        sFormatBuffer.AddFormat("%s(%d)", Protocols::InteractionModel::StatusName(status->GetStatus()),
-                                static_cast<int>(status->GetStatus()));
+        storage.formatBuffer.AddFormat("%s(%d)", Protocols::InteractionModel::StatusName(status->GetStatus()),
+                                       static_cast<int>(status->GetStatus()));
 #else
         if (status->IsSuccess())
         {
-            sFormatBuffer.Add("Success");
+            storage.formatBuffer.Add("Success");
         }
         else
         {
-            sFormatBuffer.AddFormat("Status<%d>", static_cast<int>(status->GetStatus()));
+            storage.formatBuffer.AddFormat("Status<%d>", static_cast<int>(status->GetStatus()));
         }
 #endif
 
         chip::Optional<ClusterStatus> clusterCode = status->GetClusterSpecificCode();
         if (clusterCode.HasValue())
         {
-            sFormatBuffer.AddFormat(", Code %d", static_cast<int>(clusterCode.Value()));
+            storage.formatBuffer.AddFormat(", Code %d", static_cast<int>(clusterCode.Value()));
         }
-        return sFormatBuffer.c_str();
+        return storage.formatBuffer.c_str();
     }
 
     // all std::variant cases exhausted

diff --git a/src/app/data-model-provider/ActionReturnStatus.h b/src/app/data-model-provider/ActionReturnStatus.h
@@ -44,6 +44,21 @@ namespace DataModel {
 class ActionReturnStatus
 {
 public:
+    /// Provides storage for the c_str() call for the action status.
+    struct StringStorage
+    {
+        // Generally size should be sufficient.
+        // The longest status code from StatusCodeList is NO_UPSTREAM_SUBSCRIPTION(197)
+        // so we need space for one of:
+        //    "NO_UPSTREAM_SUBSCRIPTION(197)\0" = 30   // explicit verbose status code
+        //    "FAILURE(1), Code 255\0")                // cluster failure, verbose
+        //    "SUCCESS(0), Code 255\0")                // cluster success, verbose
+        //    "Status<197>, Code 255\0")               // Cluster failure, non-verbose
+        //
+        // CHIP_ERROR has its own (global/static!) storage
+        chip::StringBuilder<32> formatBuffer;
+    };
+
     ActionReturnStatus(CHIP_ERROR error) : mReturnStatus(error) {}
     ActionReturnStatus(Protocols::InteractionModel::Status status) :
         mReturnStatus(Protocols::InteractionModel::ClusterStatusCode(status))
@@ -81,12 +96,8 @@ class ActionReturnStatus
 
     /// Get the formatted string of this status.
     ///
-    /// NOTE: this is NOT thread safe in the general case, however the safety guarantees
-    ///       are similar to chip::ErrorStr which also assumes a static buffer.
-    ///
-    /// Use this in the chip main event loop (and since that is a single thread,
-    /// there should be no races)
-    const char * c_str() const;
+    /// May use `storage` for storying the actual underlying character string.
+    const char * c_str(StringStorage & storage) const;
 
 private:
     std::variant<CHIP_ERROR, Protocols::InteractionModel::ClusterStatusCode> mReturnStatus;

diff --git a/src/app/data-model-provider/StringBuilderAdapters.cpp b/src/app/data-model-provider/StringBuilderAdapters.cpp
@@ -23,7 +23,8 @@ template <>
 StatusWithSize ToString<chip::app::DataModel::ActionReturnStatus>(const chip::app::DataModel::ActionReturnStatus & status,
                                                                   pw::span<char> buffer)
 {
-    return pw::string::Format(buffer, "ActionReturnStatus<%s>", status.c_str());
+    chip::app::DataModel::ActionReturnStatus::StringStorage storage;
+    return pw::string::Format(buffer, "ActionReturnStatus<%s>", status.c_str(storage));
 }
 
 } // namespace pw
diff --git a/src/app/data-model-provider/tests/TestActionReturnStatus.cpp b/src/app/data-model-provider/tests/TestActionReturnStatus.cpp
@@ -111,3 +111,45 @@ TEST(TestActionReturnStatus, TestStatusCode)
     ASSERT_EQ(ActionReturnStatus(CHIP_IM_CLUSTER_STATUS(0x12)).GetStatusCode(), ClusterStatusCode::ClusterSpecificFailure(0x12));
     ASSERT_EQ(ActionReturnStatus(CHIP_IM_GLOBAL_STATUS(Timeout)).GetStatusCode(), ClusterStatusCode(Status::Timeout));
 }
+
+TEST(TestActionReturnStatus, TestCString)
+{
+    /// only tests the strings that we build and NOT the CHIP_ERROR ones which
+    /// are tested separately. for chip_error we just say it should not be empty.
+    ActionReturnStatus::StringStorage buffer;
+    ActionReturnStatus status(Status::Success);
+
+    // chip-error returns something non-empty
+    status = CHIP_ERROR_NOT_FOUND;
+    ASSERT_STRNE(status.c_str(buffer), "");
+
+    status = CHIP_NO_ERROR;
+    ASSERT_STRNE(status.c_str(buffer), "");
+
+    // the items below we control
+#if CHIP_CONFIG_IM_STATUS_CODE_VERBOSE_FORMAT
+    status = Status::Success;
+    ASSERT_STREQ(status.c_str(buffer), "SUCCESS(0)");
+
+    status = Status::UnsupportedCommand;
+    ASSERT_STREQ(status.c_str(buffer), "UNSUPPORTED_COMMAND(129)");
+
+    status = ClusterStatusCode::ClusterSpecificSuccess(31);
+    ASSERT_STREQ(status.c_str(buffer), "SUCCESS(0), Code 31");
+
+    status = ClusterStatusCode::ClusterSpecificFailure(32);
+    ASSERT_STREQ(status.c_str(buffer), "FAILURE(1), Code 32");
+#else
+    status = Status::Success;
+    ASSERT_STREQ(status.c_str(buffer), "Success");
+
+    status = Status::UnsupportedCommand;
+    ASSERT_STREQ(status.c_str(buffer), "Status<129>");
+
+    status = ClusterStatusCode::ClusterSpecificSuccess(31);
+    ASSERT_STREQ(status.c_str(buffer), "Success, Code 31");
+
+    status = ClusterStatusCode::ClusterSpecificFailure(32);
+    ASSERT_STREQ(status.c_str(buffer), "Status<1>, Code 32");
+#endif
+}
diff --git a/src/app/reporting/Read-Checked.cpp b/src/app/reporting/Read-Checked.cpp
@@ -82,12 +82,13 @@ ActionReturnStatus RetrieveClusterData(DataModel::Provider * dataModel, const Ac
 
     if (statusEmber != statusDm)
     {
-        StringBuilder<128> buffer;
+        ActionReturnStatus::StringStorage buffer;
+
         // Note log + chipDie instead of VerifyOrDie so that breakpoints (and usage of rr)
         // is easier to debug.
         ChipLogError(Test, "Different return codes between ember and DM");
-        ChipLogError(Test, "  Ember status: %s", statusEmber.c_str());
-        ChipLogError(Test, "  DM status:    %s", statusDm.c_str());
+        ChipLogError(Test, "  Ember status: %s", statusEmber.c_str(buffer));
+        ChipLogError(Test, "  DM status:    %s", statusDm.c_str(buffer));
 
         // For time-dependent data, we may have size differences here: one data fitting in buffer
         // while another not, resulting in different errors (success vs out of space).

diff --git a/src/app/reporting/Read-DataModel.cpp b/src/app/reporting/Read-DataModel.cpp
@@ -95,7 +95,8 @@ DataModel::ActionReturnStatus RetrieveClusterData(DataModel::Provider * dataMode
     // and will be sent to the client as well).
     if (!status.IsOutOfSpaceEncodingResponse())
     {
-        ChipLogError(DataManagement, "Failed to read attribute: %s", status.c_str());
+        DataModel::ActionReturnStatus::StringStorage storage;
+        ChipLogError(DataManagement, "Failed to read attribute: %s", status.c_str(storage));
     }
     return status;
 }