fix(modem): Fixed AT commands to copy strings to prevent overrides

Previously we used std::string_view, which pointed to the lower-layers
buffer which might have been reused for other asynchronous operations
before processing it, thus causing corruption of replies.

Closes #463

Author: david-cermak

components/esp_modem/include/cxx_include/esp_modem_command_library_utils.hpp

@@ -1,5 +1,5 @@
 /*
- * SPDX-FileCopyrightText: 2021-2022 Espressif Systems (Shanghai) CO LTD
+ * SPDX-FileCopyrightText: 2021-2024 Espressif Systems (Shanghai) CO LTD
  *
  * SPDX-License-Identifier: Apache-2.0
  */
@@ -25,18 +25,16 @@ command_result generic_command(CommandableIf *t, const std::string &command,
  * @brief Utility command to send command and return reply (after DCE says OK)
  * @param t Anything that is "command-able"
  * @param command Command to issue
- * @param output String to return
- * @param timeout_ms
+ * @param output String to return (could be either std::string& or std::string_view&)
  * @param timeout_ms Command timeout in ms
  * @return Generic command return type (OK, FAIL, TIMEOUT)
  */
-command_result generic_get_string(CommandableIf *t, const std::string &command, std::string &output, uint32_t timeout_ms = 500);
+template <typename T> command_result generic_get_string(CommandableIf *t, const std::string &command, T &output, uint32_t timeout_ms = 500);
 
 /**
  * @brief Generic command that passes on "OK" and fails on "ERROR"
  * @param t Anything that is "command-able"
  * @param command Command to issue
- * @param timeout
  * @param timeout_ms Command timeout in ms
  * @return Generic command return type (OK, FAIL, TIMEOUT)
  */

components/esp_modem/src/esp_modem_command_library.cpp

@@ -51,7 +51,35 @@ command_result generic_command(CommandableIf *t, const std::string &command,
     return generic_command(t, command, pass, fail, timeout_ms);
 }
 
-static command_result generic_get_string(CommandableIf *t, const std::string &command, std::string_view &output, uint32_t timeout_ms = 500)
+/*
+ * Purpose of this namespace is to provide different means of assigning the result to a string-like parameter.
+ * By default we assign strings, which comes with an allocation. Alternatively we could take `std::span`
+ * with user's buffer and directly copy the result, thus avoiding allocations (this is not used as of now)
+ */
+namespace str_copy {
+
+bool set(std::string &dest, std::string_view &src)
+{
+    dest = src;
+    return true;
+}
+
+/* This is an example of using std::span output in generic_get_string()
+bool set(std::span<char> &dest, std::string_view &src)
+{
+    if (dest.size() >= src.size()) {
+        std::copy(src.begin(), src.end(), dest.data());
+        dest = dest.subspan(0, src.size());
+        return true;
+    }
+    ESP_LOGE(TAG, "Cannot set result of size %d (to span of size %d)", dest.size(), src.size());
+    return false;
+}
+*/
+
+} // str_copy
+
+template <typename T> command_result generic_get_string(CommandableIf *t, const std::string &command, T &output, uint32_t timeout_ms)
 {
     ESP_LOGV(TAG, "%s", __func__ );
     return t->command(command, [&](uint8_t *data, size_t len) {
@@ -70,26 +98,16 @@ static command_result generic_get_string(CommandableIf *t, const std::string &co
             } else if (token.find("ERROR") != std::string::npos) {
                 return command_result::FAIL;
             } else if (token.size() > 2) {
-                output = token;
+                if (!str_copy::set(output, token)) {
+                    return command_result::FAIL;
+                }
             }
             response = response.substr(pos + 1);
         }
         return command_result::TIMEOUT;
     }, timeout_ms);
 }
 
