From 740dd2ea2fb24b2a5d89364daa2301b76c540c18 Mon Sep 17 00:00:00 2001 From: Jakub Rzeszutko Date: Tue, 21 Jan 2025 12:36:52 +0100 Subject: [PATCH] shell: fix unsafe API calls and add configurable autoflush behavior Fixes an issue where the shell API could block indefinitely when called from threads other than the shell's processing thread, especially when the transport (e.g. USB CDC ACM) was unavailable or inactive. Replaced `k_mutex_lock` calls with an indefinite timeout (`K_FOREVER`) by using a fixed timeout (`K_MSEC(SHELL_TX_MTX_TIMEOUT_MS)`) in shell API functions to prevent indefinite blocking. Link: https://github.com/zephyrproject-rtos/zephyr/issues/84274 Signed-off-by: Jakub Rzeszutko (cherry picked from commit b0a0febe589662bddb4bfe40c0faaf5795812fb1) --- subsys/shell/shell.c | 26 +++++++++++++++++++------- 1 file changed, 19 insertions(+), 7 deletions(-) diff --git a/subsys/shell/shell.c b/subsys/shell/shell.c index 0032fed7f789..8ce5f9c3afb8 100644 --- a/subsys/shell/shell.c +++ b/subsys/shell/shell.c @@ -31,6 +31,7 @@ "WARNING: A print request was detected on not active shell backend.\n" #define SHELL_MSG_TOO_MANY_ARGS "Too many arguments in the command.\n" #define SHELL_INIT_OPTION_PRINTER (NULL) +#define SHELL_TX_MTX_TIMEOUT_MS 50 #define SHELL_THREAD_PRIORITY \ COND_CODE_1(CONFIG_SHELL_THREAD_PRIORITY_OVERRIDE, \ @@ -1350,7 +1351,9 @@ void shell_thread(void *shell_handle, void *arg_log_backend, K_FOREVER); if (err != 0) { - k_mutex_lock(&sh->ctx->wr_mtx, K_FOREVER); + if (k_mutex_lock(&sh->ctx->wr_mtx, K_MSEC(SHELL_TX_MTX_TIMEOUT_MS)) != 0) { + return; + } z_shell_fprintf(sh, SHELL_ERROR, "Shell thread error: %d", err); k_mutex_unlock(&sh->ctx->wr_mtx); @@ -1441,7 +1444,9 @@ int shell_start(const struct shell *sh) z_shell_log_backend_enable(sh->log_backend, (void *)sh, sh->ctx->log_level); } - k_mutex_lock(&sh->ctx->wr_mtx, K_FOREVER); + if (k_mutex_lock(&sh->ctx->wr_mtx, K_MSEC(SHELL_TX_MTX_TIMEOUT_MS)) != 0) { + return -EBUSY; + } if (IS_ENABLED(CONFIG_SHELL_VT100_COLORS)) { z_shell_vt100_color_set(sh, SHELL_NORMAL); @@ -1543,7 +1548,10 @@ void shell_vfprintf(const struct shell *sh, enum shell_vt100_color color, return; } - k_mutex_lock(&sh->ctx->wr_mtx, K_FOREVER); + if (k_mutex_lock(&sh->ctx->wr_mtx, K_MSEC(SHELL_TX_MTX_TIMEOUT_MS)) != 0) { + return; + } + if (!z_flag_cmd_ctx_get(sh) && !sh->ctx->bypass && z_flag_use_vt100_get(sh)) { z_shell_cmd_line_erase(sh); } @@ -1552,6 +1560,7 @@ void shell_vfprintf(const struct shell *sh, enum shell_vt100_color color, z_shell_print_prompt_and_cmd(sh); } z_transport_buffer_flush(sh); + k_mutex_unlock(&sh->ctx->wr_mtx); } @@ -1677,10 +1686,9 @@ int shell_prompt_change(const struct shell *sh, const char *prompt) return -EINVAL; } - static const size_t mtx_timeout_ms = 20; size_t prompt_length = z_shell_strlen(prompt); - if (k_mutex_lock(&sh->ctx->wr_mtx, K_MSEC(mtx_timeout_ms))) { + if (k_mutex_lock(&sh->ctx->wr_mtx, K_MSEC(SHELL_TX_MTX_TIMEOUT_MS)) != 0) { return -EBUSY; } @@ -1703,7 +1711,9 @@ int shell_prompt_change(const struct shell *sh, const char *prompt) void shell_help(const struct shell *sh) { - k_mutex_lock(&sh->ctx->wr_mtx, K_FOREVER); + if (k_mutex_lock(&sh->ctx->wr_mtx, K_MSEC(SHELL_TX_MTX_TIMEOUT_MS)) != 0) { + return; + } shell_internal_help_print(sh); k_mutex_unlock(&sh->ctx->wr_mtx); } @@ -1736,7 +1746,9 @@ int shell_execute_cmd(const struct shell *sh, const char *cmd) sh->ctx->cmd_buff_len = cmd_len; sh->ctx->cmd_buff_pos = cmd_len; - k_mutex_lock(&sh->ctx->wr_mtx, K_FOREVER); + if (k_mutex_lock(&sh->ctx->wr_mtx, K_MSEC(SHELL_TX_MTX_TIMEOUT_MS)) != 0) { + return -ENOEXEC; + } ret_val = execute(sh); k_mutex_unlock(&sh->ctx->wr_mtx);