[ovs-dev] [PATCH 2/3] lib: Add --user for daemon

[Ben Pfaff]

On Thu, Sep 03, 2015 at 04:33:42PM -0700, Andy Zhou wrote:
> Allow daemon running as root to accept --user option, that accepts
> "user:group" string as input. Performs sanity check on the input,
> and store the converted uid and gid.
> 
> daemon_become_new_user() needs to be called to make the actual
> switch.
> 
> Signed-off-by: Andy Zhou <azhou at nicira.com>

Thanks for implementing this.

There should be a new-line after void here:
> +void daemon_set_new_user(const char *user_spec)
> +{

I think the ability to use setuid() and friends only depends on having
uid 0, not gid 0:
> +    uid = getuid();
> +    gid = getgid();
> +
> +    if (gid || uid) {
> +        VLOG_FATAL("%s: only root can use --user option", pidfile);
> +    }

Here in daemon_set_new_user() I would use xmemdup0() instead:
> +    if (len) {
> +        user = xzalloc(len + 1);
> +        strncpy(user, user_spec, len);

and this should be xstrdup():
> +        user = strdup(pwd.pw_name);
> +    }

I think that the parsing in daemon_set_new_user() assumes that white
space, if present, will precede ':'.  If not, then 'len' will end up
negative, which looks bad to me.  I think I'd just not bother worrying
about white space in the parameter.

I am not sure that it is valuable to check that the user belongs to the
specified group.  I don't think that other software tends to perform
such a check; I don't see one in Apache, for example.

I might have other comments when I look at the final patch.