Skip to content

Commit

Permalink
shell: fix unsafe API calls and add configurable autoflush behavior
Browse files Browse the repository at this point in the history
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: #84274

Signed-off-by: Jakub Rzeszutko <[email protected]>
(cherry picked from commit b0a0feb)
  • Loading branch information
jakub-uC authored and dkalowsk committed Jan 31, 2025
1 parent 98a1a15 commit 4a1ffd8
Showing 1 changed file with 19 additions and 7 deletions.
26 changes: 19 additions & 7 deletions subsys/shell/shell.c
Original file line number Diff line number Diff line change
Expand Up @@ -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, \
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
}
Expand All @@ -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);
}

Expand Down Expand Up @@ -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;
}

Expand All @@ -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);
}
Expand Down Expand Up @@ -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);

Expand Down

0 comments on commit 4a1ffd8

Please sign in to comment.