Re: [Libguestfs] [libguestfs-common PATCH 09/12] options: generalize "--key" selector parsing for C-language utilities

On Tue, Jun 28, 2022 at 01:49:12PM +0200, Laszlo Ersek wrote: > The key_store_add_from_selector() function parses the argument of the > "--key" option in C-language tools: > > OPTION_key [options/options.h] > key_store_add_from_selector() [options/keys.c] > key_store_import_key() [options/keys.c] > > The function currently (a) uses a horrible "goto", (b) is not flexible > enough for selector types that do not take precisely 1 type-specific > parameter. Rewrite the function with more informative error messages and > an easier to follow structure, plus make the type-specific parameter count > a function of the type. > > Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1809453 > Signed-off-by: Laszlo Ersek <lersek(a)redhat.com&gt; > --- > options/keys.c | 46 ++++++++++---------- > 1 file changed, 24 insertions(+), 22 deletions(-) > > diff --git a/options/keys.c b/options/keys.c > index 7729fe79c99b..a6ef2d78b589 100644 > --- a/options/keys.c > +++ b/options/keys.c > @@ -210,48 +210,50 @@ free_keys (struct matching_key *keys, size_t nr_matches) > } > > struct key_store * > key_store_add_from_selector (struct key_store *ks, const char *selector) > { > - CLEANUP_FREE_STRING_LIST char **fields = > - guestfs_int_split_string (':', selector); > + CLEANUP_FREE_STRING_LIST char **fields; > + size_t field_count; > struct key_store_key key; > > + fields = guestfs_int_split_string (':', selector); This code isn't wrong, but there is a "fun" catch with __attribute__((cleanup)) to be aware of. It wasn't obvious to me until I'd hit this problem a few times. __attribute__((cleanup, free_strings)) char **fields; fields = something; is fine, but: __attribute__((cleanup, free_strings)) char **fields; /* Add some new code */ if (!check_something) return; fields = something; is very bad. It will call free_strings on an uninitialized stack value. That was probably the reason why the original code tries to initialize fields where it was defined. Now again, this is not wrong, as long as everyone is aware that a future code addition could break things. Libvirt style insists that such variables are always initialized to NULL, but we don't do that consistently.

> if (!fields) > error (EXIT_FAILURE, errno, "guestfs_int_split_string"); > + field_count = guestfs_int_count_strings (fields); > > - if (guestfs_int_count_strings (fields) != 3) { > - invalid_selector: > - error (EXIT_FAILURE, 0, "invalid selector for --key: %s", selector); > - } > - > - /* 1: device */ > + /* field#0: ID */ > + if (field_count < 1) > + error (EXIT_FAILURE, 0, _("selector '%s': missing ID"), selector); > key.id = strdup (fields[0]); > if (!key.id) > error (EXIT_FAILURE, errno, "strdup"); > > - /* 2: key type */ > - if (STREQ (fields[1], "key")) > + /* field#1...: TYPE, and TYPE-specific properties */ > + if (field_count < 2) > + error (EXIT_FAILURE, 0, _("selector '%s': missing TYPE"), selector); > + > + if (STREQ (fields[1], "key")) { > key.type = key_string; > - else if (STREQ (fields[1], "file")) > - key.type = key_file; > - else > - goto invalid_selector; > - > - /* 3: actual key */ > - switch (key.type) { > - case key_string: > + if (field_count != 3) > + error (EXIT_FAILURE, 0, > + _("selector '%s': missing KEY_STRING, or too many fields"), > + selector); > key.string.s = strdup (fields[2]); > if (!key.string.s) > error (EXIT_FAILURE, errno, "strdup"); > - break; > - case key_file: > + } else if (STREQ (fields[1], "file")) { > + key.type = key_file; > + if (field_count != 3) > + error (EXIT_FAILURE, 0, > + _("selector '%s': missing FILENAME, or too many fields"), > + selector); > key.file.name = strdup (fields[2]); > if (!key.file.name) > error (EXIT_FAILURE, errno, "strdup"); > - break; > - } > + } else > + error (EXIT_FAILURE, 0, _("selector '%s': invalid TYPE"), selector); > > return key_store_import_key (ks, &key); Reviewed-by: Richard W.M. Jones <rjones(a)redhat.com&gt;