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

[Patrick Steinhardt]

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.

Patrick

signature.asc
Description: PGP signature