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