[PATCH 0/2] authz-list-file: Error handling fixes

[PATCH 0/2] authz-list-file: Error handling fixes

[Markus Armbruster]

Possibly 5.2 material, because a silly mistake in use of object-add
can crash QEMU.

Markus Armbruster (2):
  authz-list-file: Fix file read error handling
  authz-list-file: Improve an error message

 authz/listfile.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

-- 
2.26.2

[PATCH 1/2] authz-list-file: Fix file read error handling

[Markus Armbruster]

The Error ** argument must be NULL, &error_abort, &error_fatal, or a
pointer to a variable containing NULL.  Passing an argument of the
latter kind twice without clearing it in between is wrong: if the
first call sets an error, it no longer points to NULL for the second
call.

qauthz_list_file_complete() is wrong that way: it passes @errp to
qauthz_list_file_complete() without checking for failure.  If it runs
into another failure, it trips error_setv()'s assertion.  Reproducer:

    $ qemu-system-x86_64 -nodefaults -S -display none -object authz-list-file,id=authz0,filename=
    qemu-system-x86_64: ../util/error.c:59: error_setv: Assertion `*errp == NULL' failed.
    Aborted (core dumped)

Fix it to check for failure.

Fixes: 55d869846de802a16af1a50584c51737bd664387
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 authz/listfile.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/authz/listfile.c b/authz/listfile.c
index 24feac35ab..1421e674a4 100644
--- a/authz/listfile.c
+++ b/authz/listfile.c
@@ -128,6 +128,9 @@ qauthz_list_file_complete(UserCreatable *uc, Error **errp)
     }
 
     fauthz->list = qauthz_list_file_load(fauthz, errp);
+    if (!fauthz->list) {
+        return;
+    }
 
     if (!fauthz->refresh) {
         return;
-- 
2.26.2

[PATCH 2/2] authz-list-file: Improve an error message

[Markus Armbruster]

When qauthz_list_file_load() rejects JSON values other than JSON
object with a rather confusing error message:

    $ echo 1 | qemu-system-x86_64 -nodefaults -S -display none  -object authz-list-file,id=authz0,filename=/dev/stdin
    qemu-system-x86_64: -object authz-list-file,id=authz0,filename=/dev/stdin: Invalid parameter type for 'obj', expected: dict

Improve to

    qemu-system-x86_64: -object authz-list-file,id=authz0,filename=/dev/stdin: File '/dev/stdin' must contain a JSON object

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 authz/listfile.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/authz/listfile.c b/authz/listfile.c
index 1421e674a4..da3a0e69a2 100644
--- a/authz/listfile.c
+++ b/authz/listfile.c
@@ -73,7 +73,8 @@ qauthz_list_file_load(QAuthZListFile *fauthz, Error **errp)
 
     pdict = qobject_to(QDict, obj);
     if (!pdict) {
-        error_setg(errp, QERR_INVALID_PARAMETER_TYPE, "obj", "dict");
+        error_setg(errp, "File '%s' must contain a JSON object",
+                   fauthz->filename);
         goto cleanup;
     }
 
-- 
2.26.2

Re: [PATCH 2/2] authz-list-file: Improve an error message

[Daniel P. Berrangé]

On Fri, Nov 13, 2020 at 07:23:58AM +0100, Markus Armbruster wrote:
> When qauthz_list_file_load() rejects JSON values other than JSON
> object with a rather confusing error message:
> 
>     $ echo 1 | qemu-system-x86_64 -nodefaults -S -display none  -object authz-list-file,id=authz0,filename=/dev/stdin
>     qemu-system-x86_64: -object authz-list-file,id=authz0,filename=/dev/stdin: Invalid parameter type for 'obj', expected: dict
> 
> Improve to
> 
>     qemu-system-x86_64: -object authz-list-file,id=authz0,filename=/dev/stdin: File '/dev/stdin' must contain a JSON object
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  authz/listfile.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>

> 
> diff --git a/authz/listfile.c b/authz/listfile.c
> index 1421e674a4..da3a0e69a2 100644
> --- a/authz/listfile.c
> +++ b/authz/listfile.c
> @@ -73,7 +73,8 @@ qauthz_list_file_load(QAuthZListFile *fauthz, Error **errp)
>  
>      pdict = qobject_to(QDict, obj);
>      if (!pdict) {
> -        error_setg(errp, QERR_INVALID_PARAMETER_TYPE, "obj", "dict");
> +        error_setg(errp, "File '%s' must contain a JSON object",
> +                   fauthz->filename);

This code pattern was copied from other places in QEMU which use the
same QERR_INVALID_PARAMETER_TYPE. There are another 10 or so examples
of this error message pattern.


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH 1/2] authz-list-file: Fix file read error handling

[Daniel P. Berrangé]

On Fri, Nov 13, 2020 at 07:23:57AM +0100, Markus Armbruster wrote:
> The Error ** argument must be NULL, &error_abort, &error_fatal, or a
> pointer to a variable containing NULL.  Passing an argument of the
> latter kind twice without clearing it in between is wrong: if the
> first call sets an error, it no longer points to NULL for the second
> call.
> 
> qauthz_list_file_complete() is wrong that way: it passes @errp to
> qauthz_list_file_complete() without checking for failure.  If it runs
> into another failure, it trips error_setv()'s assertion.  Reproducer:
> 
>     $ qemu-system-x86_64 -nodefaults -S -display none -object authz-list-file,id=authz0,filename=
>     qemu-system-x86_64: ../util/error.c:59: error_setv: Assertion `*errp == NULL' failed.
>     Aborted (core dumped)
> 
> Fix it to check for failure.
> 
> Fixes: 55d869846de802a16af1a50584c51737bd664387
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  authz/listfile.c | 3 +++
>  1 file changed, 3 insertions(+)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH 2/2] authz-list-file: Improve an error message

[Markus Armbruster]

Daniel P. Berrangé <berrange@redhat.com> writes:

> On Fri, Nov 13, 2020 at 07:23:58AM +0100, Markus Armbruster wrote:
>> When qauthz_list_file_load() rejects JSON values other than JSON
>> object with a rather confusing error message:
>> 
>>     $ echo 1 | qemu-system-x86_64 -nodefaults -S -display none  -object authz-list-file,id=authz0,filename=/dev/stdin
>>     qemu-system-x86_64: -object authz-list-file,id=authz0,filename=/dev/stdin: Invalid parameter type for 'obj', expected: dict
>> 
>> Improve to
>> 
>>     qemu-system-x86_64: -object authz-list-file,id=authz0,filename=/dev/stdin: File '/dev/stdin' must contain a JSON object
>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  authz/listfile.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>

Thanks!

>> diff --git a/authz/listfile.c b/authz/listfile.c
>> index 1421e674a4..da3a0e69a2 100644
>> --- a/authz/listfile.c
>> +++ b/authz/listfile.c
>> @@ -73,7 +73,8 @@ qauthz_list_file_load(QAuthZListFile *fauthz, Error **errp)
>>  
>>      pdict = qobject_to(QDict, obj);
>>      if (!pdict) {
>> -        error_setg(errp, QERR_INVALID_PARAMETER_TYPE, "obj", "dict");
>> +        error_setg(errp, "File '%s' must contain a JSON object",
>> +                   fauthz->filename);
>
> This code pattern was copied from other places in QEMU which use the
> same QERR_INVALID_PARAMETER_TYPE. There are another 10 or so examples
> of this error message pattern.

Yes.  My "[PATCH 00/10] Chipping away at qerror.h" fixes other patterns,
but not this one.  I intend to chip some more, as time permits.