From 741d1660347641ad1fa594bed1b5c55e46e92500 Mon Sep 17 00:00:00 2001 From: David Cermak Date: Wed, 3 Jan 2024 20:59:14 +0100 Subject: [PATCH] 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 https://github.com/espressif/esp-protocols/issues/463 --- .../esp_modem_command_library_utils.hpp | 8 +-- .../src/esp_modem_command_library.cpp | 64 ++++++++++++------- 2 files changed, 44 insertions(+), 28 deletions(-) diff --git a/components/esp_modem/include/cxx_include/esp_modem_command_library_utils.hpp b/components/esp_modem/include/cxx_include/esp_modem_command_library_utils.hpp index b47c150d88..9cae9cdf9b 100644 --- a/components/esp_modem/include/cxx_include/esp_modem_command_library_utils.hpp +++ b/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 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) */ diff --git a/components/esp_modem/src/esp_modem_command_library.cpp b/components/esp_modem/src/esp_modem_command_library.cpp index 46e005db02..6f06bb7bd2 100644 --- a/components/esp_modem/src/esp_modem_command_library.cpp +++ b/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 &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 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,7 +98,9 @@ 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); } @@ -78,18 +108,6 @@ static command_result generic_get_string(CommandableIf *t, const std::string &co }, 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;