Re: [Xen-devel] [PATCH RFC V2 05/10] libxl_json: introduce parser functions for builtin types

[Wei Liu <wei.liu2@xxxxxxxxxx>]

On Tue, Apr 22, 2014 at 04:09:58PM +0100, Ian Campbell wrote:
> Please could you CC Anthony too, I think he wrote all this
> libxl__json_object stuff.
> 
> On Thu, 2014-04-17 at 12:13 +0100, Wei Liu wrote:
> > @@ -396,6 +396,66 @@ out:
> >      return s;
> >  }
> >  
> > +int libxl_cpuid_policy_list_parse_json(libxl_ctx *ctx,
> > +                                       const libxl__json_object *o,
> > +                                       libxl_cpuid_policy_list *p)
> > +{
> > +    int i, size;
> > +    libxl_cpuid_policy_list l;
> > +
> > +    if (!libxl__json_object_is_array(o))
> > +        return -1;
> > +
> > +    if (!o->u.array->count)
> > +        return 0;
> > +
> > +    size = o->u.array->count;
> > +    /* need one extra slot as setinel */
> 
> "sentinel"
> 
> > +    l = *p = calloc(size + 1, sizeof(libxl_cpuid_policy));
> 
> This function should GC_INIT and use the provided gc for allocations,
> shouldn't it?.
> 

I see. You're right here.

> > +
> > +    if (!l)
> > +        return -1;
> > +
> > +    memset(l, 0, sizeof(libxl_cpuid_policy) * (size + 1));
> > +    l[size].input[0] = XEN_CPUID_INPUT_UNUSED;
> > +    l[size].input[1] = XEN_CPUID_INPUT_UNUSED;
> > +
> > +    for (i = 0; i < size; i++) {
> > +        const libxl__json_object *t;
> > +        int j;
> > +
> > +        if (flexarray_get(o->u.array, i, (void**)&t) != 0)
> > +            return -1;          /* dispose_fn will clean up memory
> > +                                 * allocation */
> 
> You haven't done a gc allocation, so it won't. Looking closer I think
> this falls into case #1 of the comment in libxl.h, which means the
> allocation should have come from the heap, but that means you need to
> manually free it.
> 

The aformentioned "calloc" falls into #1 but not this one.

My comment is a bit confusing. The comment here actually refers to all
the malloc'ed memory in the loop, which will be freed by
libxl_cpuid_dispose.

In any case I will check other occurences of this.

> (I see this comment about dispose_fn a lot, so I won't repeat it)
> 
> > +        for (j = 0; j < 4; j++) {
> 
> This, and the code it is based on, really ought to use ARRAY_SIZE I
> think.
> 

Ack.

> > +            const libxl__json_object *r;
> > +
> > +            r = libxl__json_map_get(policy_names[j], t, JSON_STRING);
> > +            if (!r)
> > +                l[i].policy[j] = NULL;
> > +            else
> > +                l[i].policy[j] = strdup(r->u.string);
> > +        }
> > +    }
> > +
> > +    assert(i == size);
> 
> Given a lack of break's in the for loop this seems overly defensive to
> me.
> 

I will remove this.

> > +
> > +    return 0;
> > +}
> > +
> >  /*
> >   * Local variables:
> >   * mode: C
> > diff --git a/tools/libxl/libxl_json.c b/tools/libxl/libxl_json.c
> > index 6f1116f..e2d5dbe 100644
> > --- a/tools/libxl/libxl_json.c
> > +++ b/tools/libxl/libxl_json.c
> > @@ -100,6 +100,35 @@ yajl_gen_status libxl_defbool_gen_json(yajl_gen hand,
> >      return libxl__yajl_gen_asciiz(hand, libxl_defbool_to_string(*db));
> >  }
> >  
> > +int libxl_defbool_parse_json(libxl_ctx *ctx, const libxl__json_object *o,
> > +                             libxl_defbool *p)
> > +{
> > +    if (!libxl__json_object_is_string(o))
> > +        return -1;
> > +
> > +    if (!strncmp(o->u.string, "<default>", strlen("<default>")))
> > +        p->val = 0;
> > +    else if (!strncmp(o->u.string, "True", strlen("True")))
> > +        p->val = 1;
> > +    else if (!strncmp(o->u.string, "False", strlen("False")))
> 
> Should have some internal #defines for these strings and use in the
> generator too. Also you should use LIBXL__DEFBOOL_DEFAULT and friends.
> 

Ack.

> > @@ -128,6 +166,34 @@ out:
> >      return s;
> >  }
> >  
> > +int libxl_bitmap_parse_json(libxl_ctx *ctx, const libxl__json_object *o,
> > +                            libxl_bitmap *p)
> > +{
> > +    int i;
> > +    int size;
> > +    const libxl__json_object *t;
> > +
> > +    if (!libxl__json_object_is_array(o))
> > +        return -1;
> > +
> > +    if (!o->u.array->count)
> > +        return 0;
> > +
> > +    t = libxl__json_array_get(o, o->u.array->count - 1);
> > +    size = t->u.i + 1;
> 
> What is this doing? It's retrieving the last item in the array, assuming
> it is an integer and then inferring the length of the array from it?
> 

Yes.

> Oh this is because a bitmask is represented as an array which contains a
> list of the bit indexes which are true.
> 
> Is the json array guaranteed to be sorted? I guess it is if libxl
> generated it, but can we rely on that?
> 

Yes. I basically reverse-engineer the process to generate the array and
find out that it's sorted.

> I wonder if it is too late to change the json representation of a
> bitmask that we are using. Thoughts?
> 

Probably yes. It's a public function in libxl API so I wouldn't be
surprised if some high level tool depends on it.

> > +
> > +    libxl_bitmap_alloc(ctx, p, size);
> > +
> > +    for (i = 0; (t = libxl__json_array_get(o, i)); i++) {
> > +        if (!libxl__json_object_is_integer(t))
> > +            return -1;          /* dispose_fn will clean up memory
> > +                                 * allocation */
> > +        libxl_bitmap_set(p, t->u.i);
> > +    }
> > +
> > +    return 0;
> > +}
> > +
> >  yajl_gen_status libxl_key_value_list_gen_json(yajl_gen hand,
> >                                                libxl_key_value_list *pkvl)
> >  {
> > @@ -155,6 +221,43 @@ out:
> >      return s;
> >  }
> >  
> > +int libxl_key_value_list_parse_json(libxl_ctx *ctx, const 
> > libxl__json_object *o,
> > +                                    libxl_key_value_list *p)
> > +{
> > +    libxl__json_map_node *node = NULL;
> > +    flexarray_t *maps = NULL;
> > +    int i, size;
> > +    libxl_key_value_list kvl;
> > +
> > +    if (!libxl__json_object_is_map(o))
> > +        return -1;
> > +
> > +    maps = o->u.map;
> > +    size = maps->count * 2;
> > +    kvl = *p = calloc(size, sizeof(char *));
> > +    if (!kvl)
> > +        return -1;
> > +
> > +    memset(kvl, 0, sizeof(char *) * size);
> > +
> > +    for (i = 0; i < maps->count; i++) {
> > +        int idx = i * 2;
> > +        if (flexarray_get(maps, i, (void**)&node) != 0)
> > +            return -1;
> > +        assert(libxl__json_object_is_string(node->obj) ||
> > +               libxl__json_object_is_null(node->obj));
> 
> I think we may need to be mindful of potentially malicious json code
> which a toolstack is feeding to these parser functions, don't we?
> 
> IOW asserting and crashing the entire daemon would be unfortunate.
> 

I think "return -1" will be more appropriate in this case.

> > +        kvl[idx]   = strdup(node->map_key);
> > +        if (libxl__json_object_is_string(node->obj))
> > +            kvl[idx+1] = strdup(node->obj->u.string);
> > +        else
> > +            kvl[idx+1] = NULL;
> > +    }
> > +
> > +    assert(i == maps->count);
> 
> Too defensive again.
> 

Ack.

> > +
> > +    return 0;
> > +}
> > +
> >  yajl_gen_status libxl_string_list_gen_json(yajl_gen hand, 
> > libxl_string_list *pl)
> >  {
> >      libxl_string_list l = *pl;
> > @@ -201,6 +343,27 @@ out:
> >      return s;
> >  }
> >  
> > +int libxl_hwcap_parse_json(libxl_ctx *ctx, const libxl__json_object *o,
> > +                           libxl_hwcap *p)
> > +{
> > +    int i;
> > +
> > +    if (!libxl__json_object_is_array(o))
> > +        return -1;
> 
> I jut noticed the error handling here, but I'm sure it was the same for
> all the preceding. libxl functions should return an ERROR_* code and not
> -1 unless you have a good reason not to.
> 

OK. Will change to that.

Wei.

> Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel