From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from list by lists.gnu.org with archive (Exim 4.90_1) id 1iRydF-0004vz-PZ for mharc-grub-devel@gnu.org; Tue, 05 Nov 2019 08:12:25 -0500 Received: from eggs.gnu.org ([2001:470:142:3::10]:36495) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1iRydC-0004t6-HX for grub-devel@gnu.org; Tue, 05 Nov 2019 08:12:24 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1iRydB-0002NM-2E for grub-devel@gnu.org; Tue, 05 Nov 2019 08:12:22 -0500 Received: from wout2-smtp.messagingengine.com ([64.147.123.25]:60897) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1iRydA-0002LA-DS for grub-devel@gnu.org; Tue, 05 Nov 2019 08:12:21 -0500 Received: from compute1.internal (compute1.nyi.internal [10.202.2.41]) by mailout.west.internal (Postfix) with ESMTP id B4C12456; Tue, 5 Nov 2019 08:12:17 -0500 (EST) Received: from mailfrontend2 ([10.202.2.163]) by compute1.internal (MEProxy); Tue, 05 Nov 2019 08:12:17 -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=vI42Mwbnz06T2gEQsluLgzaMca5 zqT7+Tye5Q7WrBmA=; b=EcZBGLqMdN/zBGq4hOLqL9HuK6wMjd6nMiY00ohG9ZP by761ulIs66E2eDqtwMv8gUNh+ZUPcEWbptlFCZr3Z7Sg2FMcG7/3CPNDvG4OiJW WXX8PggMMI/BXegmfQDX6ACWTq8lUVrlBSPcNW/prqCyrOduXKSzCh9At5d+Y6xo bwuzGa0In3YwkcMqh+JYBxUSh4knqDMIzaU5ieQ9eTcLvJyYzxQ0yiAAr0LJnx4i Mucpt77QX+Z8PfGTvykh98/UEhpaMC7cIKimoEWborVazHVMDIvIZttEmOgCvZA1 vG9jDTcazIpBxVNXCFeL1jMO5Y1cn6pj8xe1pYUGyjw== 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=vI42Mw bnz06T2gEQsluLgzaMca5zqT7+Tye5Q7WrBmA=; b=GcB6UpuXHj0GohYdp52n6B pMiJgHPN5hdua7zxCiDl8q6j26u1KVNdZqH6bjkKWInAfRZaT+JBHYf8Nlusx0WN UFn4v6l70GEd1jSH6MZqf530csbmaLIKYg0BsRQHHin/eIouGVodbPpb9l2TEeJI 55TgbEZC7rPEbtKfZxeJCWp/fH6zgp7ULyOrXl0ICRj599N4Oj1kpRHCMJSnJClx 18nVsKmTsUoW1XrP0d/L3bD73TPBQVpBqyvLkDxKIIS7hBVdg2mkC4pJyCwIa5DW mIpqoK/o/LGMTvaT7EKhx/UJp7gw3SBa+28kJK3O7RTzAs2N0nabVfFxGmT26AmQ == X-ME-Sender: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgedufedrudduhedghedtucetufdoteggodetrfdotf fvucfrrhhofhhilhgvmecuhfgrshhtofgrihhlpdfqfgfvpdfurfetoffkrfgpnffqhgen uceurghilhhouhhtmecufedttdenucesvcftvggtihhpihgvnhhtshculddquddttddmne cujfgurhepfffhvffukfhfgggtuggjsehgtderredttddvnecuhfhrohhmpefrrghtrhhi tghkucfuthgvihhnhhgrrhguthcuoehpshesphhkshdrihhmqeenucfkphepjeejrdduud drudeliedruddtkeenucfrrghrrghmpehmrghilhhfrhhomhepphhssehpkhhsrdhimhen ucevlhhushhtvghrufhiiigvpedt X-ME-Proxy: Received: from NSJAIL (x4d0bc46c.dyn.telefonica.de [77.11.196.108]) by mail.messagingengine.com (Postfix) with ESMTPA id 41E3C3060057; Tue, 5 Nov 2019 08:12:16 -0500 (EST) Received: from localhost (10.192.0.11 [10.192.0.11]) by NSJAIL (OpenSMTPD) with ESMTPSA id d9554a89 (TLSv1.3:TLS_AES_256_GCM_SHA384:256:NO); Tue, 5 Nov 2019 13:12:14 +0000 (UTC) Date: Tue, 5 Nov 2019 14:12:13 +0100 From: Patrick Steinhardt To: The development of GNU GRUB Cc: Max Tottenham Subject: Re: [PATCH v2 2/6] json: Implement wrapping interface Message-ID: <20191105131213.GA177916@ncase> References: <20191105125438.GA20442@lon-lp55b.london.corp.akamai.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="PNTmBPCT7hxwcZjr" Content-Disposition: inline In-Reply-To: <20191105125438.GA20442@lon-lp55b.london.corp.akamai.com> X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] [fuzzy] X-Received-From: 64.147.123.25 X-BeenThere: grub-devel@gnu.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: The development of GNU GRUB List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 05 Nov 2019 13:12:24 -0000 --PNTmBPCT7hxwcZjr Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Tue, Nov 05, 2019 at 12:54:38PM +0000, Max Tottenham via Grub-devel wrot= e: > On 11/05, Patrick Steinhardt wrote: > > +grub_err_t > > +grub_json_parse (grub_json_t **out, const char *string, grub_size_t st= ring_len) > > +{ > > + grub_size_t ntokens =3D 128; > > + grub_json_t *json =3D NULL; > > + jsmn_parser parser; > > + grub_err_t err; > > + int jsmn_err; > > + > > + json =3D grub_zalloc (sizeof (*json)); > > + if (!json) > > + return GRUB_ERR_OUT_OF_MEMORY; > > + json->idx =3D 0; > > + json->string =3D grub_strndup (string, string_len); >=20 > I'm assuming the idea here is to ensure that the lifetime of the > returned grub_json_t doesn't depend on the lifetime of the input string? >=20 > This concerns me a little bit, from a quick scan - given your usage of > grub_json_parse in the luks2 module it appears that json_header (the > underlying input string) will always outlive the json object. >=20 > In which case, as there are no other existing users, we may want to make > the API contract "The lifetime/validity of the returned object is bound > by that of the input string", if we can comment/document that clearly > then there is no need for this extra allocation and we can simply do: >=20 > json->string =3D string; >=20 >=20 > Thoughts? Thing is that the parser needs to modify the string. This is kind of an optimization in order to avoid the need for calling `strdup` when returning a string, e.g. in `grub_json_getstring`. We just modify the underlying string to have a '\0' in the right place and then return it directly. It might feel weird to callers that the parsed string is getting modified under their hands, which is why I decided to just `strdup` it. That being said, I don't have any hard feelings about this. I'm fine with avoiding the `grub_strndup`, and if it's documented so that callers know that it's getting modified then it should be safe enough. > > + if (!json->string) > > + { > > + err =3D GRUB_ERR_OUT_OF_MEMORY; > > + goto out; > > + } > > + > > + jsmn_init(&parser); > > + > > + while (1) > > + { > > + json->tokens =3D grub_realloc (json->tokens, sizeof (jsmntok_t) = * ntokens); >=20 > According to the docs, calling jsmn_parse with tokens =3D NULL will return > the number of tokens required to properly parse the JSON, without trying > to 'allocate' them in the token buffer (with the 'ntokens' parameter > being ignored) jsmn's examples curiously have the same realloc-loop that I do. I'll check again and definitely convert to what you propose if supported. > > + if (!json->tokens) > > + { > > + err =3D GRUB_ERR_OUT_OF_MEMORY; > > + goto out; > > + } > > + > > + jsmn_err =3D jsmn_parse (&parser, string, string_len, json->toke= ns, ntokens); > > + if (jsmn_err >=3D 0) > > + break; > > + if (jsmn_err !=3D JSMN_ERROR_NOMEM) > > + { > > + err =3D GRUB_ERR_BAD_ARGUMENT; > > + goto out; > > + } > > + > > + ntokens <<=3D 1; > > + } > > + > > + err =3D GRUB_ERR_NONE; > > + *out =3D json; > > +out: > > + if (err && json) > > + { > > + grub_free (json->string); > > + grub_free (json->tokens); >=20 > Irrespective of the above - these two free's on > json->string/json->tokens could be called on a NULL agrgment (e.g. if > their initial allocation fails), I don't know whether it's common > practice to allow grub_free (NULL), but I would encourage checking > whether these are actually allocated before attempting to free them. free(3P) explicitly mandates that it may be called with a `NULL` pointer as argument, in which case it will just do nothing. `grub_free` is in fact doing the same, so we should be good here. Patrick --PNTmBPCT7hxwcZjr Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAABCAAdFiEEtmscHsieVjl9VyNUEXxntp6r8SwFAl3BdSsACgkQEXxntp6r 8SxjDA/7BW0XZw0bSpQxBPoLnvDT89vGCr9pgkURzxJsIXjwrbC2YJeulkfCzyuF LhIXhhuriLPxn4baudUpiaQBd/NsWS02QQvrT6o6TyB5VTKabeWV6RIfCa5NKpvg hEaemgYNG21J5/J/jRGK/XApDy4LX1ReS8Bfp4vCOZqKKWmgRXi4IxGssxSceRW7 mx+CECN7fisajoiEB72xvfGm2YJLlKJhyiSAyN2D2/pc1n18wASMABpRV3e4cXmk oMaluHwx8P3n/sXdxq4luJHKMpSrFWZoQsGBPJL2WJTL1HDW2M84J93Tl0dFTvE3 B4eSDrHr6vM60KHO3Jc2j/4+wsw537VVS1ow6MgSRyv5ZGTOudIrHgMA6Yo+oPWA b2w/+nGfpqEvWIvJMdCf29Q0HUYMZ45HvGcieXe1Q7E0bfCqHTBtEUEuNH9pwCoB LTAj5+urcstXs3f/yyk6k5DJdxEgRVKlAQ02SJT4l1xyRLQw6UYruoLUazJuq61b e076hm9HEkXCm7w4CahOqaWEvZwr80qFSZ8JFVmC1hbYnaE91aXVzk2MIJNu5IrF hFi27EBL6ZnbnqAHQMi8R2NpzOwHaDkpbAWlh2KmTtthAzZj10b8zh44lAbMH+AP 18v4dszWlRQGazuk1oSCKI0RGYMmzVkcBWMy2+Zv+gvYAbjPi94= =sucQ -----END PGP SIGNATURE----- --PNTmBPCT7hxwcZjr--