From c1d5717013fe874d50084de806ecdf6ff5f60e9d Mon Sep 17 00:00:00 2001 From: Ivan Grokhotkov Date: Tue, 30 Aug 2022 18:45:16 +0200 Subject: [PATCH 1/2] console: fix a crash when initializing usb_serial_jtag console MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The crash occurred when calling setvbuf(stdin,...) with stdin==NULL. This happened because esp_console_repl_task started running before its args->uart_channel was initialized; then esp_console_repl_task went into the code path 'uart_channel != CONFIG_ESP_CONSOLE_UART_NUM', and tried to 'fopen("/dev/uart/0");` Since the UART VFS is not registered when ESP_CONSOLE_USB_SERIAL_JTAG option is enabled, fopen failed and 'stdin' was NULL. Fix by moving the initialization of repl task arguments before the start of the task, same as it is done for the usb_cdcacm case. The crash started happening after the commit 287ab7566b9. I haven’t verified this, but I guess the reason why it wasn’t happening before was that xTaskCreate was not correctly yielding to the newly created higher-priority 'repl' task, therefore the code which was setting the repl task arguments after xTaskCreate had time to execute. It should be noted that the 'uart_channel' argument is a bit hacky, in the first place. The code should be refactored to pass a callback function to the repl task, and let this callback initialize stdin and stdout based on the chosen console channel. Then esp_console_repl_task does not require assumptions about the specific interface used. Closes https://github.com/espressif/esp-idf/issues/9662 --- components/console/esp_console_repl.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/components/console/esp_console_repl.c b/components/console/esp_console_repl.c index c99e1c4b69..d1baaccfbb 100644 --- a/components/console/esp_console_repl.c +++ b/components/console/esp_console_repl.c @@ -174,6 +174,11 @@ esp_err_t esp_console_new_repl_usb_serial_jtag(const esp_console_dev_usb_serial_ // setup prompt esp_console_setup_prompt(repl_config->prompt, &usb_serial_jtag_repl->repl_com); + /* Fill the structure here as it will be used directly by the created task. */ + usb_serial_jtag_repl->uart_channel = CONFIG_ESP_CONSOLE_UART_NUM; + usb_serial_jtag_repl->repl_com.state = CONSOLE_REPL_STATE_INIT; + usb_serial_jtag_repl->repl_com.repl_core.del = esp_console_repl_usb_serial_jtag_delete; + /* spawn a single thread to run REPL */ if (xTaskCreate(esp_console_repl_task, "console_repl", repl_config->task_stack_size, &usb_serial_jtag_repl->repl_com, repl_config->task_priority, &usb_serial_jtag_repl->repl_com.task_hdl) != pdTRUE) { @@ -181,9 +186,6 @@ esp_err_t esp_console_new_repl_usb_serial_jtag(const esp_console_dev_usb_serial_ goto _exit; } - usb_serial_jtag_repl->uart_channel = CONFIG_ESP_CONSOLE_UART_NUM; - usb_serial_jtag_repl->repl_com.state = CONSOLE_REPL_STATE_INIT; - usb_serial_jtag_repl->repl_com.repl_core.del = esp_console_repl_usb_serial_jtag_delete; *ret_repl = &usb_serial_jtag_repl->repl_com.repl_core; return ESP_OK; _exit: From 5b88c6b14290924c75b412e38ecaeb0b76990486 Mon Sep 17 00:00:00 2001 From: Ivan Grokhotkov Date: Tue, 30 Aug 2022 19:37:15 +0200 Subject: [PATCH 2/2] console: pass esp_console_repl_universal_t pointer to the repl task For usb_serial_jtag REPL only, xTaskCreate was passing a pointer to esp_console_repl_com_t, while esp_console_repl_task was expecting a pointer to esp_console_repl_universal_t. The way the two structures are defined, this makes no difference, and the pointer values are the same. Still, this could potentially break in the future. (I am not sure what is the distinction between repl_com (common?) and repl_universal; it seems that `int uart_channel` could just as well be part of esp_console_repl_com_t; alternatively, as suggested in the previous commit, this structure could contain a callback function pointer, which would allow `esp_console_new_repl_*` functions to specify how stdin/stdout should be initialized by the REPL task.) --- components/console/esp_console_repl.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/components/console/esp_console_repl.c b/components/console/esp_console_repl.c index d1baaccfbb..cb60df5452 100644 --- a/components/console/esp_console_repl.c +++ b/components/console/esp_console_repl.c @@ -181,7 +181,7 @@ esp_err_t esp_console_new_repl_usb_serial_jtag(const esp_console_dev_usb_serial_ /* spawn a single thread to run REPL */ if (xTaskCreate(esp_console_repl_task, "console_repl", repl_config->task_stack_size, - &usb_serial_jtag_repl->repl_com, repl_config->task_priority, &usb_serial_jtag_repl->repl_com.task_hdl) != pdTRUE) { + usb_serial_jtag_repl, repl_config->task_priority, &usb_serial_jtag_repl->repl_com.task_hdl) != pdTRUE) { ret = ESP_FAIL; goto _exit; }