fix(shell): one-past-the-end write in argv array when a command has MAX_CMD_ARGS tokens - #112
Conversation
shell_execute declares `char *args[MAX_CMD_ARGS]` (16 slots) but shell_parse fills args[0..15] and then writes args[argc] = NULL. With 16+ whitespace-separated tokens argc reaches 16, so the NULL is written to args[16] -- one pointer past the end of the on-stack array, corrupting the adjacent stack slot. This is reachable from any keyboard input, e.g. a line of 16 words. Size the array MAX_CMD_ARGS + 1 to hold the argv-style NULL sentinel.
|
Small runtime note, I built this locally with the same setup as CI (Zig 0.16.0 + NASM, the USE_ZIG=1 path) and ran it in QEMU with AURI_TEST_MODE. With the fix, a 16-token command (echo + 15 args, so argc == 16) is handled cleanly and the shell keeps working: On the unfixed build the same command did not crash either. The write is always exactly one slot (argc is capped at MAX_CMD_ARGS, so it's always args[16] = NULL), and with the current GCC/Zig stack layout that slot happens to land on harmless padding. So to be straight about it: it's a real one-past-the-end write (undefined behaviour, and the tree builds with -fno-stack-protector), worth fixing as hardening, but I couldn't get it to actually crash on the current toolchain. So it's a latent defect, not a live crash. The fix stays a safe one-liner (args[MAX_CMD_ARGS + 1]). |
|
LGTM |
When a command line has 16 or more space-separated tokens, shell_parse writes one pointer past the end of the args array on the stack.
shell_execute declares the array with exactly MAX_CMD_ARGS slots:
shell_parse fills args[0..15] and then always writes the NULL sentinel at args[argc]:
So a line of 16 words makes argc reach 16 and args[16] = NULL writes 4 bytes past the array. It's reachable from normal keyboard input, and the tree builds with -fno-stack-protector. It's undefined behaviour; whether it does anything visible depends on the stack layout (see the note below, I could not get it to actually crash on the current toolchain).
Fix is to size the array for the NULL terminator, like a normal argv:
No behaviour change for valid input.