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

[Daniel Kiper]

On Tue, Nov 05, 2019 at 02:12:13PM +0100, Patrick Steinhardt wrote:
> On Tue, Nov 05, 2019 at 12:54:38PM +0000, Max Tottenham via Grub-devel wrote:
> > On 11/05, Patrick Steinhardt wrote:
> > > +grub_err_t
> > > +grub_json_parse (grub_json_t **out, const char *string, grub_size_t 
> > > string_len)
> > > +{
> > > +  grub_size_t ntokens = 128;
> > > +  grub_json_t *json = NULL;
> > > +  jsmn_parser parser;
> > > +  grub_err_t err;
> > > +  int jsmn_err;
> > > +
> > > +  json = grub_zalloc (sizeof (*json));
> > > +  if (!json)
> > > +    return GRUB_ERR_OUT_OF_MEMORY;
> > > +  json->idx = 0;
> > > +  json->string = grub_strndup (string, string_len);
> >
> > I'm assuming the idea here is to ensure that the lifetime of the
> > returned grub_json_t doesn't depend on the lifetime of the input string?
> >
> > This concerns me a little bit, from a quick scan - given your usage of
> > grub_json_parse in the luks2 module it appears that json_header (the
> > underlying input string) will always outlive the json object.
> >
> > In which case, as there are no other existing users, we may want to make
> > the API contract "The lifetime/validity of the returned object is bound
> > by that of the input string", if we can comment/document that clearly
> > then there is no need for this extra allocation and we can simply do:
> >
> >     json->string = string;
> >
> >
> > Thoughts?
>
> Thing is that the parser needs to modify the string. This is kind
> of an optimization in order to avoid the need for calling
> `strdup` when returning a string, e.g. in `grub_json_getstring`.
> We just modify the underlying string to have a '\0' in the right
> place and then return it directly. It might feel weird to callers
> that the parsed string is getting modified under their hands,
> which is why I decided to just `strdup` it.
>
> That being said, I don't have any hard feelings about this. I'm
> fine with avoiding the `grub_strndup`, and if it's documented so
> that callers know that it's getting modified then it should be
> safe enough.
>
> > > +  if (!json->string)
> > > +    {
> > > +      err = GRUB_ERR_OUT_OF_MEMORY;
> > > +      goto out;
> > > +    }
> > > +
> > > +  jsmn_init(&parser);
> > > +
> > > +  while (1)
> > > +    {
> > > +      json->tokens = grub_realloc (json->tokens, sizeof (jsmntok_t) * 
> > > ntokens);
> >
> > According to the docs, calling jsmn_parse with tokens = NULL will return
> > the number of tokens required to properly parse the JSON, without trying
> > to 'allocate' them in the token buffer (with the 'ntokens' parameter
> > being ignored)
>
> jsmn's examples curiously have the same realloc-loop that I do.
> I'll check again and definitely convert to what you propose if
> supported.

In general if allocation functions are not called thousands times and
do not alloc tons of memory then I think that data duplication gives
us more flexibility here.

> > > +      if (!json->tokens)
> > > + {
> > > +   err = GRUB_ERR_OUT_OF_MEMORY;
> > > +   goto out;
> > > + }
> > > +
> > > +      jsmn_err = jsmn_parse (&parser, string, string_len, json->tokens, 
> > > ntokens);
> > > +      if (jsmn_err >= 0)
> > > + break;
> > > +      if (jsmn_err != JSMN_ERROR_NOMEM)
> > > + {
> > > +   err = GRUB_ERR_BAD_ARGUMENT;
> > > +   goto out;
> > > + }
> > > +
> > > +      ntokens <<= 1;
> > > +    }
> > > +
> > > +  err = GRUB_ERR_NONE;
> > > +  *out = json;
> > > +out:
> > > +  if (err && json)
> > > +    {
> > > +      grub_free (json->string);
> > > +      grub_free (json->tokens);
> >
> > Irrespective of the above - these two free's on
> > json->string/json->tokens could be called on a NULL agrgment (e.g. if
> > their initial allocation fails), I don't know whether it's common
> > practice to allow grub_free (NULL), but I would encourage checking
> > whether these are actually allocated before attempting to free them.
>
> free(3P) explicitly mandates that it may be called with a `NULL`
> pointer as argument, in which case it will just do nothing.
> `grub_free` is in fact doing the same, so we should be good here.

Yeah, that is correct/acceptable free()/grub_free() usage.

Daniel