From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from list by lists.gnu.org with archive (Exim 4.90_1)
	id 1iZUFV-0006yq-GK
	for mharc-grub-devel@gnu.org; Tue, 26 Nov 2019 01:22:57 -0500
Received: from eggs.gnu.org ([2001:470:142:3::10]:60701)
 by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from <ps@pks.im>)
 id 1iZUFR-0006wK-O0
 for grub-devel@gnu.org; Tue, 26 Nov 2019 01:22:55 -0500
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
 (envelope-from <ps@pks.im>) id 1iZUFP-00047J-Iw
 for grub-devel@gnu.org; Tue, 26 Nov 2019 01:22:52 -0500
Received: from out1-smtp.messagingengine.com ([66.111.4.25]:44871)
 by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32)
 (Exim 4.71) (envelope-from <ps@pks.im>) id 1iZUFO-00045w-Vz
 for grub-devel@gnu.org; Tue, 26 Nov 2019 01:22:51 -0500
Received: from compute1.internal (compute1.nyi.internal [10.202.2.41])
 by mailout.nyi.internal (Postfix) with ESMTP id A6206226C4;
 Tue, 26 Nov 2019 01:22:48 -0500 (EST)
Received: from mailfrontend2 ([10.202.2.163])
 by compute1.internal (MEProxy); Tue, 26 Nov 2019 01:22:48 -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=tq0KTxim+z7ypt7dSoT7e6TM4JG
 lbggJG4oMcx9IYOc=; b=DeEFepAddZjCa6+CeBWEJHHWsVZfdw+rHRngiKdNjIB
 +Vk0Z37TWcCLcf75ZL7gNMxPi9iQzQqZuaZTbvJg8kFlKeDhLz8TMGR8nk/hr8Kh
 fKk+5Lnl88nd7wqpviuIEEES7kApj6ilXzmChJGHZDZbMNyIjBji5JicFXx/pQhQ
 Cm/6CwxKQAA0DHJoa5L/Jmd8vFC7+pTq80DbUcJgd/N/x2vqL0R78zggr0cHCJwQ
 JmEMm2IbSR5ygKk052GubF21z/S/QE/yXmXSr27Z8LODlOmWVdGFo+ltxVhZtHFY
 zoU7h88cyAAcWQXKrWsPkxpz/CIjfTTUFm52NEsqOPQ==
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=tq0KTx
 im+z7ypt7dSoT7e6TM4JGlbggJG4oMcx9IYOc=; b=KjF7WEAKW05F2B101KQKcm
 XMdtRmXOrv613eZVfxcZ0qFUwaf9IfbHvgREOA3lX6zDYWIVt68OUV0X718G3CVQ
 alQbc1kTJj9vpdtzM5SY5Znuj1jTExTDW2lKWCj05iUmN2adtATmoiTvTuC2c0Lw
 8/ccc2RoClBNi1pBoJbszV4z41XIpoYGg+xeQCgb25dHO9jPEp+JP7j9oqV9fI2f
 xuE8XtAJ+EfClBQOwwFeZ/Ga3B8gu+kfyuyTtUL2e5jEmWEUU9gtNqEnL804eyqd
 QJcvhZ/jnAF27DqLl/rlOdHv7UFBQJ+sDTSSNZ+kVMZPU3yD9zShTK10eruYXZYQ
 ==
X-ME-Sender: <xms:t8TcXYkhcL-1thxa8ExFzlNx6smS2qqV8p3nSFnpLVqpf--Ea8txxw>
X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgedufedrudeivddgleegucetufdoteggodetrfdotf
 fvucfrrhhofhhilhgvmecuhfgrshhtofgrihhlpdfqfgfvpdfurfetoffkrfgpnffqhgen
 uceurghilhhouhhtmecufedttdenucesvcftvggtihhpihgvnhhtshculddquddttddmne
 cujfgurhepfffhvffukfhfgggtuggjsehgtderredttddvnecuhfhrohhmpefrrghtrhhi
 tghkucfuthgvihhnhhgrrhguthcuoehpshesphhkshdrihhmqeenucfkphepjeejrdduud
 drvdegiedrudeiudenucfrrghrrghmpehmrghilhhfrhhomhepphhssehpkhhsrdhimhen
 ucevlhhushhtvghrufhiiigvpedt
