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

[Daniel Kiper]

On Fri, Nov 15, 2019 at 01:36:18PM +0100, Patrick Steinhardt wrote:
> On Fri, Nov 15, 2019 at 12:56:40PM +0100, Daniel Kiper wrote:
> > On Thu, Nov 14, 2019 at 02:12:39PM +0100, Patrick Steinhardt wrote:
> > > On Thu, Nov 14, 2019 at 01:37:18PM +0100, Daniel Kiper wrote:
> > > > On Wed, Nov 13, 2019 at 02:22:34PM +0100, Patrick Steinhardt wrote:
> [snip]
> > > > > +    }
> > > > > +  return GRUB_JSON_UNDEFINED;
> > > > > +}
> > > > > +
> > > > > +grub_err_t
> > > > > +grub_json_getchild(grub_json_t *out, const grub_json_t *parent, 
> > > > > grub_size_t n)
> > > > > +{
> > > > > +  jsmntok_t *p = &((jsmntok_t *)parent->tokens)[parent->idx];
> > > >
> > > > Should not you check parent for NULL first? Same thing for functions
> > > > above and below.
> > >
> > > I'm not too sure about this. Calling with a NULL pointer wouldn't
> > > make any sense whatsoever, as you cannot get anything from
> > > nothing. It would certainly be the more defensive approach to
> > > check for NULL here, and if that's GRUB's coding style then I'm
> > > fine with that. If not, I'd say we should just crash with NPE to
> > > detect misuse of the API early on.
> >
> > You are not able to predict all cases. So, I am leaning toward to have
> > checks for NULL than crashing GRUB randomly because we have not checked
> > for it.
>
> Fair enough, will do.
>
> > > > > +  grub_size_t offset = 1;
> > > > > +
> > > > > +  if (n >= (unsigned) p->size)
> > > >
> > > > Should not you cast to grub_size_t? Or should n be type of p->size?
> > > > Then you would avoid a cast.
> > >
> > > I find the choice of `int` quite weird on jsmn's side: there's
> > > not a single place where the size field is getting a negative
> > > assignment. So if you ask me, `size_t` or even just `unsigned
> > > int` would have been the better choice here, which is why I just
> > > opted for `grub_size_t` instead in our wrapping interface.
> >
> > If jsmn is using something "intish" then I think that we should use
> > grub_ssize_t. Even if variables of a such type does not get negative
> > values right now.
> >
> > > But you're right, I should cast to `grub_size_t` instead of
> > > `unsigned` to make it a bit clearer here.
> >
> > ...grub_ssize_t then?
>
> The question is whether we want a near 1:1 mapping here or
> something that makes sense (even though making sense is
> subjective). I tend towards the latter one of doing the right
> thing, mostly because I cannot make sense of a negative value
> here. For an array, getting the -1'th child doesn't make sense,
> so we'd have to extend the current check like following:
>
>     if (n < 0 || n >= p->size)
>         return -1;
>
> If not checking for `n < 0`, we'd iterate children until `n`
> overflows and reaches `-1` eventually, which would result in
> out-of-bounds reads. So as we currently cannot make any sense of
> that value, I tend to just say that `grub_size_t` is the correct
> type here even though it mismatches what jsmn is doing.
>
> That being said, we could certainly define what a negative value
> would do, like e.g. `-1` would get the first child from the rear
> of the array. But that wouldn't match what jsmn uses `size` for,
> either.

In general I agree with you. However, if jsmn uses ints for indexes
then I would do the same in the GRUB. Otherwise, if jsmn starts using
negative values for something we can be badly surprised. And of course
you can ask jsmn author why he decided to use ints for indexes. I am
also interested in hearing why he did it.

Daniel