Re: [PATCH v5 2/6] json: Implement wrapping interface

[Patrick Steinhardt]

On Fri, Nov 29, 2019 at 04:34:58PM +0100, Daniel Kiper wrote:
> On Fri, Nov 29, 2019 at 07:51:46AM +0100, Patrick Steinhardt wrote:
[snip]
> > +grub_err_t
> > +grub_json_getsize (grub_size_t *out, const grub_json_t *json)
> > +{
> > +  int size;
> 
> I hope that ((jsmntok_t *)json->tokens)() returns int...

Yeah, the JSON token's size is stored as an int.

> > +  if (!json)
> > +    return GRUB_ERR_BAD_ARGUMENT;
> 
> Hmmm... I am looking at this and I have a feeling that we are not
> consistent. If we want to be consistent we should check "out" for NULL
> too. Same below. However, this complicates the code needlessly. How jsmn
> copes with NULL? Does it care at all? Maybe we should just drop these
> checks and state clearly somewhere in docs/comments that the caller must
> provide valid pointers. Full stop!

jsmn doesn't care at all, as it doesn't provide any accessing
functions but the parsing code, only. Thus every NULL pointer
handling needs to be done by the user.

I'm fine with just saying "Give us valid pointers" as it was in
my initial design.

> > +  *out = (size_t) size;
> 
> s/size_t/grub_size_t/
> 
> However, TBH I would prefer grub_ssize_t here...

Instead of the error return code or in addition to that? It would
require the caller to always check the returned size for negative
values.

> > +static grub_err_t
> > +get_value (grub_json_type_t *out_type, const char **out_string, const 
> > grub_json_t *parent, const char *key)
> > +{
> > +  const grub_json_t *p = parent;
> > +  grub_json_t child;
> > +  grub_err_t ret;
> > +  jsmntok_t *tok;
> > +
> > +  if (!parent)
> > +    return GRUB_ERR_BAD_ARGUMENT;
> > +
> > +  if (key)
> > +    {
> > +      ret = grub_json_getvalue (&child, parent, key);
> > +      if (ret)
> > +   return ret;
> > +      p = &child;
> > +    }
> > +
> > +  tok = &((jsmntok_t *) p->tokens)[p->idx];
> > +  p->string[tok->end] = '\0';
> 
> Are you sure that tok is never NULL?

Yeah, it cannot be as we're directly taking the address after
indexing into the array. What _could_ happen is that somebody
provided an invalid parent with a bogus index, but that would
again only be possible by misuse of the API.

Patrick

signature.asc
Description: PGP signature