X-ME-Proxy: <xmx:t8TcXZu17yPHVU6DRcqRb3eEt7cKSQ4NZ00SHtHmcq8tsTXOsY29OA>
 <xmx:t8TcXbzGe3C40_n4ifoj5XvJmyw2k4dJPm0KIwnjOAgjiH_7Qhy1ew>
 <xmx:t8TcXUzvYaC5_59eaIBuSNBQxGGxjwkK0goa-SS90fBaVKhCq3jyUw>
 <xmx:uMTcXVUAim8Eszv_c5EDhtzTqx3AuAKu3x2RB-2B95-clgBllg06YQ>
Received: from NSJAIL (x4d0bf6a1.dyn.telefonica.de [77.11.246.161])
 by mail.messagingengine.com (Postfix) with ESMTPA id 491E83060060;
 Tue, 26 Nov 2019 01:22:47 -0500 (EST)
Received: from localhost (<unknown> [10.192.0.11])
 by NSJAIL (OpenSMTPD) with ESMTPSA id ccf37567
 (TLSv1.3:TLS_AES_256_GCM_SHA384:256:NO); 
 Tue, 26 Nov 2019 06:22:45 +0000 (UTC)
Date: Tue, 26 Nov 2019 07:22:45 +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: <20191126062245.GA2647@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>
 <20191115123618.GA2733@ncase>
 <20191118144505.wkgvcsexmhoolvn2@tomti.i.net-space.pl>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha256;
 protocol="application/pgp-signature"; boundary="envbJBWh7q8WU6mo"
Content-Disposition: inline
In-Reply-To: <20191118144505.wkgvcsexmhoolvn2@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: 66.111.4.25
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: Tue, 26 Nov 2019 06:22:55 -0000


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

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 wrot=
e:
> > > > > > +  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->siz=
e?
> > > > > 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 >=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.
>=20
> 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.
>=20
> 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

--envbJBWh7q8WU6mo
Content-Type: application/pgp-signature; name="signature.asc"

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

iQIzBAABCAAdFiEEtmscHsieVjl9VyNUEXxntp6r8SwFAl3cxLMACgkQEXxntp6r
8SyFGw/8CyPKCph4EkTrdp+dB6oK1OFBJygIe402Away7LBEMIB3xRD3hyu+va6C
S4Ud9EHTsgRiuuYAL2UObIdiEZ/21TaJFEsNCYC27bmgTuvC7kqAklyFQ8Zh7dM6
bDeoSzyJp+FE6gsO8noOBufd3l2x5RJpdXvq2sx0JFFPg2BUvaYH9n/wWsAsA82d
aWowcJDxMtZBn1d02AxDoG5cdfovMFPjxz0jFFb/zHhRrCpxJVZ/0r6D5tR5HRjP
cN2+PIntK3n6dyDaHUfnS/SMcAcLYRZv4nNeeSmXXG7N6Ec7rXRUchpYc+7mY4FT
0o065Kcqt7bWBZzNoCpjRb/ADt57MiTDwU0evadwAgjDbcRshi3x4BI1N6wZFl99
vfv4ppioMx0BKZbrieVLWFJNlEBxrzeMYaC9vqWO2bNJ3ZqAIWUBLJzoR2FAUOm+
Lv+XExMVPUIpTvsGNZTac0HG7/JvFy1+FzQ3Rdg5WoE24f6UhZjQggHDiCPiqimW
cSJXRoRfW1rNigfcEeRAhY8Yogbd85nxQuVuLHS9vhpMwLMdntvvvsxZP1DgzUN3
yO65AwgMjfyItMofR8DO+G0DfOMRMTSRWvxjU0RMK0mDIQsH4CAiTn5tkeQPpZWT
MYLLyHyY8GcdlzSJVUAcxmM6vH3nGSSzRJAOgztMu/s+++77ViI=
=ouan
-----END PGP SIGNATURE-----

--envbJBWh7q8WU6mo--