From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from list by lists.gnu.org with archive (Exim 4.90_1)
	id 1iVapy-0003Er-UD
	for mharc-grub-devel@gnu.org; Fri, 15 Nov 2019 07:36:30 -0500
Received: from eggs.gnu.org ([2001:470:142:3::10]:45670)
 by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from <ps@pks.im>)
 id 1iVapv-00038U-2h
 for grub-devel@gnu.org; Fri, 15 Nov 2019 07:36:28 -0500
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
 (envelope-from <ps@pks.im>) id 1iVapt-0004QC-MK
 for grub-devel@gnu.org; Fri, 15 Nov 2019 07:36:26 -0500
Received: from wout5-smtp.messagingengine.com ([64.147.123.21]:39167)
 by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32)
 (Exim 4.71) (envelope-from <ps@pks.im>) id 1iVapt-0004PI-0k
 for grub-devel@gnu.org; Fri, 15 Nov 2019 07:36:25 -0500
Received: from compute1.internal (compute1.nyi.internal [10.202.2.41])
 by mailout.west.internal (Postfix) with ESMTP id C17545CA;
 Fri, 15 Nov 2019 07:36:21 -0500 (EST)
Received: from mailfrontend1 ([10.202.2.162])
 by compute1.internal (MEProxy); Fri, 15 Nov 2019 07:36:22 -0500
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=pks.im; h=date
 :from:to:cc:subject:message-id:references:mime-version
 :content-type:in-reply-to; s=fm2; bh=euncmovfoTT4qGereE5x1xG6Vq0
 CpRJkYcCr8xygxxQ=; b=zP1nF+263wtkH86sw1KBm1skl6uzeSQldkXjiC7v5e2
 PMMBuW9aAke4MrMlbxOW0oPQaxsSE4ZSXQQRqE1TxgUObrvsMQWc0JuvvFRz3d7k
 qXB3XmMTe/z3+5jdNwRpicvYSeJ0gL+7c1RePpVFt6jEZndpkU2qe9SUPq2nZd/1
 aDzPgn3v2k5XWJMP9Y45GXvoUSNuur7YCgrcvZf2+Zs06C0Bs3IlSJJi3iU6JBm4
 9EWM9uh+bSVvApL85DlHX26sAGWp3cAmdSZPrgA8Nr9K71wu/+6lTqYAEB1LIPCd
 xCxdqdYrSWb8Kd/g0QsG4KbuLdd+zds3ksDg3ipfqDg==
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=
 messagingengine.com; h=cc:content-type:date:from:in-reply-to
 :message-id:mime-version:references:subject:to:x-me-proxy
 :x-me-proxy:x-me-sender:x-me-sender:x-sasl-enc; s=fm1; bh=euncmo
 vfoTT4qGereE5x1xG6Vq0CpRJkYcCr8xygxxQ=; b=igz0Y9qVmbzlzY/Li4NtJp
 kKN1RUAAUSvqfQx+7cTG/fk7+LgkYMFHCNvKjhDo4SWBduX547Rj7vNs5xwYeczw
 dzr2LmMxTcSoycIBg+7Bf68wimxY0AFUz3ENdCMPbSMXHbteLY1snmQXzclHprVm
 CwkMvXDpofPEHYojMgkPIgCFZqJCnv7K+7wn85/5QMl91KEST5ebf1ogb/ovexs7
 XtKHPfZEKVWjuupKX4ZI1TEIwjTId1F7/5BycqCZ/wRzdVkz291KHX1z5xhu+ZZY
 40zRe9bsPBEKXIi5gOYAtFVby1wr//z0BP0Tr2rZEy8VcdwdjU9z8U4QKQSEH0NQ
 ==
X-ME-Sender: <xms:xJvOXQrhN-n6MFh6OlnuS1Xf6yzMnRqRPml1zkT3AVb4o0ofU6HeGQ>
X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgedufedrudefhedggedvucetufdoteggodetrfdotf
 fvucfrrhhofhhilhgvmecuhfgrshhtofgrihhlpdfqfgfvpdfurfetoffkrfgpnffqhgen
 uceurghilhhouhhtmecufedttdenucesvcftvggtihhpihgvnhhtshculddquddttddmne
 cujfgurhepfffhvffukfhfgggtuggjsehgtderredttddvnecuhfhrohhmpefrrghtrhhi
 tghkucfuthgvihhnhhgrrhguthcuoehpshesphhkshdrihhmqeenucfkphepkeelrdduge
 drvdefiedrvddtieenucfrrghrrghmpehmrghilhhfrhhomhepphhssehpkhhsrdhimhen
 ucevlhhushhtvghrufhiiigvpedt
X-ME-Proxy: <xmx:xJvOXQXbg5g5Zwx0-xn7ovs_jR2AYXW_VIr5JvICc9TKGfqwKBUgpw>
 <xmx:xJvOXcKe_2Cg66EcRJI0_F7YKDlerUhUnMNb_IKFcEWMC-gfEDaW4A>
 <xmx:xJvOXb8BiTDfaxwpiej4u-pEt1fk_B3PoIepePtjh1nXXXFlXlBWug>
 <xmx:xZvOXaxzW-59uBC90z5COA_lsA7Db51OvIZk7oiDYkLR5jJbXPyWQg>
