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

[Laszlo Ersek]

On 06/28/22 16:39, Richard W.M. Jones wrote:
> 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 at redhat.com>
>> ---
>>  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.

I'll initialize the variable to NULL.

> 
>>    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 at redhat.com>
> 
> 

Thanks!