Re: [Xen-devel] [Qemu-devel] [PATCH 7/8] os-posix: Provide new -runasid option
Markus Armbruster writes ("Re: [Qemu-devel] [PATCH 7/8] os-posix: Provide new -runasid option"): > Ian Jacksonwrites: > > qemu_strtoul fails (returns an error) if the delimiter (that is, the > > first character which is not processed as digit by strtoul) is not > > '\0'. > > It does that *only* when its @endptr argument is null. Since you pass > , it should work fine here. I have just read it again and you are right. Sorry. I will fix this then. > > Does that all make sense ? > > Your code makes sense to me. It's just the comment that confuses me. > Does ID mean "implementation-defined behavior"? That would be wrong: Yes, that's what I meant by ID. >6.3.1.3 Signed and unsigned integers > >[#1] When a value with integer type is converted to another >integer type other than _Bool, if the value can be >represented by the new type, it is unchanged. > >[#2] Otherwise, if the new type is unsigned, the value is >converted by repeatedly adding or subtracting one more than >the maximum value that can be represented in the new type >until the value is in the range of the new type. That applies if the new type (uid_t, here) is unsigned. But I think uid_t's signedness is not specified, so it might be signed. (SuS says, in the section on types.h, only Additionally: * nlink_t, uid_t, gid_t, and id_t shall be integer types. C99 6.3.1.3 continues: Otherwise, the new type is signed and the value cannot be represented in it; either the result is implementation-defined or an implementation-defined signal is raised. So the type of uid_t is ID too. > One more thing: please report errors with error_report(). Here: > error_report("Could not obtain uid"); > > No need to quote optarg back at the user, because error_report() already > does. Ah, I will do that. Thanks. Regards, Ian. ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [Qemu-devel] [PATCH 7/8] os-posix: Provide new -runasid option
Ian Jacksonwrites: > Hi. Thanks for the (re)-review. > > Markus Armbruster writes ("Re: [Qemu-devel] [PATCH 7/8] os-posix: Provide new > -runasid option"): >> Ian Jackson writes: >> > +case QEMU_OPTION_runasid: >> > +errno = 0; >> > +lv = strtoul(optarg, , 0); /* can't qemu_strtoul, want >> > *ep=='.' */ >> >> I'm afraid I can't see why qemu_strtoul() wouldn't work here. Can you >> enlighten me? > > qemu_strtoul fails (returns an error) if the delimiter (that is, the > first character which is not processed as digit by strtoul) is not > '\0'. It does that *only* when its @endptr argument is null. Since you pass , it should work fine here. >Normally this is desirable, because it correctly rejects > strings like "123blather". But here we are trying to process a string > whose first non-digit is ':', and we will handle the tail part > explicitly. > > It would be possible to use strchr and then to write a '\0' over the > ':' but I dislike that processing style (and it is forbidden by the > const annotations on os_parse_cmd_args etc.) I'd dislike that, too. >> > +user_uid = lv; /* overflow here is ID in C99 */ >> >> I don't get the comment. You check for overflow the obvious way below. >> Sure you need a comment? > > This relates to overflow in the assignment, only. lv is an unsigned > long and user_uid is a uid_t (which may be smaller). Normally, signed > integer overflow is UB, but this is not the case when converting from > another integer type. > > There are two possible overflows: 1. a string that strtoul cannot get > to fit in an unsigned long, which produces a nonzero errno; and, 2. a > value which fits in an unsigned long but not a uid_t. In the latter > case, we convert it _back_ into an unsigned long, as an implicit > conversion in this middle test: > >> > +if (errno || *ep != '.' || user_uid != lv || user_uid == >> > (uid_t)-1) { > > If that succeeds, we know we can round-trip it so it is valid. The > remaining check needed is that it doesn't round trip to the sentinel > uid value. > > Does that all make sense ? Your code makes sense to me. It's just the comment that confuses me. Does ID mean "implementation-defined behavior"? That would be wrong: 6.3.1.3 Signed and unsigned integers [#1] When a value with integer type is converted to another integer type other than _Bool, if the value can be represented by the new type, it is unchanged. [#2] Otherwise, if the new type is unsigned, the value is converted by repeatedly adding or subtracting one more than the maximum value that can be represented in the new type until the value is in the range of the new type. > I'm not sure how much of this to document > in comments. It's deeply annoying that C is such a puzzle language > here and that therefore such complicated reasoning and > not-immediately-obvious code is needed, to do something so simple. > > If you would like me to remove the comment, I can drop it, of course. > I don't really mind. I'd drop it. You might find this variation of your code slightly more obvious, or slightly less: if (errno || *ep != '.' || (uid_t)lv != lv || user_uid == (uid_t)-1) { fprintf(stderr, "Could not obtain uid from \"%s\"", optarg); exit(1); } user_uid = lv; Your choice. One more thing: please report errors with error_report(). Here: error_report("Could not obtain uid"); No need to quote optarg back at the user, because error_report() already does. >> > +#ifndef _WIN32 >> > +DEF("runasid", HAS_ARG, QEMU_OPTION_runasid, \ >> > +"-runasid uid.gid change to numeric uid and gid just before >> > starting the VM\n", >> > +QEMU_ARCH_ALL) >> > +#endif >> > +STEXI >> > +@item -runasid @var{uid}.@var{gid} >> >> Didn't we agree on using ':' instead of '.' as separator? >> >> Sure we need yet another option? Why can't we compatibly extend -runas? > > For some reason you are looking at an old version of the patch. That > may be my fault - there were a few mis-posts. Sorry about that. It could be just as well my fault: my inbox spun out of control before KVM Forum. > The final version does indeed have ':' and does reuse the -runas > option. > >> If we truly need both, the rationale belongs into the commit message, >> and we need to consider how they interact. I'd recommend left-to-right >> semantics, i.e. if you specify both, the last one wins. Not what your >> current code does, if I read it correctly. > > Happily this is now irrelevant. > > I will repost this series after I hear from you about strtoul and the > overflow comment. Thanks! ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [Qemu-devel] [PATCH 7/8] os-posix: Provide new -runasid option
Hi. Thanks for the (re)-review. Markus Armbruster writes ("Re: [Qemu-devel] [PATCH 7/8] os-posix: Provide new -runasid option"): > Ian Jacksonwrites: > > +case QEMU_OPTION_runasid: > > +errno = 0; > > +lv = strtoul(optarg, , 0); /* can't qemu_strtoul, want *ep=='.' > > */ > > I'm afraid I can't see why qemu_strtoul() wouldn't work here. Can you > enlighten me? qemu_strtoul fails (returns an error) if the delimiter (that is, the first character which is not processed as digit by strtoul) is not '\0'. Normally this is desirable, because it correctly rejects strings like "123blather". But here we are trying to process a string whose first non-digit is ':', and we will handle the tail part explicitly. It would be possible to use strchr and then to write a '\0' over the ':' but I dislike that processing style (and it is forbidden by the const annotations on os_parse_cmd_args etc.) > > +user_uid = lv; /* overflow here is ID in C99 */ > > I don't get the comment. You check for overflow the obvious way below. > Sure you need a comment? This relates to overflow in the assignment, only. lv is an unsigned long and user_uid is a uid_t (which may be smaller). Normally, signed integer overflow is UB, but this is not the case when converting from another integer type. There are two possible overflows: 1. a string that strtoul cannot get to fit in an unsigned long, which produces a nonzero errno; and, 2. a value which fits in an unsigned long but not a uid_t. In the latter case, we convert it _back_ into an unsigned long, as an implicit conversion in this middle test: > > +if (errno || *ep != '.' || user_uid != lv || user_uid == > > (uid_t)-1) { If that succeeds, we know we can round-trip it so it is valid. The remaining check needed is that it doesn't round trip to the sentinel uid value. Does that all make sense ? I'm not sure how much of this to document in comments. It's deeply annoying that C is such a puzzle language here and that therefore such complicated reasoning and not-immediately-obvious code is needed, to do something so simple. If you would like me to remove the comment, I can drop it, of course. I don't really mind. > > +#ifndef _WIN32 > > +DEF("runasid", HAS_ARG, QEMU_OPTION_runasid, \ > > +"-runasid uid.gid change to numeric uid and gid just before > > starting the VM\n", > > +QEMU_ARCH_ALL) > > +#endif > > +STEXI > > +@item -runasid @var{uid}.@var{gid} > > Didn't we agree on using ':' instead of '.' as separator? > > Sure we need yet another option? Why can't we compatibly extend -runas? For some reason you are looking at an old version of the patch. That may be my fault - there were a few mis-posts. Sorry about that. The final version does indeed have ':' and does reuse the -runas option. > If we truly need both, the rationale belongs into the commit message, > and we need to consider how they interact. I'd recommend left-to-right > semantics, i.e. if you specify both, the last one wins. Not what your > current code does, if I read it correctly. Happily this is now irrelevant. I will repost this series after I hear from you about strtoul and the overflow comment. Regards, Ian. ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [Qemu-devel] [PATCH 7/8] os-posix: Provide new -runasid option
Sorry for the slow response. Ian Jacksonwrites: > This allows the caller to specify a uid and gid to use, even if there > is no corresponding password entry. This will be useful in certain > Xen configurations. > > Signed-off-by: Ian Jackson > --- > v3: Error messages fixed. Thanks to Peter Maydell and Ross Lagerwall. > v2: Coding style fixes. > --- > os-posix.c | 49 - > qemu-options.hx | 12 > 2 files changed, 52 insertions(+), 9 deletions(-) > > diff --git a/os-posix.c b/os-posix.c > index 92e9d85..6cc5868 100644 > --- a/os-posix.c > +++ b/os-posix.c > @@ -43,6 +43,8 @@ > #endif > > static struct passwd *user_pwd; > +static uid_t user_uid = (uid_t)-1; > +static gid_t user_gid = (gid_t)-1; > static const char *chroot_dir; > static int daemonize; > static int daemon_pipe; > @@ -134,6 +136,9 @@ void os_set_proc_name(const char *s) > */ > void os_parse_cmd_args(int index, const char *optarg) > { > +unsigned long lv; > +char *ep; > +int rc; > switch (index) { > #ifdef CONFIG_SLIRP > case QEMU_OPTION_smb: > @@ -150,6 +155,22 @@ void os_parse_cmd_args(int index, const char *optarg) > exit(1); > } > break; > +case QEMU_OPTION_runasid: > +errno = 0; > +lv = strtoul(optarg, , 0); /* can't qemu_strtoul, want *ep=='.' */ I'm afraid I can't see why qemu_strtoul() wouldn't work here. Can you enlighten me? > +user_uid = lv; /* overflow here is ID in C99 */ I don't get the comment. You check for overflow the obvious way below. Sure you need a comment? > +if (errno || *ep != '.' || user_uid != lv || user_uid == (uid_t)-1) { > +fprintf(stderr, "Could not obtain uid from \"%s\"", optarg); > +exit(1); > +} > +lv = 0; > +rc = qemu_strtoul(ep + 1, 0, 0, ); > +user_gid = lv; /* overflow here is ID in C99 */ Likewise. > +if (rc || user_gid != lv || user_gid == (gid_t)-1) { > +fprintf(stderr, "Could not obtain gid from \"%s\"", optarg); > +exit(1); > +} > +break; > case QEMU_OPTION_chroot: > chroot_dir = optarg; > break; > @@ -166,18 +187,28 @@ void os_parse_cmd_args(int index, const char *optarg) > > static void change_process_uid(void) > { > -if (user_pwd) { > -if (setgid(user_pwd->pw_gid) < 0) { > -fprintf(stderr, "Failed to setgid(%d)\n", user_pwd->pw_gid); > +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); > exit(1); > } > -if (initgroups(user_pwd->pw_name, user_pwd->pw_gid) < 0) { > -fprintf(stderr, "Failed to initgroups(\"%s\", %d)\n", > -user_pwd->pw_name, user_pwd->pw_gid); > -exit(1); > +if (user_pwd) { > +if (initgroups(user_pwd->pw_name, user_pwd->pw_gid) < 0) { > +fprintf(stderr, "Failed to initgroups(\"%s\", %d)\n", > +user_pwd->pw_name, user_pwd->pw_gid); > +exit(1); > +} > +} else { > +if (setgroups(1, _gid) < 0) { > +fprintf(stderr, "Failed to setgroups(1, [%d])", > +user_gid); > +exit(1); > +} > } > -if (setuid(user_pwd->pw_uid) < 0) { > -fprintf(stderr, "Failed to setuid(%d)\n", user_pwd->pw_uid); > +if (setuid(intended_uid) < 0) { > +fprintf(stderr, "Failed to setuid(%d)\n", intended_uid); > exit(1); > } > if (setuid(0) != -1) { > diff --git a/qemu-options.hx b/qemu-options.hx > index 9f6e2ad..34a5329 100644 > --- a/qemu-options.hx > +++ b/qemu-options.hx > @@ -3968,6 +3968,18 @@ Immediately before starting guest execution, drop root > privileges, switching > to the specified user. > ETEXI > > +#ifndef _WIN32 > +DEF("runasid", HAS_ARG, QEMU_OPTION_runasid, \ > +"-runasid uid.gid change to numeric uid and gid just before starting > the VM\n", > +QEMU_ARCH_ALL) > +#endif > +STEXI > +@item -runasid @var{uid}.@var{gid} Didn't we agree on using ':' instead of '.' as separator? Sure we need yet another option? Why can't we compatibly extend -runas? If we truly need both, the rationale belongs into the commit message, and we need to consider how they interact. I'd recommend left-to-right semantics, i.e. if you specify both, the last one wins. Not what your current code does, if I read it correctly. > +@findex -runasid > +Immediately before starting guest execution, drop root privileges, switching > +to the specified uid and
Re: [Xen-devel] [Qemu-devel] [PATCH 7/8] os-posix: Provide new -runasid option
Markus Armbruster writes ("Re: [Qemu-devel] [PATCH 7/8] os-posix: Provide new -runasid option"): > Actually, a numeric UID without group name or ID could be made to work > just fine as long as it maps to a user name. The use case may not be > worth the bother, though. In libxl's use case, it wouldn't map to a name. > Using '.' to separate user and group is suboptimal, because POSIX > portable user and group names may contain it: ... > Coreutils uses ':'. Let's follow its lead. I'm happy to change it to use `:'. Can you confirm that this command line interface is then OK ? We would like to stablise the corresponding code in libxl... Ian. ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [Qemu-devel] [PATCH 7/8] os-posix: Provide new -runasid option
Markus Armbruster writes ("Re: [Qemu-devel] [PATCH 7/8] os-posix: Provide new -runasid option"): > Actually, a numeric UID without group name or ID could be made to work > just fine as long as it maps to a user name. The use case may not be > worth the bother, though. In libxl's use case, it wouldn't map to a name. > Using '.' to separate user and group is suboptimal, because POSIX > portable user and group names may contain it: ... > Coreutils uses ':'. Let's follow its lead. I'm happy to change it to use `:'. Can you confirm that this command line interface is then OK ? We would like to stablise the corresponding code in libxl... Ian. ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [Qemu-devel] [PATCH 7/8] os-posix: Provide new -runasid option
Ian Jacksonwrites: > Markus Armbruster writes ("Re: [Qemu-devel] [PATCH 7/8] os-posix: Provide new > -runasid option"): >> The last thing the QEMU command line needs is more exotic options. Are >> you sure we need a new one here? Can we make existing -runas serve? >> Precedence: Coreutils[*]. Pseudo-code: >> >> if argument is a decimal number starting with '+': >> user ID >> else if argument is a valid user name: >> user name >> else if argument is a valid user ID: >> user ID >> else: >> error > > I can do this. So -runas . then. I don't think it makes > sense to try to -runas because: you wouldn't have a username > to pass to initgroups: not calling initgroups would be a bear trap; > and otherwise we wouldn't know what gid to use. Actually, a numeric UID without group name or ID could be made to work just fine as long as it maps to a user name. The use case may not be worth the bother, though. Using '.' to separate user and group is suboptimal, because POSIX portable user and group names may contain it: 3.426 User Name A string that is used to identify a user; see also User Database. To be portable across systems conforming to IEEE Std 1003.1-2001, the value is composed of characters from the portable filename character set. The hyphen should not be used as the first character of a portable user name. and 3.276 Portable Filename Character Set The set of characters from which portable filenames are constructed. A B C D E F G H I J K L M N O P Q R S T U V W X Y Z a b c d e f g h i j k l m n o p q r s t u v w x y z 0 1 2 3 4 5 6 7 8 9 . _ - http://pubs.opengroup.org/onlinepubs/95399/basedefs/xbd_chap03.html Coreutils uses ':'. Let's follow its lead. ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [Qemu-devel] [PATCH 7/8] os-posix: Provide new -runasid option
(resending, more competently this time) Daniel P. Berrange writes ("Re: [Qemu-devel] [PATCH 7/8] os-posix: Provide new -runasid option"): > Just use getpwuid() to get the "struct passwd *", then change_process_uid() > doesn't need any changes at all AFAICT. See my comments in the commit message. There may be multiple passwd entries referring to a particular uid. I think in this area it would be better to expect the caller to be explicit, rather than taking getpwuid's notion of the first one. Ian. ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [Qemu-devel] [PATCH 7/8] os-posix: Provide new -runasid option
Daniel P. Berrange writes ("Re: [Qemu-devel] [PATCH 7/8] os-posix: Provide new -runasid option"): > Just use getpwuid() to get the "struct passwd *", then change_process_uid() > doesn't need any changes at all AFAICT. See my comments in the commit message. There may be multiple passwd entries referring to a particular uid. I think in this area it would be better to expect the caller to be explicit, rather than taking getpwuid's notion of the first one. Ian. ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [Qemu-devel] [PATCH 7/8] os-posix: Provide new -runasid option
On Mon, Oct 09, 2017 at 04:05:10PM +0100, Ian Jackson wrote: > Markus Armbruster writes ("Re: [Qemu-devel] [PATCH 7/8] os-posix: Provide new > -runasid option"): > > The last thing the QEMU command line needs is more exotic options. Are > > you sure we need a new one here? Can we make existing -runas serve? > > Precedence: Coreutils[*]. Pseudo-code: > > > > if argument is a decimal number starting with '+': > > user ID > > else if argument is a valid user name: > > user name > > else if argument is a valid user ID: > > user ID > > else: > > error > > I can do this. So -runas . then. I don't think it makes > sense to try to -runas because: you wouldn't have a username > to pass to initgroups: not calling initgroups would be a bear trap; > and otherwise we wouldn't know what gid to use. Just use getpwuid() to get the "struct passwd *", then change_process_uid() doesn't need any changes at all AFAICT. Regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :| ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [Qemu-devel] [PATCH 7/8] os-posix: Provide new -runasid option
(resending to right address for xen-devel) Markus Armbruster writes ("Re: [Qemu-devel] [PATCH 7/8] os-posix: Provide new -runasid option"): > The last thing the QEMU command line needs is more exotic options. Are > you sure we need a new one here? Can we make existing -runas serve? > Precedence: Coreutils[*]. Pseudo-code: > > if argument is a decimal number starting with '+': > user ID > else if argument is a valid user name: > user name > else if argument is a valid user ID: > user ID > else: > error I can do this. So -runas . then. I don't think it makes sense to try to -runas because: you wouldn't have a username to pass to initgroups: not calling initgroups would be a bear trap; and otherwise we wouldn't know what gid to use. Ian. ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [Qemu-devel] [PATCH 7/8] os-posix: Provide new -runasid option
Markus Armbruster writes ("Re: [Qemu-devel] [PATCH 7/8] os-posix: Provide new -runasid option"): > The last thing the QEMU command line needs is more exotic options. Are > you sure we need a new one here? Can we make existing -runas serve? > Precedence: Coreutils[*]. Pseudo-code: > > if argument is a decimal number starting with '+': > user ID > else if argument is a valid user name: > user name > else if argument is a valid user ID: > user ID > else: > error I can do this. So -runas . then. I don't think it makes sense to try to -runas because: you wouldn't have a username to pass to initgroups: not calling initgroups would be a bear trap; and otherwise we wouldn't know what gid to use. Ian. ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [Qemu-devel] [PATCH 7/8] os-posix: Provide new -runasid option
Ian Jacksonwrites: > This allows the caller to specify a uid and gid to use, even if there > is no corresponding password entry. This will be useful in certain > Xen configurations. > > Signed-off-by: Ian Jackson [...] > diff --git a/qemu-options.hx b/qemu-options.hx > index 9f6e2ad..34a5329 100644 > --- a/qemu-options.hx > +++ b/qemu-options.hx > @@ -3968,6 +3968,18 @@ Immediately before starting guest execution, drop root > privileges, switching > to the specified user. > ETEXI > > +#ifndef _WIN32 > +DEF("runasid", HAS_ARG, QEMU_OPTION_runasid, \ > +"-runasid uid.gid change to numeric uid and gid just before starting > the VM\n", > +QEMU_ARCH_ALL) > +#endif > +STEXI > +@item -runasid @var{uid}.@var{gid} > +@findex -runasid > +Immediately before starting guest execution, drop root privileges, switching > +to the specified uid and gid. > +ETEXI > + > DEF("prom-env", HAS_ARG, QEMU_OPTION_prom_env, > "-prom-env variable=value\n" > "set OpenBIOS nvram variables\n", The last thing the QEMU command line needs is more exotic options. Are you sure we need a new one here? Can we make existing -runas serve? Precedence: Coreutils[*]. Pseudo-code: if argument is a decimal number starting with '+': user ID else if argument is a valid user name: user name else if argument is a valid user ID: user ID else: error [*] https://www.gnu.org/software/coreutils/manual/html_node/Disambiguating-names-and-IDs.html ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [Qemu-devel] [PATCH 7/8] os-posix: Provide new -runasid option
(resending to fix xen-devel CC) Peter Maydell writes ("Re: [Qemu-devel] [PATCH 7/8] os-posix: Provide new -runasid option"): > On 4 October 2017 at 17:18, Ian Jacksonwrote: > > static void change_process_uid(void) > > { > > -if (user_pwd) { > > -if (setgid(user_pwd->pw_gid) < 0) { > > +if (user_pwd || user_uid != (uid_t)-1) { > > +if (setgid(user_pwd ? user_pwd->pw_gid : user_gid) < 0) { > > fprintf(stderr, "Failed to setgid(%d)\n", user_pwd->pw_gid); > > If you're changing the gid we pass to setgid() I think you should > also change the value we tell the user we tried to use in the > error message, or it could be rather confusing. Thanks for the review. Sorry about the mess here. I have fixed all these problems. Ian. ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [Qemu-devel] [PATCH 7/8] os-posix: Provide new -runasid option
Peter Maydell writes ("Re: [Qemu-devel] [PATCH 7/8] os-posix: Provide new -runasid option"): > On 4 October 2017 at 17:18, Ian Jacksonwrote: > > static void change_process_uid(void) > > { > > -if (user_pwd) { > > -if (setgid(user_pwd->pw_gid) < 0) { > > +if (user_pwd || user_uid != (uid_t)-1) { > > +if (setgid(user_pwd ? user_pwd->pw_gid : user_gid) < 0) { > > fprintf(stderr, "Failed to setgid(%d)\n", user_pwd->pw_gid); > > If you're changing the gid we pass to setgid() I think you should > also change the value we tell the user we tried to use in the > error message, or it could be rather confusing. Thanks for the review. Sorry about the mess here. I have fixed all these problems. Ian. ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [Qemu-devel] [PATCH 7/8] os-posix: Provide new -runasid option
On 4 October 2017 at 17:18, Ian Jacksonwrote: > This allows the caller to specify a uid and gid to use, even if there > is no corresponding password entry. This will be useful in certain > Xen configurations. > > Signed-off-by: Ian Jackson > --- > @@ -166,17 +187,19 @@ void os_parse_cmd_args(int index, const char *optarg) > > static void change_process_uid(void) > { > -if (user_pwd) { > -if (setgid(user_pwd->pw_gid) < 0) { > +if (user_pwd || user_uid != (uid_t)-1) { > +if (setgid(user_pwd ? user_pwd->pw_gid : user_gid) < 0) { > fprintf(stderr, "Failed to setgid(%d)\n", user_pwd->pw_gid); If you're changing the gid we pass to setgid() I think you should also change the value we tell the user we tried to use in the error message, or it could be rather confusing. > exit(1); > } > -if (initgroups(user_pwd->pw_name, user_pwd->pw_gid) < 0) { > +if ((user_pwd > + ? initgroups(user_pwd->pw_name, user_pwd->pw_gid) > + : setgroups(1, _gid)) < 0) { > fprintf(stderr, "Failed to initgroups(\"%s\", %d)\n", > user_pwd->pw_name, user_pwd->pw_gid); ...and here we might claim we failed initgroups() when we actually failed setgroups(). > exit(1); > } > -if (setuid(user_pwd->pw_uid) < 0) { > +if (setuid(user_pwd ? user_pwd->pw_uid : user_gid) < 0) { > fprintf(stderr, "Failed to setuid(%d)\n", user_pwd->pw_uid); This error message should be updated too. thanks -- PMM ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel