[libvirt] [PATCH 5/7] tests: Add checks for possible errors

[Peter Krempa]

On Tue, Apr 17, 2018 at 12:22:16 -0400, John Ferlan wrote:
> If virJSONValueObjectGetArray fails to find "members" in @localroot,
> then using @rootmembers in subsequent calls which assume that it was
> successful will not go well. So add a check that it was successfully
> fetched and an error if not.
> 
> Similarly virJSONValueArraySize returns an 'ssize_t', so the callers
> should use the right type and they should check the return value
> against -1 and print an error if something is wrong.
> 
> Found by Coverity
> 
> Signed-off-by: John Ferlan <jferlan at redhat.com>
> ---
>  tests/testutilsqemuschema.c | 40 +++++++++++++++++++++++++++++-----------
>  1 file changed, 29 insertions(+), 11 deletions(-)
> 
> diff --git a/tests/testutilsqemuschema.c b/tests/testutilsqemuschema.c
> index 21f5d119e8..79c526ac7c 100644
> --- a/tests/testutilsqemuschema.c
> +++ b/tests/testutilsqemuschema.c
> @@ -97,14 +97,19 @@ struct testQEMUSchemaValidateObjectMemberData {
>  
>  static virJSONValuePtr
>  testQEMUSchemaStealObjectMemberByName(const char *name,
> -                                      virJSONValuePtr members)
> +                                      virJSONValuePtr members,
> +                                      virBufferPtr debug)
>  {
>      virJSONValuePtr member;
>      virJSONValuePtr ret = NULL;
> -    size_t n;
> +    ssize_t n;
>      size_t i;
>  
> -    n = virJSONValueArraySize(members);
> +    if ((n = virJSONValueArraySize(members)) < 0) {
> +        virBufferAddLit(debug, "ERROR: members size < 0");
> +        return NULL;
> +    }
> +
>      for (i = 0; i < n; i++) {
>          member = virJSONValueArrayGet(members, i);
>  
> @@ -132,7 +137,7 @@ testQEMUSchemaValidateObjectMember(const char *key,
>      virBufferStrcat(data->debug, key, ": ", NULL);
>  
>      /* lookup 'member' entry for key */
> -    if (!(keymember = testQEMUSchemaStealObjectMemberByName(key, data->rootmembers))) {
> +    if (!(keymember = testQEMUSchemaStealObjectMemberByName(key, data->rootmembers, data->debug))) {
>          virBufferAddLit(data->debug, "ERROR: attribute not in schema");
>          goto cleanup;


All of this bove should be fixed by checking the return value of
virJSONValueObjectGetArray in testQEMUSchemaValidateObject where
data.rootmembers.

This will make sure that the callee gets an array and thus the array
size function will not return -1;

>      }

[...]

> @@ -203,7 +208,11 @@ testQEMUSchemaValidateObjectMergeVariant(virJSONValuePtr root,
>          return -2;
>      }
>  
> -    n = virJSONValueArraySize(variants);

'variants' was retrieved via virJSONValueObjectStealArray thus this will
not return -1 as it was type-checked.

> +    if ((n = virJSONValueArraySize(variants)) < 0) {
> +        virBufferAddLit(debug, "ERROR: 'variants' array size < 0");
> +        return -2;
> +    }
> +
>      for (i = 0; i < n; i++) {
>          variant = virJSONValueArrayGet(variants, i);
>  
> @@ -307,7 +316,10 @@ testQEMUSchemaValidateObject(virJSONValuePtr obj,
>  
>  
>      /* validate members */
> -    data.rootmembers = virJSONValueObjectGetArray(localroot, "members");
> +    if (!(data.rootmembers = virJSONValueObjectGetArray(localroot, "members"))) {
> +        virBufferAddLit(debug, "ERROR: missing attribute 'members'\n");
> +        goto cleanup;
> +    }
>      if (virJSONValueObjectForeachKeyValue(obj,
>                                            testQEMUSchemaValidateObjectMember,
>                                            &data) < 0)

[...]

> @@ -358,7 +370,10 @@ testQEMUSchemaValidateEnum(virJSONValuePtr obj,
>          return -2;
>      }
>  
> -    n = virJSONValueArraySize(values);

'values' was retrieved via virJSONValueObjectGetArray thus this will
not return -1 as it was type-checked.

> +    if ((n = virJSONValueArraySize(values)) < 0) {
> +        virBufferAddLit(debug, "ERROR: enum values array size < 0");
> +        return -2;
> +    }
>      for (i = 0; i < n; i++) {
>          value = virJSONValueArrayGet(values, i);
>  

[...]

> @@ -439,7 +454,10 @@ testQEMUSchemaValidateAlternate(virJSONValuePtr obj,
>      virBufferAdjustIndent(debug, 3);
>      indent = virBufferGetIndent(debug, false);
>  
> -    n = virJSONValueArraySize(members);

'members' was retrieved via virJSONValueObjectGetArray thus this will
not return -1 as it was type-checked.

> +    if ((n = virJSONValueArraySize(members)) < 0) {
> +        virBufferAddLit(debug, "ERROR: alternate schema 'members' array size < 0");
> +        return -2;
> +    }
>      for (i = 0; i < n; i++) {
>          membertype = NULL;

Also all of the error messages are misleading. The schema can't have an
array with < 0 elements, since that is not possible in JSON. The problem
always will be that the field is either missing or of incorrect type.

Rather than adding pointless checks it might be worth just to modify
virJSONValueArraySize so that it does not do type checking and offload
that to the caller.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20180418/9dd4de26/attachment-0001.sig>