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