-command_result generic_get_string(CommandableIf *t, const std::string &command, std::string &output, uint32_t timeout_ms)
-{
-    ESP_LOGV(TAG, "%s", __func__ );
-    std::string_view out;
-    auto ret = generic_get_string(t, command, out, timeout_ms);
-    if (ret == command_result::OK) {
-        output = out;
-    }
-    return ret;
-}
-
-
 command_result generic_command_common(CommandableIf *t, const std::string &command, uint32_t timeout_ms)
 {
     ESP_LOGV(TAG, "%s", __func__ );
@@ -153,7 +171,7 @@ command_result hang_up(CommandableIf *t)
 command_result get_battery_status(CommandableIf *t, int &voltage, int &bcs, int &bcl)
 {
     ESP_LOGV(TAG, "%s", __func__ );
-    std::string_view out;
+    std::string out;
     auto ret = generic_get_string(t, "AT+CBC\r", out);
     if (ret != command_result::OK) {
         return ret;
@@ -189,7 +207,7 @@ command_result get_battery_status(CommandableIf *t, int &voltage, int &bcs, int
 command_result get_battery_status_sim7xxx(CommandableIf *t, int &voltage, int &bcs, int &bcl)
 {
     ESP_LOGV(TAG, "%s", __func__ );
-    std::string_view out;
+    std::string out;
     auto ret = generic_get_string(t, "AT+CBC\r", out);
     if (ret != command_result::OK) {
         return ret;
@@ -224,7 +242,7 @@ command_result set_flow_control(CommandableIf *t, int dce_flow, int dte_flow)
 command_result get_operator_name(CommandableIf *t, std::string &operator_name, int &act)
 {
     ESP_LOGV(TAG, "%s", __func__ );
-    std::string_view out;
+    std::string out;
     auto ret = generic_get_string(t, "AT+COPS?\r", out, 75000);
     if (ret != command_result::OK) {
         return ret;
@@ -361,7 +379,7 @@ command_result set_cmux(CommandableIf *t)
 command_result read_pin(CommandableIf *t, bool &pin_ok)
 {
     ESP_LOGV(TAG, "%s", __func__ );
-    std::string_view out;
+    std::string out;
     auto ret = generic_get_string(t, "AT+CPIN?\r", out);
     if (ret != command_result::OK) {
         return ret;
@@ -413,7 +431,7 @@ command_result at_raw(CommandableIf *t, const std::string &cmd, std::string &out
 command_result get_signal_quality(CommandableIf *t, int &rssi, int &ber)
 {
     ESP_LOGV(TAG, "%s", __func__ );
-    std::string_view out;
+    std::string out;
     auto ret = generic_get_string(t, "AT+CSQ\r", out);
     if (ret != command_result::OK) {
         return ret;
@@ -451,7 +469,7 @@ command_result set_network_attachment_state(CommandableIf *t, int state)
 command_result get_network_attachment_state(CommandableIf *t, int &state)
 {
     ESP_LOGV(TAG, "%s", __func__ );
-    std::string_view out;
+    std::string out;
     auto ret = generic_get_string(t, "AT+CGATT?\r", out);
     if (ret != command_result::OK) {
         return ret;
@@ -478,7 +496,7 @@ command_result set_radio_state(CommandableIf *t, int state)
 command_result get_radio_state(CommandableIf *t, int &state)
 {
     ESP_LOGV(TAG, "%s", __func__ );
-    std::string_view out;
+    std::string out;
     auto ret = generic_get_string(t, "AT+CFUN?\r", out);
     if (ret != command_result::OK) {
         return ret;
@@ -543,7 +561,7 @@ command_result set_network_bands_sim76xx(CommandableIf *t, const std::string &mo
 command_result get_network_system_mode(CommandableIf *t, int &mode)
 {
     ESP_LOGV(TAG, "%s", __func__ );
-    std::string_view out;
+    std::string out;
     auto ret = generic_get_string(t, "AT+CNSMOD?\r", out);
     if (ret != command_result::OK) {
         return ret;
@@ -571,7 +589,7 @@ command_result set_gnss_power_mode(CommandableIf *t, int mode)
 command_result get_gnss_power_mode(CommandableIf *t, int &mode)
 {
     ESP_LOGV(TAG, "%s", __func__ );
-    std::string_view out;
+    std::string out;
     auto ret = generic_get_string(t, "AT+CGNSPWR?\r", out);
     if (ret != command_result::OK) {
         return ret;