From mboxrd@z Thu Jan  1 00:00:00 1970
From: David Gibson <david@gibson.dropbear.id.au>
To: passt-dev@passt.top
Subject: Re: [PATCH 07/28] Clean up parsing in conf_runas()
Date: Thu, 29 Sep 2022 11:44:21 +1000
Message-ID: <YzT4dYUdDb2ahSCU@yekko>
In-Reply-To: <20220928225755.6bfb59f9@elisabeth>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="===============5810868803084434040=="

--===============5810868803084434040==
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable

On Wed, Sep 28, 2022 at 10:57:55PM +0200, Stefano Brivio wrote:
> On Wed, 28 Sep 2022 14:33:18 +1000
> David Gibson <david(a)gibson.dropbear.id.au> wrote:
>=20
> > conf_runas() handles several of the different possible cases for the
> > --runas argument in a slightly odd order.  Although it can parse both
> > numeric UIDs/GIDs and user/group names, it can't parse a numeric UID
> > combined with a group name or vice versa.  That's not obviously useful, b=
ut
> > it's slightly surprising gap to have.
>=20
> Ah, actually, I had noticed that: I usually type my UID as number, but
> I can't remember GIDs... then I had half a mind of "fixing" that, and
> never got to it.
>=20
> > [...]
> >
> > +++ b/conf.c
> > @@ -859,46 +859,50 @@ dns6:
> >   *
> >   * Return: 0 on success, negative error code on failure
> >   */
> > -static int conf_runas(const char *opt, unsigned int *uid, unsigned int *=
gid)
> > +static int conf_runas(char *opt, unsigned int *uid, unsigned int *gid)
>=20
> While I like your approach better than the existing one, I see one
> detail that, albeit minor, might drive somebody mad: if the UID is
> valid, but the GID isn't, we go back to conf_ugid():
>=20
> 		if (ret)
> 			err("Invalid --runas option: %s", runas);
>=20
> and print the UID part, only, because we cut before the separator.

Urgh, right.  This is why having worked a bit with Rust and Go, I'm
really coming to hate C-style strings: even really rudimentary parsing
usually requires either allocation & copies or modifying logically
read-only inputs.

> Perhaps we could simply put the separator back before returning
> -EINVAL (instead of copying the buffer) -- we just have zero or one
> occurrences of ':'.

That would work, although making sure we do that on every exit path
looks messier than I'd like.  I went with a different approach of
removing that message from the caller, and instead printing more
specific messages for each of the cases within conf_runas().  A little
more involved, but I think gives a marginally better UX overall.

--=20
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

--===============5810868803084434040==
Content-Type: application/pgp-signature
Content-Transfer-Encoding: base64
Content-Disposition: attachment; filename="signature.asc"
MIME-Version: 1.0

LS0tLS1CRUdJTiBQR1AgU0lHTkFUVVJFLS0tLS0KCmlRSXpCQUVCQ0FBZEZpRUVvVUx4V3U0L1dz
MGRCK1h0Z3lwWTRnRXdZU0lGQW1NMCtHNEFDZ2tRZ3lwWTRnRXcKWVNMMjJnLy9ZK09GSEc3M0t2
RUxLdDd2eFVrMzY1MktaY1JDL1BUeVBwUUZUaWpkdi9XcmIyaHpIdkdFUHJQSQpKbzU1bUhsa2Fk
Sk9YczEvdURCT2NjeWxRMzlubSs5R2R0TnArdkhsSHlpdUNpaFVIZUhPWjZPN00vZTBRT0pzCkt2
NTlOOXVGU2NaZGRTbnBEWk11UHBYLys0S2xmZ3g4bmpOZldNMStGazhFQzJTVnM1aHpJeVI2NjZU
N1NUWnIKYmdPcnJ0Y1YxcE00WXNtSHgvNm1GMERxWHJvZG96MG1xM3NCeE1PMStrdzk0bGFRL0Jw
OWNjSENFTXpIN29qeAprcFRCVGhhZXNtanJDekQ3SEUrRDlFdVhRdlR3OGUraExLaC9nZEZiMVJX
eWZqd3lyTERuS0REZlhxMUpzYjdGCm9PSm1TRytJVzBKellxV2IyWndXRHMvTU9zREJMM0psOHBv
UmlSWGpVYjJDTmdNYzRMdWxkVEZ2MFdxQ2o2YUcKYmNGcDBKbkZ1N3k3TEZYZy9SanlQaFhNVEVs
dXRwblNHcGx6WFdYSFpBOVU2Tm9ValdGL0VSbVJjaXFFTC9GVgpMMlBWNjc4dmZmK1hvSmRJNTFZ
Z3h3VE96NVJxWXNlbWoyTk9rVGdEWjBEcS9jOGQ0WithMm5sQTF0UkZCeTFwCnBRZTZzZFNud3hn
Vnk5QjNjekxKRmNMNG5UM0x6cXVCTGZxVVNZVngyaEFlS0JiV1VoMC9Ybnk5VUFSUk9rNmMKLzhG
cDlBd0JTb2lJOWFXME13R1Z5Nnd1ZmVZT0Zuc2pjZFhTUzFBNkpKRUYyc1BTYVZ1R1J5L1g5Szdp
WjgrOQpENnhlRDBhZ1dxNURmWmY1K0trWDdJaFJHRTJ3c2lmTVltQzAvdW9SYU14RXB5L1Btb009
Cj1WdDBECi0tLS0tRU5EIFBHUCBTSUdOQVRVUkUtLS0tLQo=

--===============5810868803084434040==--