kargs: Fix crash on quoted values in /proc/cmdline#3583
kargs: Fix crash on quoted values in /proc/cmdline#3583jmarrero wants to merge 1 commit intoostreedev:mainfrom
Conversation
|
Skipping CI for Draft Pull Request. |
There was a problem hiding this comment.
Code Review
This pull request addresses a crash in ostree_kernel_args_append_proc_cmdline by replacing a naive space-based split with the quote-aware split_kernel_args function. The split_kernel_args function was also modified to handle unterminated quotes gracefully by treating the remainder of the string as a single token. Comprehensive unit and integration tests were added to verify these changes. A review comment suggests further improving split_kernel_args by ensuring empty strings are not added to the argument list, which could occur if the input ends with a space.
| if (quoted) | ||
| g_debug ("Missing terminating quote in '%s', treating remainder as single token.\n", str); | ||
| g_ptr_array_add (strv, g_strndup (start, str + len - start)); |
There was a problem hiding this comment.
To avoid adding an empty string to the arguments list (which can happen if the input string ends with a space or is empty), it's better to check if the remaining slice is non-empty before adding it. This prevents an empty key from being added to the OstreeKernelArgs structure, which could lead to malformed output when generating the command line string back.
if (str + len > start)
{
if (quoted)
g_debug ("Missing terminating quote in '%s', treating remainder as single token.\n", str);
g_ptr_array_add (strv, g_strndup (start, str + len - start));
}ostree_kernel_args_append_proc_cmdline() crashes with SIGABRT when /proc/cmdline contains quoted kernel arguments with spaces (e.g. "testparam=value with spaces"). This was reported by Arch Linux maintainers when building ostree 2026.1 and reproduced on 2025.7. Root cause: commit abc7d5b ("kargs: parse spaces in kargs input and keep quotes", March 2024) added split_kernel_args(), a quote-aware string splitter, and updated ostree_kernel_args_append() and three other functions to use it. However, it did not update ostree_kernel_args_append_proc_cmdline(), which still used a naive g_strsplit(" ") to tokenize /proc/cmdline. When the cmdline contained quoted values with spaces, g_strsplit broke them into fragments with unterminated quotes. These fragments were then passed to ostree_kernel_args_append() -> split_kernel_args(), which hit a g_assert_false(quoted) and aborted. The bug was latent since v2024.5 because the test-admin-deploy-karg.sh and test-admin-instutil-set-kargs.sh tests read the real /proc/cmdline from the build host. CI environments never had quoted values in their cmdline, so the crash never triggered. The Arch build server recently acquired a quoted value (likely from a kernel or GRUB config change), exposing the bug. Additionally, bootloaders may reformat quotes: what rpm-ostree stores as testparam="value with spaces" appears in /proc/cmdline as "testparam=value with spaces" (GRUB wraps the entire token in quotes instead of just the value). Fix this with two changes: 1. Replace g_strsplit() with split_kernel_args() in ostree_kernel_args_append_proc_cmdline() so the initial tokenization of /proc/cmdline is quote-aware. Individual args passed to ostree_kernel_args_append() are now already properly split. 2. Replace g_assert_false(quoted) in split_kernel_args() with a g_debug() warning that continues execution, treating the remainder as a single token. Since we cannot control what bootloaders put in /proc/cmdline, a hard abort on unterminated quotes is never appropriate. Add a unit test (test_kargs_quoted_cmdline) covering standard quoted values, GRUB-reformatted quoting, and multiple quoted args. Add a kola destructive test (kargs-proc-cmdline-quoted.sh) that injects a quoted karg via rpm-ostree, reboots, and verifies --karg-proc-cmdline and --import-proc-cmdline succeed without crashing. Assisted-by: OpenCode (Claude claude-opus-4-6) Signed-off-by: Joseph Marrero Corchado <jmarrero@redhat.com>
84156d6 to
44dbf13
Compare
|
I know we probably need this in AGENTS.md but to me the commit message is overly verbose. I have this in my system prompt: https://github.com/cgwalters/homegit/blob/main/dotfiles/.config/AGENTS.md#commit-messages |
|
I will plead ignorance to the details on how all of the rust<->C interop works on the ostree side, but is it feasible at all to swap to using the |
|
We only have Rust bindings for libostree; the C library does not call any Rust right now. Doing so would totally be possible, and I started some of that before but it would be a huge change and in practice we're already rewriting all of this in Rust with the bootc + composefs backend anyways so let's just focus on that. |
ostree_kernel_args_append_proc_cmdline() crashes with SIGABRT when /proc/cmdline contains quoted kernel arguments with spaces (e.g. "testparam=value with spaces"). This was reported by Arch Linux maintainers when building ostree 2026.1 and reproduced on 2025.7.
Root cause: commit abc7d5b ("kargs: parse spaces in kargs input and keep quotes", March 2024) added split_kernel_args(), a quote-aware string splitter, and updated ostree_kernel_args_append() and three other functions to use it. However, it did not update ostree_kernel_args_append_proc_cmdline(), which still used a naive g_strsplit(" ") to tokenize /proc/cmdline. When the cmdline contained quoted values with spaces, g_strsplit broke them into fragments with unterminated quotes. These fragments were then passed to ostree_kernel_args_append() -> split_kernel_args(), which hit a g_assert_false(quoted) and aborted.
The bug was latent since v2024.5 because the test-admin-deploy-karg.sh and test-admin-instutil-set-kargs.sh tests read the real /proc/cmdline from the build host. CI environments never had quoted values in their cmdline, so the crash never triggered. The Arch build server recently acquired a quoted value (likely from a kernel or GRUB config change), exposing the bug.
Additionally, bootloaders may reformat quotes: what rpm-ostree stores as testparam="value with spaces" appears in /proc/cmdline as "testparam=value with spaces" (GRUB wraps the entire token in quotes instead of just the value).
Fix this with two changes:
Replace g_strsplit() with split_kernel_args() in ostree_kernel_args_append_proc_cmdline() so the initial tokenization of /proc/cmdline is quote-aware. Individual args passed to ostree_kernel_args_append() are now already properly split.
Replace g_assert_false(quoted) in split_kernel_args() with a g_debug() warning that continues execution, treating the remainder as a single token. Since we cannot control what bootloaders put in /proc/cmdline, a hard abort on unterminated quotes is never appropriate.
Add a unit test (test_kargs_quoted_cmdline) covering standard quoted values, GRUB-reformatted quoting, and multiple quoted args. Add a kola destructive test (kargs-proc-cmdline-quoted.sh) that injects a quoted karg via rpm-ostree, reboots, and verifies --karg-proc-cmdline and --import-proc-cmdline succeed without crashing.
Assisted-by: OpenCode (Claude claude-opus-4-6)
Fixes #3582