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

[Markus Armbruster]

Ian Jackson <address@hidden> writes:

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

No argument.  But why can't os_parse_cmd_args() call getpwnam() as it
does now, then store user_pwd->pw_uid, ->pw_gid and ->pw_name instead of
user_pwd?  Store a null name when it parses the argument as UID:GID.

>> Ian Jackson <address@hidden> 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

Yes, that's better.  But perhaps you like my idea above.

[...]