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

[Patrick Steinhardt]

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.

> > +      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.

Patrick

signature.asc
Description: PGP signature