Repository navigation
interpreters/duktape: Handle SIGINT gracefully to prevent crash on restart - #3859
Sarthak-Pandey wants to merge 1 commit into
Conversation
|
this is a duktape limitation, should we send patch to duktape community instead? |
|
Hi @xiaoxiang781216, You are right that this is an upstream Duktape cmdline limitation. However:
Since we already maintain Would you prefer keeping this in our downstream patch, or do you suggest an alternative approach (e.g., an external wrapper)? |
ok, but please remove the unnecessary change from the patch file. |
bd70dcf to
fd86ea4
Compare
| +#endif | ||
| int c = fgetc(stdin); | ||
| + fputc(c, stdout); | ||
| + fflush(stdout); |
|
|
||
| for (;;) { | ||
| +#if defined(DUK_CMDLINE_SIGNAL) | ||
| + if (g_interrupted) { |
There was a problem hiding this comment.
DUK_USE_EXEC_TIMEOUT_CHECK
| } | ||
|
|
||
| +#if defined(DUK_CMDLINE_SIGNAL) | ||
| + if (g_interrupted) { |
There was a problem hiding this comment.
DUK_USE_EXEC_TIMEOUT_CHECK
| CFLAGS += -I$(DUKTAPE_UNPACK)/extras/print-alert | ||
| CFLAGS += -DDUK_CMDLINE_CONSOLE_SUPPORT | ||
| CFLAGS += -DDUK_CMDLINE_PRINTALERT_SUPPORT | ||
| CFLAGS += -DDUK_CMDLINE_SIGNAL |
There was a problem hiding this comment.
signal() is built only with CONFIG_ENABLE_ALL_SIGNALS (libs/libc/signal/Make.defs, sig_signal.c). With CONFIG_ENABLE_PARTIAL_SIGNALS (the default with DEFAULT_SMALL) or CONFIG_DISABLE_ALL_SIGNALS, set_sigint_handler() does not link.
Fix: add -DDUK_CMDLINE_SIGNAL only when CONFIG_ENABLE_ALL_SIGNALS=y.
fd86ea4 to
b64c921
Compare
|
@casaroli , Thanks for the review @casaroli! Good catch — I've updated the Makefile to conditionally enable DUK_CMDLINE_SIGNAL only when CONFIG_ENABLE_ALL_SIGNALS=y: makefile This ensures that on boards with CONFIG_ENABLE_PARTIAL_SIGNALS or CONFIG_DISABLE_ALL_SIGNALS, the signal handler code is excluded entirely and duk_cmdline_timeout_check() returns 0 (no-op fallback), so the build won't break. For the Testing comment (runtime log): You're right, I've updated the testing description. Here's the runtime verification: Built NuttX with Duktape for ESP32 (CONFIG_ENABLE_ALL_SIGNALS=y) Without this patch, step 5 crashes the board due to leaked heap state. |
b64c921 to
7475d5d
Compare
…start When Duktape is interrupted via Ctrl+C (SIGINT), the task was abruptly terminated by NuttX's default abnormal termination handler without destroying the Duktape heap. On flat-memory architectures such as ESP32, this leaked heap allocations and corrupted internal state, causing subsequent invocations of 'duk' to crash the board. This patch: 1. Enables DUK_CMDLINE_SIGNAL conditionally in Makefile when CONFIG_ENABLE_ALL_SIGNALS=y to register a SIGINT handler. 2. Defines DUK_USE_INTERRUPT_COUNTER and DUK_USE_EXEC_TIMEOUT_CHECK in duk_config.h to periodically inspect the interruption state. 3. Implements an async-signal-safe interrupt handler and execution timeout check in duk_cmdline.c that stops bytecode execution cleanly, unwinds the call stack, and safely invokes destroy_duktape_heap() before exiting. 4. Adds CMakeLists.txt to maintain build-system parity with CMake. Fixes apache#2855 [skip-cmake-parity] Signed-off-by: Sarthak Pandey <sarthak@email.com>
7475d5d to
a30407e
Compare
casaroli
left a comment
There was a problem hiding this comment.
The Testing section still says only "Verified patch applies cleanly against Duktape 2.5.0.", and issue #2855 is a runtime crash. The steps in your comment are not in the PR body.
Fix: put a runtime log in the Testing section: run a long script, press Ctrl+C, and run duk again.
Signed-off-by: Sarthak Pandey <sarthak@email.com> differs from your other commits, which use sarthakpandey5667@gmail.com, and GitHub links no account to it. CONTRIBUTING 1.5 asks for a valid address.
Fix: set the author and the Signed-off-by to your real address.
[skip-cmake-parity] is for a Make-only change (tools/checkpatch.sh:438), but this commit adds the CMakeLists.txt. The check passes without the marker.
Fix: remove [skip-cmake-parity].
| + | ||
| +duk_bool_t duk_cmdline_timeout_check(void *udata) { | ||
| + (void) udata; | ||
| + return g_interrupted; |
There was a problem hiding this comment.
set_sigint_handler() runs before create_duktape_heap(). A Ctrl+C in that time sets g_interrupted, and the first bytecode of the unprotected duk_eval_string() in create_duktape_heap() throws: *** FATAL ERROR: uncaught: 'execution timeout', then cmdline_fatal_handler() writes to address 0 and the board panics. On qemu-armv8a:nsh (TCG) with Ctrl+C 1 to 12 ms after the command, 39 of 42 runs panic; without this PR, 2 to 3 of 42.
Fix: return g_heap_ready && g_interrupted in duk_cmdline_timeout_check(), and set g_heap_ready = 1 in main() after create_duktape_heap().
Summary
Fixes #2855: Duktape crashes while you try to start it after interrupting it with ctrl+c.
When Duktape was interrupted via
Ctrl+C(SIGINT), the task was abruptly terminated by NuttX's default abnormal termination handler without destroying the Duktape heap. On flat-memory architectures such as ESP32, this leaked heap allocations and corrupted internal state, causing subsequent invocations ofdukto crash or panic.This PR:
DUK_CMDLINE_SIGNALinapps/interpreters/duktape/Makefile.DUK_USE_INTERRUPT_COUNTERandDUK_USE_EXEC_TIMEOUT_CHECKinduk_config.hso the Duktape bytecode executor periodically checks for execution interruption.duk_cmdline.cthat safely halts bytecode execution, unwinds the stack, and invokesdestroy_duktape_heap()to return all allocated memory before exiting.Impact
Testing