[libvirt] [PATCH] util: Fix return value for virJSONValueFromString if it fails

[Daniel P. Berrange]

On Wed, Mar 23, 2011 at 08:07:25PM +0800, Osier Yang wrote:
> Problem:
>   "parser.head" is not NULL even if it's free'ed by "virJSONValueFree",
> returning "parser.head" when "virJSONValueFromString" fails will cause
> unexpected errors (libvirtd will crash sometimes), e.g.
>   In function "qemuMonitorJSONArbitraryCommand":
> 
>         if (!(cmd = virJSONValueFromString(cmd_str)))
>             goto cleanup;
> 
>         if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0)
>             goto cleanup;
> 
>         ......
> 
>      cleanup:
>         virJSONValueFree(cmd);
> 
>   It will continues to send command to monitor even if "virJSONValueFromString"
> is failed, and more worse, it trys to free "cmd" again.
> 
>   Crash example:
> {"error":{"class":"QMPBadInputObject","desc":"Expected 'execute' in QMP input","data":{"expected":"execute"}}}
> {"error":{"class":"QMPBadInputObject","desc":"Expected 'execute' in QMP input","data":{"expected":"execute"}}}
> error: server closed connection:
> error: unable to connect to '/var/run/libvirt/libvirt-sock', libvirtd may need to be started: Connection refused
> error: failed to connect to the hypervisor
> 
>   This fix is to:
>     1) return NULL for failure of "virJSONValueFromString",
>     2) and it seems "virJSONValueFree" uses incorrect loop index for type
>        of "VIR_JSON_TYPE_OBJECT", fix it together.
> 
> * src/util/json.c
> ---
>  src/util/json.c |    7 +++++--
>  1 files changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/src/util/json.c b/src/util/json.c
> index f90594c..be47f64 100644
> --- a/src/util/json.c
> +++ b/src/util/json.c
> @@ -65,7 +65,7 @@ void virJSONValueFree(virJSONValuePtr value)
> 
>      switch (value->type) {
>      case VIR_JSON_TYPE_OBJECT:
> -        for (i = 0 ; i < value->data.array.nvalues ; i++) {
> +        for (i = 0 ; i < value->data.object.npairs; i++) {
>              VIR_FREE(value->data.object.pairs[i].key);
>              virJSONValueFree(value->data.object.pairs[i].value);
>          }
> @@ -897,6 +897,7 @@ virJSONValuePtr virJSONValueFromString(const char *jsonstring)
>      yajl_parser_config cfg = { 1, 1 };
>      yajl_handle hand;
>      virJSONParser parser = { NULL, NULL, 0 };
> +    virJSONValuePtr ret = NULL;
> 
>      VIR_DEBUG("string=%s", jsonstring);
> 
> @@ -917,6 +918,8 @@ virJSONValuePtr virJSONValueFromString(const char *jsonstring)
>          goto cleanup;
>      }
> 
> +    ret = parser.head;
> +
>  cleanup:
>      yajl_free(hand);
> 
> @@ -930,7 +933,7 @@ cleanup:
> 
>      VIR_DEBUG("result=%p", parser.head);
> 
> -    return parser.head;
> +    return ret;
>  }

ACK

Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|