From 869f9abda1fb899ec9c5c430803aa1ae9f725cb4 Mon Sep 17 00:00:00 2001 From: HuijingHei Date: Mon, 4 Mar 2024 10:44:42 +0800 Subject: [PATCH] kargs: parse spaces in kargs input and keep quotes According to Jonathan's suggestion, should fix the code from ostree repo. With this patch: - kargs input like "init_on_alloc=1 init_on_free=1", will be parsed as 2 seperated args `init_on_alloc=1` and `init_on_free=1`, instead of whole; - According to https://www.kernel.org/doc/html/v4.14/admin-guide/kernel-parameters.html, need to keep spaces in double-quotes, like `param="spaces in here"` will be parsed as whole instead of 3. Fixes https://github.com/coreos/rpm-ostree/issues/4821 --- src/libostree/ostree-kernel-args.c | 203 ++++++++++++++++++--------- src/ostree/ot-admin-builtin-deploy.c | 5 +- tests/test-admin-deploy-karg.sh | 9 +- tests/test-kargs.c | 35 ++++- 4 files changed, 173 insertions(+), 79 deletions(-) diff --git a/src/libostree/ostree-kernel-args.c b/src/libostree/ostree-kernel-args.c index 9a9e0be248..ec4b702d96 100644 --- a/src/libostree/ostree-kernel-args.c +++ b/src/libostree/ostree-kernel-args.c @@ -163,6 +163,49 @@ strcmp0_equal (gconstpointer v1, gconstpointer v2) return g_strcmp0 (v1, v2) == 0; } +/* Split string with whitespace, but keep quotes. + For example, "test=\"1 2\"" will be saved as whole. + */ +static char ** +split_whitespace (const char *str) +{ + gboolean quoted = FALSE; + g_return_val_if_fail (str != NULL, NULL); + + GPtrArray *strv = g_ptr_array_new (); + + guint len = strlen (str); + guint start; + // Skip first whitespaces + for (start = 0; start < len && str[start] == ' '; start++) + { + } + for (guint i = start; str[i] != '\0'; i++) + { + if (str[i] == '"') + quoted = !quoted; + else if (str[i] == ' ' && !quoted) + { + g_ptr_array_add (strv, g_strndup (str + start, i - start)); + start = i + 1; + } + + if (i == (len - 1)) + { + if (!quoted) + g_ptr_array_add (strv, g_strndup (str + start, len - start)); + else + { + g_printerr ("Error: Missing terminating quote in '%s'.\n", str); + g_assert_false (quoted); + } + } + } + + g_ptr_array_add (strv, NULL); + return (char **)g_ptr_array_free (strv, FALSE); +} + /** * ostree_kernel_args_new: (skip) * @@ -285,35 +328,42 @@ _ostree_kernel_arg_get_key_array (OstreeKernelArgs *kargs) gboolean ostree_kernel_args_new_replace (OstreeKernelArgs *kargs, const char *arg, GError **error) { - g_autofree char *arg_owned = g_strdup (arg); - const char *key = arg_owned; - const char *val = split_keyeq (arg_owned); - - GPtrArray *entries = g_hash_table_lookup (kargs->table, key); - if (!entries) - return glnx_throw (error, "No key '%s' found", key); - g_assert_cmpuint (entries->len, >, 0); + // Split the arg + g_auto (GStrv) argv = split_whitespace (arg); - /* first handle the case where the user just wants to replace an old value */ - if (val && strchr (val, '=')) + for (char **iter = argv; iter && *iter; iter++) { - g_autofree char *old_val = g_strdup (val); - const char *new_val = split_keyeq (old_val); - g_assert (new_val); + g_autofree char *arg_owned = g_strdup (*iter); + const char *key = arg_owned; + const char *val = split_keyeq (arg_owned); - guint i = 0; - if (!ot_ptr_array_find_with_equal_func (entries, old_val, kernel_args_entry_value_equal, &i)) - return glnx_throw (error, "No karg '%s=%s' found", key, old_val); + GPtrArray *entries = g_hash_table_lookup (kargs->table, key); + if (!entries) + return glnx_throw (error, "No key '%s' found", key); + g_assert_cmpuint (entries->len, >, 0); - kernel_args_entry_replace_value (entries->pdata[i], new_val); - return TRUE; - } + /* first handle the case where the user just wants to replace an old value */ + if (val && strchr (val, '=')) + { + g_autofree char *old_val = g_strdup (val); + const char *new_val = split_keyeq (old_val); + g_assert (new_val); + + guint i = 0; + if (!ot_ptr_array_find_with_equal_func (entries, old_val, kernel_args_entry_value_equal, + &i)) + return glnx_throw (error, "No karg '%s=%s' found", key, old_val); - /* can't know which val to replace without the old_val=new_val syntax */ - if (entries->len > 1) - return glnx_throw (error, "Multiple values for key '%s' found", key); + kernel_args_entry_replace_value (entries->pdata[i], new_val); + continue; + } - kernel_args_entry_replace_value (entries->pdata[0], val); + /* can't know which val to replace without the old_val=new_val syntax */ + if (entries->len > 1) + return glnx_throw (error, "Multiple values for key '%s' found", key); + + kernel_args_entry_replace_value (entries->pdata[0], val); + } return TRUE; } @@ -381,39 +431,47 @@ ostree_kernel_args_delete_key_entry (OstreeKernelArgs *kargs, const char *key, G gboolean ostree_kernel_args_delete (OstreeKernelArgs *kargs, const char *arg, GError **error) { - g_autofree char *arg_owned = g_strdup (arg); - const char *key = arg_owned; - const char *val = split_keyeq (arg_owned); - - GPtrArray *entries = g_hash_table_lookup (kargs->table, key); - if (!entries) - return glnx_throw (error, "No key '%s' found", key); - g_assert_cmpuint (entries->len, >, 0); + // Split the arg + g_auto (GStrv) argv = split_whitespace (arg); - /* special-case: we allow deleting by key only if there's only one val */ - if (entries->len == 1) + for (char **iter = argv; iter && *iter; iter++) { - /* but if a specific val was passed, check that it's the same */ - OstreeKernelArgsEntry *e = entries->pdata[0]; - if (val && !strcmp0_equal (val, _ostree_kernel_args_entry_get_value (e))) - return glnx_throw (error, "No karg '%s=%s' found", key, val); - return ostree_kernel_args_delete_key_entry (kargs, key, error); - } + g_autofree char *arg_owned = g_strdup (*iter); - /* note val might be NULL here, in which case we're looking for `key`, not `key=` or - * `key=val` */ - guint i = 0; - if (!ot_ptr_array_find_with_equal_func (entries, val, kernel_args_entry_value_equal, &i)) - { - if (!val) - /* didn't find NULL -> only key= key=val1 key=val2 style things left, so the user - * needs to be more specific */ - return glnx_throw (error, "Multiple values for key '%s' found", arg); - return glnx_throw (error, "No karg '%s' found", arg); - } + const char *key = arg_owned; + const char *val = split_keyeq (arg_owned); + + GPtrArray *entries = g_hash_table_lookup (kargs->table, key); + if (!entries) + return glnx_throw (error, "No key '%s' found", key); + g_assert_cmpuint (entries->len, >, 0); + + /* special-case: we allow deleting by key only if there's only one val */ + if (entries->len == 1) + { + /* but if a specific val was passed, check that it's the same */ + OstreeKernelArgsEntry *e = entries->pdata[0]; + if (val && !strcmp0_equal (val, _ostree_kernel_args_entry_get_value (e))) + return glnx_throw (error, "No karg '%s=%s' found", key, val); + g_assert (ostree_kernel_args_delete_key_entry (kargs, key, error)); + continue; + } - g_assert (g_ptr_array_remove (kargs->order, entries->pdata[i])); - g_assert (g_ptr_array_remove_index (entries, i)); + /* note val might be NULL here, in which case we're looking for `key`, not `key=` or + * `key=val` */ + guint i = 0; + if (!ot_ptr_array_find_with_equal_func (entries, val, kernel_args_entry_value_equal, &i)) + { + if (!val) + /* didn't find NULL -> only key= key=val1 key=val2 style things left, so the user + * needs to be more specific */ + return glnx_throw (error, "Multiple values for key '%s' found", arg); + return glnx_throw (error, "No karg '%s' found", arg); + } + + g_assert (g_ptr_array_remove (kargs->order, entries->pdata[i])); + g_assert (g_ptr_array_remove_index (entries, i)); + } return TRUE; } @@ -499,27 +557,34 @@ ostree_kernel_args_replace (OstreeKernelArgs *kargs, const char *arg) void ostree_kernel_args_append (OstreeKernelArgs *kargs, const char *arg) { - gboolean existed = TRUE; - GPtrArray *entries = NULL; - char *duped = g_strdup (arg); - const char *val = split_keyeq (duped); + // Split the arg + g_auto (GStrv) argv = split_whitespace (arg); - entries = g_hash_table_lookup (kargs->table, duped); - if (!entries) + for (char **iter = argv; iter && *iter; iter++) { - entries = g_ptr_array_new_with_free_func (kernel_args_entry_free_from_table); - existed = FALSE; - } + gboolean existed = TRUE; + GPtrArray *entries = NULL; - OstreeKernelArgsEntry *entry = _ostree_kernel_args_entry_new (); - _ostree_kernel_args_entry_set_key (entry, duped); - _ostree_kernel_args_entry_set_value (entry, g_strdup (val)); + char *duped = g_strdup (*iter); + const char *val = split_keyeq (duped); - g_ptr_array_add (entries, entry); - g_ptr_array_add (kargs->order, entry); + entries = g_hash_table_lookup (kargs->table, duped); + if (!entries) + { + entries = g_ptr_array_new_with_free_func (kernel_args_entry_free_from_table); + existed = FALSE; + } + + OstreeKernelArgsEntry *entry = _ostree_kernel_args_entry_new (); + _ostree_kernel_args_entry_set_key (entry, duped); + _ostree_kernel_args_entry_set_value (entry, g_strdup (val)); - if (!existed) - g_hash_table_replace (kargs->table, duped, entries); + g_ptr_array_add (entries, entry); + g_ptr_array_add (kargs->order, entry); + + if (!existed) + g_hash_table_replace (kargs->table, duped, entries); + } } /** @@ -644,7 +709,7 @@ ostree_kernel_args_parse_append (OstreeKernelArgs *kargs, const char *options) if (!options) return; - args = g_strsplit (options, " ", -1); + args = split_whitespace (options); for (iter = args; *iter; iter++) { char *arg = *iter; diff --git a/src/ostree/ot-admin-builtin-deploy.c b/src/ostree/ot-admin-builtin-deploy.c index f14cce5d54..66d857241b 100644 --- a/src/ostree/ot-admin-builtin-deploy.c +++ b/src/ostree/ot-admin-builtin-deploy.c @@ -184,10 +184,7 @@ ot_admin_builtin_deploy (int argc, char **argv, OstreeCommandInvocation *invocat && (opt_kernel_argv || opt_kernel_argv_append || opt_kernel_argv_delete)) { OstreeBootconfigParser *bootconfig = ostree_deployment_get_bootconfig (merge_deployment); - g_auto (GStrv) previous_args - = g_strsplit (ostree_bootconfig_parser_get (bootconfig, "options"), " ", -1); - kargs = ostree_kernel_args_new (); - ostree_kernel_args_append_argv (kargs, previous_args); + kargs = ostree_kernel_args_from_string (ostree_bootconfig_parser_get (bootconfig, "options")); } /* Now replace/extend the above set. Note that if no options are specified, diff --git a/tests/test-admin-deploy-karg.sh b/tests/test-admin-deploy-karg.sh index 39fcdf3614..bb6ad02700 100755 --- a/tests/test-admin-deploy-karg.sh +++ b/tests/test-admin-deploy-karg.sh @@ -24,7 +24,7 @@ set -euo pipefail # Exports OSTREE_SYSROOT so --sysroot not needed. setup_os_repository "archive" "syslinux" -echo "1..4" +echo "1..5" ${CMD_PREFIX} ostree --repo=sysroot/ostree/repo pull-local --remote=testos testos-repo testos/buildmain/x86_64-runtime rev=$(${CMD_PREFIX} ostree --repo=sysroot/ostree/repo rev-parse testos/buildmain/x86_64-runtime) @@ -77,3 +77,10 @@ assert_not_file_has_content sysroot/boot/loader/entries/ostree-2-testos.conf 'op assert_not_file_has_content sysroot/boot/loader/entries/ostree-2-testos.conf 'options.*APPENDARG=VALAPPEND' echo "ok deploy --karg-delete" + +${CMD_PREFIX} ostree admin deploy --os=testos --karg-append 'test="1 2"' testos:testos/buildmain/x86_64-runtime +assert_file_has_content sysroot/boot/loader/entries/ostree-2-testos.conf 'options.*test="1 2"' +${CMD_PREFIX} ostree admin deploy --os=testos --karg-delete 'test="1 2"' testos:testos/buildmain/x86_64-runtime +assert_not_file_has_content sysroot/boot/loader/entries/ostree-2-testos.conf 'options.*test="1 2"' + +echo "ok deploy --karg-delete with quotes" diff --git a/tests/test-kargs.c b/tests/test-kargs.c index bd1735d4b8..90f11e5f85 100644 --- a/tests/test-kargs.c +++ b/tests/test-kargs.c @@ -24,8 +24,7 @@ static gboolean check_string_existance (OstreeKernelArgs *karg, const char *string_to_find) { - g_autofree gchar *string_with_spaces = ostree_kernel_args_to_string (karg); - g_auto (GStrv) string_list = g_strsplit (string_with_spaces, " ", -1); + g_auto (GStrv) string_list = ostree_kernel_args_to_strv (karg); return g_strv_contains ((const char *const *)string_list, string_to_find); } @@ -140,6 +139,16 @@ test_kargs_delete (void) g_assert_no_error (error); g_assert (ret); g_assert (!check_string_existance (karg, "nosmt")); + + /* Make sure we also support quotes and spaces */ + ostree_kernel_args_append (karg, "foo=\"1 2\" bar=0"); + check_string_existance (karg, "foo=\"1 2\""); + check_string_existance (karg, "bar=0"); + ret = ostree_kernel_args_delete (karg, "foo=\"1 2\" bar=0", &error); + g_assert_no_error (error); + g_assert (ret); + g_assert (!check_string_existance (karg, "foo=\"1 2\"")); + g_assert (!check_string_existance (karg, "bar=0")); } static void @@ -189,6 +198,16 @@ test_kargs_replace (void) g_assert (ret); g_assert (!check_string_existance (karg, "test=firstval")); g_assert (check_string_existance (karg, "test=newval")); + + /* Replace with input contains quotes and spaces, it should support */ + ostree_kernel_args_append (karg, "foo=1 bar=\"1 2\""); + ret = ostree_kernel_args_new_replace (karg, "foo=\"1 2\" bar", &error); + g_assert_no_error (error); + g_assert (ret); + g_assert (!check_string_existance (karg, "foo=1")); + g_assert (!check_string_existance (karg, "bar=\"1 2\"")); + g_assert (check_string_existance (karg, "foo=\"1 2\"")); + g_assert (check_string_existance (karg, "bar")); } /* In this function, we want to verify that ostree_kernel_args_append @@ -206,6 +225,7 @@ test_kargs_append (void) ostree_kernel_args_append (append_arg, "test="); ostree_kernel_args_append (append_arg, "test"); ostree_kernel_args_append (append_arg, "second_test"); + ostree_kernel_args_append (append_arg, "second_test=0 second_test=\"1 2\""); /* We loops through the kargs inside table to verify * the functionality of append because at this stage @@ -230,6 +250,10 @@ test_kargs_append (void) g_assert_cmpstr (key, ==, "second_test"); g_assert (ot_ptr_array_find_with_equal_func (value_array, NULL, kernel_args_entry_value_equal, NULL)); + g_assert (ot_ptr_array_find_with_equal_func (value_array, "0", + kernel_args_entry_value_equal, NULL)); + g_assert (ot_ptr_array_find_with_equal_func (value_array, "\"1 2\"", + kernel_args_entry_value_equal, NULL)); } } @@ -243,14 +267,15 @@ test_kargs_append (void) /* Up till this point, we verified that the above was all correct, we then * check ostree_kernel_args_to_string has the right result */ - g_autofree gchar *kargs_str = ostree_kernel_args_to_string (append_arg); - g_auto (GStrv) kargs_list = g_strsplit (kargs_str, " ", -1); + g_auto (GStrv) kargs_list = ostree_kernel_args_to_strv (append_arg); g_assert (g_strv_contains ((const char *const *)kargs_list, "test=valid")); g_assert (g_strv_contains ((const char *const *)kargs_list, "test=secondvalid")); g_assert (g_strv_contains ((const char *const *)kargs_list, "test=")); g_assert (g_strv_contains ((const char *const *)kargs_list, "test")); g_assert (g_strv_contains ((const char *const *)kargs_list, "second_test")); - g_assert_cmpint (5, ==, g_strv_length (kargs_list)); + g_assert (g_strv_contains ((const char *const *)kargs_list, "second_test=0")); + g_assert (g_strv_contains ((const char *const *)kargs_list, "second_test=\"1 2\"")); + g_assert_cmpint (7, ==, g_strv_length (kargs_list)); } int