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
_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
