kargs: parse spaces in kargs input and keep quotes

[HuijingHei]

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 coreos/rpm-ostree#4821

[openshift-ci]

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

[HuijingHei]

Do local testing and failed free(): double free detected in tcache 2, the error is vague, need to do more investigation, and any pointer is appreciated, thanks!

# cosa build-fast
# cosa run --qemu-image xx
# rpm-ostree kargs --append "test=\"1 2\""
[root@cosa-devsh ~]# rpm-ostree kargs --delete "test=\"1 2\""
error: Bus owner changed, aborting. This likely means the daemon crashed; check logs with `journalctl -xe`.

[root@cosa-devsh ~]# journalctl -u rpm-ostreed.service
...
Mar 04 08:54:33 cosa-devsh rpm-ostree[1904]: free(): double free detected in tcache 2

# journalctl -xe
Stack trace of thread 2121:
#0  0x00007fd005f5a834 __pthread_kill_implementation (libc.so.6 + 0x90834)
#1  0x00007fd005f088ee raise (libc.so.6 + 0x3e8ee)
#2  0x00007fd005ef08ff abort (libc.so.6 + 0x268ff)
#3  0x00007fd005ef17d0 __libc_message.cold (libc.so.6 + 0x277d0)
#4  0x00007fd005f647a5 malloc_printerr (libc.so.6 + 0x9a7a5)
#5  0x00007fd005f66c6f _int_free (libc.so.6 + 0x9cc6f)
#6  0x00007fd005f693de free (libc.so.6 + 0x9f3de)
#7  0x00007fd006cdf865 g_free (libglib-2.0.so.0 + 0x5e865)
#8  0x00007fd0067bd4d1 g_autoptr_cleanup_generic_gfree (libostree-1.so.1 + 0x7a4d1)
#9  0x000056187a05d7b3 kernel_arg_apply_patching (rpm-ostree + 0x5ad7b3)
#10 0x000056187a059f7b transaction_execute_thread (rpm-ostree + 0x5a9f7b)
#11 0x00007fd006e82774 g_task_thread_pool_thread (libgio-2.0.so.0 + 0xb7774)
#12 0x00007fd006d11db2 g_thread_pool_thread_proxy.lto_priv.0 (libglib-2.0.so.0 + 0x90db2)
#13 0x00007fd006d0d523 g_thread_proxy (libglib-2.0.so.0 + 0x8c523)
#14 0x00007fd005f58897 start_thread (libc.so.6 + 0x8e897)
#15 0x00007fd005fdf80c __clone3 (libc.so.6 + 0x11580c)

[ericcurtin]

Might be worth taking a peak at kernel/systemd and other karg parsing implementations, this kind of thing has been done in other places before.

Sometimes for this kinda thing it's worth testing loads of random things in a small implementation for quick iterations (and use valgrind, sanitizers, -fanalyzer etc.):

#include <stdio.h>

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)
        {
          g_ptr_array_add (strv, g_strndup (str + start, i - start));
          start = i + 1;
        }
      if (str[i] == '"')
        {
          quoted = !quoted;
        }
      if (i == (len - 1))
        {
          if (!quoted)
            g_ptr_array_add (strv, g_strndup (str + start, len - start));
        }
    }
  g_ptr_array_add (strv, NULL);
  return (char **)g_ptr_array_free (strv, FALSE);
}

int main() {
  split_whitespace(argv[1]);
}

then add as an automated test. I didn't look at the code in any great detail, but on first glance it looks like it could be a string manipulation bug based on the double free.

seems to me that we don't exactly want to split by whitespace here.

[ericcurtin]

I also wonder should you just try and edit the BLS files directly rather than change this feature.

grubby has a similar feature which I've had trouble with in the past, these days I find editing the BLS directly easier.

[ericcurtin]

The delete interface might be a little simpler/cleaner if it expected key only (the test part)

[HuijingHei]

Thanks @ericcurtin for the review, the error is fixed with the help of @jmarrero , it is because the previous change in ostree_kernel_args_delete(), it is for debug and remove that part, then no error.

[ericcurtin]

LGTM, might want to run this through gcc's "-fanalyzer" to see if it identifies any potential problems, string manipulation is always tricky.