Re: [Xen-devel] [Qemu-devel] [PATCH 08/12] os-posix: Provide new -runas <uid>:<gid> facility

[Ian Jackson <ian.jackson@xxxxxxxxxx>]

Thanks for the review.  Taking your comments out of order slightly:

Markus Armbruster writes ("Re: [Qemu-devel] [PATCH 08/12] os-posix: Provide new 
-runas <uid>:<gid> facility"):
> [change_process_uid] is the only user of @user_pwd, @user_uid, @user_gid.
> 
> Have you considered replacing global @user_pwd by @user_uid, @user_gid
> and @user_name?  --runas with numeric uid and gid would leave @user_name
> null.

That would defer the getpwnam from argument parsing to os_setup_post.
I think that's undesriable.

> Ian Jackson <ian.jackson@xxxxxxxxxxxxx> writes:
> >  static struct passwd *user_pwd;
> > +static uid_t user_uid = (uid_t)-1;
> > +static gid_t user_gid = (gid_t)-1;
> 
> As we'll see below, @user_pwd->pw_uid, @user_pwd_pw_gid take precedence
> over @user_uid, @user_gid.  Awkward.

My patch has the right behaviour: each -runas completely overrides the
previous one.  -runas that sets user_{uid,gid} always clears user_pwd
on the way.  So user_pwd can only be set if the most recent -runas was
a name, and then we should honour the name.

This is rather obscure.  I think you are right that this is confusing.
It ought to be clearer.

I will
  - add a comment next to these three variables saying they must
    all be set at the same time
  - explicitly (redundantly) clear user_pwd in os_parse_runas_uid_gid
  - explicitly set user_{uid,gid} to -1 when -runas gets a
    success from getpwnam
  - assert in change_process_uid that the combination is legal

> > +    errno = 0;
> > +    rc = qemu_strtoul(optarg, &ep, 0, &lv);
...
> > +    lv = 0;
> 
> Either zero lv before both qemu_strtoul() or neither one.

This is a hangover from the previous version which used raw strtoul.
The assignments to both lv and errno are redundant.

> > -        if (!user_pwd) {
> > -            fprintf(stderr, "User \"%s\" doesn't exist\n", optarg);
> > +        if (!user_pwd && !os_parse_runas_uid_gid(optarg)) {
> > +            error_report("User doesn't exist (and is not <uid>:<gid>)");
> 
> The error message no longer includes the offending value.  Intentional?

I was in two minds.  I will put it back.

> > +    if (user_pwd || user_uid != (uid_t)-1) {
> > +        gid_t intended_gid = user_pwd ? user_pwd->pw_gid : user_gid;
> > +        uid_t intended_uid = user_pwd ? user_pwd->pw_uid : user_uid;
> > +        if (setgid(intended_gid) < 0) {
> > +            fprintf(stderr, "Failed to setgid(%d)\n", intended_gid);
> 
> error_report(), please.  More of the same below.

I was following the existing code in this function.  I'll add a
pre-patch to fix this up here, and maybe a post-patch to fix the rest
of this file.

Thanks,
Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel