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

[Patrick Steinhardt <ps@pks.im>]

[-- Attachment #1: Type: text/plain, Size: 3828 bytes --]

On Mon, Nov 18, 2019 at 03:45:05PM +0100, Daniel Kiper wrote:
> 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:
> > > > > > +  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

One last pushback from my side, if you still disagree I'll change
it to move this patch seires forward.

So I dug back in history, and the original `size` field was
introduced with commit 4e869f7 (Complex types (objects and
arrays) now have also size - number of child elements,
2010-12-28). Even back then, there was not a single location
where it was assigned a negative value and nowadays there isn't
either. I've skipped through history since then and couldn't find
any instance where it could have been assigned a negative value,
so we can assume that for the last 9 years there wasn't any
reason for a signed integer as field.

So how about we keep the `grub_size_t` field, but introduce a
check in both `grub_json_getsize` and `grub_json_getchild` to
verify that the current token does not have negative size,
raising an error if it does. Like this, we are able to catch
weird behaviour of jsmn if we were to upgrade jsmn.h without
noticing the change in semantics, but still retain the saner or
at least more obvious API with `grub_size_t`.

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]