On Tue, Jan 8, 2013 at 12:31 AM, Karel Zak <kzak@xxxxxxxxxx> wrote: > > Sorry for the delay, I had 3 weeks vacation. > > On Sat, Dec 08, 2012 at 12:19:12AM -0800, Andy Lutomirski wrote: >> +if BUILD_SETPRIV >> +usrbin_exec_PROGRAMS += setpriv >> +dist_man_MANS += sys-utils/setpriv.1 >> +setpriv_SOURCES = sys-utils/setpriv.c >> +setpriv_LDADD = -lcap-ng >> +endif > > Maybe the caps support should be optional, anyway you need to check > for the library in the configure script. I'll make the whole thing depend on libcap-ng for now. Making the dependency optional would require extra (careful) thought to remove the "reactivate capabilities" part after calling setresuid. > >> +static void complain(const char *fmt, ...) >> +{ >> + va_list ap; >> + va_start(ap, fmt); >> + vfprintf(stderr, fmt, ap); >> + va_end(ap); >> + fputc('\n', stderr); >> + usage(EXIT_FAILURE); >> +} > > seems like poor reimplementation of err() from err.h :-) I didn't know about that. Neat! (Although I changed all complain callers to use errx instead of err - errno makes no sense there.) > >> +// Returns the number of capabilities printed > > please use /* comment */ Done. > >> +static void stdout_perror(const char *prefix) >> +{ >> + if (errno < 0 || errno >= sys_nerr) >> + printf(_("%s: error %d\n"), prefix, errno); >> + else >> + printf(_("%s: %s\n"), prefix, sys_errlist[errno]); >> +} > > warnx() from err.h > Done. >> +static void dump_label(const char *name) >> +{ >> + int fd = open("/proc/self/attr/current", O_RDONLY); >> + if (fd == -1) { >> + stdout_perror(name); >> + return; >> + } >> + >> + char buf[4097]; >> + ssize_t len = read(fd, buf, sizeof(buf)); >> + int e = errno; > > we usually have declarations at the begin of block (function)... Done. > >> +struct options { > > ...and structs at the begin of the file. The name "options" seems > also too generic (like anything for getopt_long), what about > > struct privcxt { > > } > Done <irrelevant>Why do some people like cxt and other like ctx?</irrelevant> > or so. > >> bool nnp; > > don't use bool in util-linux, within structs you can use bit arrays. There do seem to be other users of stdbool in util-linux (in login-utils and lib/mbsalign.c). Do you object to true and false or just to bool? > >> // real and effective (in that order) >> bool have_uid[2]; >> uid_t uid[2]; >> bool have_gid[2]; >> gid_t gid[2]; > > it would be more readable to use > > uid_t ruid; > uid_t euid; > > unsigned have_ruid:1, > have_euid:1; > > (and the same for groups) rather than depend on an order. Done. > >> +static void priverr(const char *str) >> +{ >> + perror(str); >> + exit(127); >> +} > > err(127, str); > > it would be also nice to have a macro (SETPRV_EX_* ?) in the code > rather than the magic number. Done. I called it SETPRIV_EXIT_CANT_SET. > >> + >> +static void parse_groups(struct options *opts, const char *str) >> +{ >> + char *groups = strdup(str); >> + char *buf = groups; /* We'll reuse it */ >> + char *c; >> + >> + opts->have_groups = true; >> + opts->num_groups = 0; >> + while ((c = strsep(&groups, ",")) != 0) >> + opts->num_groups++; >> + >> + // Start again >> + strcpy(buf, str); // It's exactly the right length >> + groups = buf; >> + >> + opts->groups = calloc(opts->num_groups, sizeof(gid_t)); >> + size_t i = 0; >> + while ((c = strsep(&groups, ",")) != 0) { >> + char *end; >> + errno = 0; >> + long val = strtol(c, &end, 10); > > strtol_or_err() (include/strutils.h) > >> + if (!*c || *end || errno || val != (long long)(gid_t)val) >> + complain(_("Invalid supplementary group id")); >> + opts->groups[i++] = (gid_t)val; >> + } >> + >> + free(groups);; >> +} >> + > >> +int main(int argc, char *argv[]) >> +{ >> + enum { >> + NNP = 256, > > SETPRIV_OPT_NNP = CHAR_MAX + 1 > >> + RUID = 256, >> + EUID, >> + RGID, >> + EGID, >> + REUID, >> + REGID, >> + CLEAR_GROUPS, >> + KEEP_GROUPS, >> + GROUPS, >> + INHCAPS, >> + LISTCAPS, >> + CAPBSET, >> + SECUREBITS, >> + SELINUX_LABEL, >> + APPARMOR_PROFILE >> + }; > > [...] > >> + if (RUID <= c && c <= REGID) { >> + /* This is easier than six independent cases. */ >> + char *end; >> + errno = 0; >> + long val = strtol(optarg, &end, 10); >> + if (!*optarg || errno || *end) >> + complain(_("Failed to parse uid or gid")); >> + >> + if (c == REUID) { > > if (opts.have_euid) > err(EXIT_FAILURE, _("duplicate euid"); > opts.have_euid = 1; > opts.euid = strtol_or_err(optarg, _("failed to parse euid"); > > ... no loop, no arrays for uids, just readable code. > > [..] Done. > >> + if (dumplevel) { >> + if (total_opts != dumplevel || optind < argc) >> + complain(_("--dump is incompatible with all other options")); >> + dump(dumplevel); >> + return 0; > > > this is main(), so return EXIT_SUCCES; Done. -- To unsubscribe from this list: send the line "unsubscribe util-linux" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html