Re: [PATCH 07/28] Clean up parsing in conf_runas()

On Wed, Sep 28, 2022 at 10:57:55PM +0200, Stefano Brivio wrote:

On Wed, 28 Sep 2022 14:33:18 +1000 David Gibson wrote:

...

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, but it's slightly surprising gap to have.

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.

...

[...]

+++ 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)

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():

if (ret) err("Invalid --runas option: %s", runas);

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. -- 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