Received: from NSJAIL (x590eecce.dyn.telefonica.de [89.14.236.206])
 by mail.messagingengine.com (Postfix) with ESMTPA id 3CFA98005A;
 Fri, 15 Nov 2019 07:36:20 -0500 (EST)
Received: from localhost (<unknown> [10.192.0.11])
 by NSJAIL (OpenSMTPD) with ESMTPSA id 9d8d263d
 (TLSv1.3:TLS_AES_256_GCM_SHA384:256:NO); 
 Fri, 15 Nov 2019 12:36:16 +0000 (UTC)
Date: Fri, 15 Nov 2019 13:36:18 +0100
From: Patrick Steinhardt <ps@pks.im>
To: Daniel Kiper <dkiper@net-space.pl>
Cc: grub-devel@gnu.org, Max Tottenham <mtottenh@akamai.com>
Subject: Re: [PATCH v3 2/6] json: Implement wrapping interface
Message-ID: <20191115123618.GA2733@ncase>
References: <cover.1572717208.git.ps@pks.im> <cover.1573651222.git.ps@pks.im>
 <680b5add59a40fbe3dc77f8ef5eb826849fe0d37.1573651222.git.ps@pks.im>
 <20191114123718.bzps5ucvdqvxivcu@tomti.i.net-space.pl>
 <20191114131239.GA3036@ncase>
 <20191115115640.y2ifpcoflhlkdatq@tomti.i.net-space.pl>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha256;
 protocol="application/pgp-signature"; boundary="BOKacYhQ+x31HxR3"
Content-Disposition: inline
In-Reply-To: <20191115115640.y2ifpcoflhlkdatq@tomti.i.net-space.pl>
X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic]
 [fuzzy]
X-Received-From: 64.147.123.21
X-BeenThere: grub-devel@gnu.org
X-Mailman-Version: 2.1.23
Precedence: list
List-Id: The development of GNU GRUB <grub-devel.gnu.org>
List-Unsubscribe: <https://lists.gnu.org/mailman/options/grub-devel>,
 <mailto:grub-devel-request@gnu.org?subject=unsubscribe>
List-Archive: <https://lists.gnu.org/archive/html/grub-devel>
List-Post: <mailto:grub-devel@gnu.org>
List-Help: <mailto:grub-devel-request@gnu.org?subject=help>
List-Subscribe: <https://lists.gnu.org/mailman/listinfo/grub-devel>,
 <mailto:grub-devel-request@gnu.org?subject=subscribe>
X-List-Received-Date: Fri, 15 Nov 2019 12:36:29 -0000


--BOKacYhQ+x31HxR3
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

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, gr=
ub_size_t n)
> > > > +{
> > > > +  jsmntok_t *p =3D &((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.
>=20
> 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 =3D 1;
> > > > +
> > > > +  if (n >=3D (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.
>=20
> 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.
>=20
> > But you're right, I should cast to `grub_size_t` instead of
> > `unsigned` to make it a bit clearer here.
>=20
> ...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 >=3D 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

--BOKacYhQ+x31HxR3
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQIzBAABCAAdFiEEtmscHsieVjl9VyNUEXxntp6r8SwFAl3Om8EACgkQEXxntp6r
8Szoxw//XWYMV1uzXL8/HpMabCZdGV5jnB9FQfZIWAdf4gXM199rhe+ht/xR4pMb
ln6HqRCxnX7JaS+cLMWQiRpogIeJ4qLT0lccOUQ1/5A+hCpIYUkPZnSZnWV6JO+2
J2RdV9YKbLDFDeAvuIkUlBinZXfgO68onl972QMr1iqa3WVPG7FOSZ+XU2J1fvWn
/r2yqNa1t8cj1KN8pEyUUJoCAHsICQoAll0TgdqjMXPmh5/gq3pEFOSDQmkZN0Io
bbJ8LMQOsPaPnyxH5KiuvRp1UtWFvSXgellUXTnw0TLDfob/e7bUvNpkG/oA0VNK
hh39iZSbhioJnvGU+pWzKytKEJa1DkW8O4ZrbyM/DIGOn87yuT0fnigPM2IpeI+t
dgKBh8a5JJ5KXcjIJ3/NI7HcA9P1zLf6xfr6EgbTumDLaQy5W4Pt4emjMQBfYTzd
U/IxTtkezqlT2Y5+/t51RvcpGwL+Rj7p5CF5v1rxPGDgPQ67NES772s44kWWZOux
QbhNyhLIneUJR+xfClehfiNW6PsQmzYXi3ZL28Iwrl+cjDJKICjpXLaY7BwCdgu2
luR2nAI1ST51PVQQD5W4UZJjfsgpDS8hxY00ugHMpmQKIt6uxiexuapFRlldoT2C
MKVzjzAoXt2dgSoUm1648SKLIt+qljhX3m1yqpD47M1nnmESPqI=
=wNAV
-----END PGP SIGNATURE-----

--BOKacYhQ+x31HxR3--