08.12.2015, 01:48, "Michael Witten" <mfwit...@gmail.com>: > User names have hitherto been handled neither consistently nor well; this > series alleviates at least some of the issues. > > Fear not the long patch series! > > Most commits involve a fairly small number of changes; while I could have > consolidated these changes into fewer commits, I think the series as a whole > provides a better narration for what's going. > > Besides a few small improvements along the way, the main thrust is: > > * Removing user-name handling from `scp' (in favor of using the > handling that is already present in `dropbear'/`dbclient'). > > * Lazily looking up the current user's name. > > * Removing unused code. > > Overall, 7 files were changed, with 37 insertions(+) and 158 deletions(-): > > cli-main.c | 2 +- > cli-runopts.c | 32 ++++++-------- > common-session.c | 1 + > runopts.h | 2 +- > scp.c | 125 ++++++++++--------------------------------------------- > scpmisc.c | 31 +------------- > scpmisc.h | 2 -
As can be seen from copyright header and git log, scp.c is a copy of corresponding file from OpenSSH (currently 4.3p2) with a few local changes. It might be a better idea to synchronize with upstream than to diverge it more. Just my 2 cents. > > This is the series: > > [01] scp: Insert comma into stderr message > [02] scp: Have `fatal()' append a newline to the message > [03] scp: only pass `-v' when DEBUG_TRACE is set > [04] scp: `-l%s' -> `-l %s' > [05] scp: const/static correctness improvements > [06] scp: Introduce `get_user_name()' > [07] scp: Use "unknown" when the user name cannot be retrieved > [08] scp: style: simplify code by using a tertiary operator > [09] scp: Use `get_user_name()' more often > [10] scp: Simplify code now that the user name is never `NULL' > [11] scp: Remove parsing of `[user@]host' > [12] scp: Remove unused functions > [13] scp: Remove `replacearg()' > [14] runopts: Mark `*cli_runopts.own_user' as `const' > [15] runopts: There's no reason to make a duplicate of "unknown" > [16] runopts: Re-introduce the `get_user_name()' function from `scp' > development > > Lastly, for convenience in reviewing the changes, here is the overall patch: > > --- a/cli-main.c > +++ b/cli-main.c > @@ -135,7 +135,7 @@ > static void cli_proxy_cmd(int *sock_in, int *sock_out) { > int ret; > > - fill_passwd(cli_opts.own_user); > + fill_passwd(get_user_name()); > > ret = spawn_command(exec_proxy_cmd, cli_opts.proxycmd, > sock_out, sock_in, NULL, NULL); > --- a/cli-runopts.c > +++ b/cli-runopts.c > @@ -36,7 +36,6 @@ > static void printhelp(); > static void parse_hostname(const char* orighostarg); > static void parse_multihop_hostname(const char* orighostarg, const char* > argv0); > -static void fill_own_user(); > #ifdef ENABLE_CLI_PUBKEY_AUTH > static void loadidentityfile(const char* filename, int warnfail); > #endif > @@ -102,6 +101,17 @@ > > } > > +const char *get_user_name() { > + static const char *user_name = NULL; > + > + if (user_name == NULL) { > + struct passwd *pwd = getpwuid(getuid()); > + user_name = pwd ? m_strdup(pwd->pw_name) : "unknown"; > + } > + > + return user_name; > +} > + > void cli_getopts(int argc, char ** argv) { > unsigned int i, j; > char ** next = 0; > @@ -175,8 +185,6 @@ > opts.keepalive_secs = DEFAULT_KEEPALIVE; > opts.idle_timeout_secs = DEFAULT_IDLE_TIMEOUT; > > - fill_own_user(); > - > for (i = 1; i < (unsigned int)argc; i++) { > /* Handle non-flag arguments such as hostname or commands > for the remote host */ > if (argv[i][0] != '-') > @@ -640,7 +648,7 @@ > } > > if (cli_opts.username == NULL) { > - cli_opts.username = m_strdup(cli_opts.own_user); > + cli_opts.username = m_strdup(get_user_name()); > } > > port = strchr(cli_opts.remotehost, '^'); > @@ -695,22 +703,6 @@ > } > #endif > > -static void fill_own_user() { > - uid_t uid; > - struct passwd *pw = NULL; > - > - uid = getuid(); > - > - pw = getpwuid(uid); > - if (pw && pw->pw_name != NULL) { > - cli_opts.own_user = m_strdup(pw->pw_name); > - } else { > - dropbear_log(LOG_INFO, "Warning: failed to identify current user. Trying > anyway."); > - cli_opts.own_user = m_strdup("unknown"); > - } > - > -} > - > #ifdef ENABLE_CLI_ANYTCPFWD > /* Turn a "[listenaddr:]listenport:remoteaddr:remoteport" string into into a > forwarding > * set, and add it to the forwarding list */ > --- a/common-session.c > +++ b/common-session.c > @@ -581,6 +581,7 @@ > return ses.authstate.pw_shell; > } > } > + > void fill_passwd(const char* username) { > struct passwd *pw = NULL; > if (ses.authstate.pw_name) > --- a/runopts.h > +++ b/runopts.h > @@ -127,7 +127,6 @@ > char *remotehost; > char *remoteport; > > - char *own_user; > char *username; > > char *cmd; > @@ -165,6 +164,7 @@ > > extern cli_runopts cli_opts; > void cli_getopts(int argc, char ** argv); > +const char *get_user_name(); > > #ifdef ENABLE_USER_ALGO_LIST > void parse_ciphers_macs(); > --- a/scp.c > +++ b/scp.c > @@ -172,25 +172,21 @@ > */ > > static void > -arg_setup(char *host, char *remuser, char *cmd) > +arg_setup(const char *user_at_host, const char *cmd) > { > - replacearg(&args, 0, "%s", ssh_program); > - if (remuser != NULL) > - addargs(&args, "-l%s", remuser); > - addargs(&args, "%s", host); > + addargs(&args, "%s", user_at_host); > addargs(&args, "%s", cmd); > } > > int > -do_cmd(char *host, char *remuser, char *cmd, int *fdin, int *fdout, int argc) > +do_cmd(const char *user_at_host, const char *cmd, int *fdin, int *fdout, int > argc) > { > int pin[2], pout[2], reserved[2]; > > if (verbose_mode) > fprintf(stderr, > - "Executing: program %s host %s, user %s, command %s\n", > - ssh_program, host, > - remuser ? remuser : "(unspecified)", cmd); > + "Executing: program %s, login %s, command %s\n", > + ssh_program, user_at_host, cmd); > > /* > * Reserve two descriptors so that the real pipes won't get > @@ -211,7 +207,7 @@ > /* uClinux needs to build the args here before vforking, > otherwise we do it later on. */ > #ifdef USE_VFORK > - arg_setup(host, remuser, cmd); > + arg_setup(user_at_host, cmd); > #endif > > /* Fork a child to execute the command on the remote host using ssh. > */ > @@ -231,7 +227,7 @@ > close(pout[1]); > > #ifndef USE_VFORK > - arg_setup(host, remuser, cmd); > + arg_setup(user_at_host, cmd); > #endif > > execvp(ssh_program, args.list); > @@ -251,16 +247,10 @@ > xfree(args.list[args.num-1]); > args.list[args.num-1]=NULL; > args.num--; > - /* pop host */ > + /* pop user_at_host */ > xfree(args.list[args.num-1]); > args.list[args.num-1]=NULL; > args.num--; > - /* pop user */ > - if (remuser != NULL) { > - xfree(args.list[args.num-1]); > - args.list[args.num-1]=NULL; > - args.num--; > - } > #endif > > /* Parent. Close the other side, and return the local side. */ > @@ -282,12 +272,9 @@ > BUF *allocbuf(BUF *, int, int); > void lostconn(int); > void nospace(void); > -int okname(char *); > void run_err(const char *,...); > void verifydir(char *); > > -struct passwd *pwd; > -uid_t userid; > int errs, remin, remout; > int pflag, iamremote, iamrecursive, targetshouldbedirectory; > > @@ -362,7 +349,9 @@ > ssh_program = xstrdup(optarg); > break; > case 'v': > +#ifdef DEBUG_TRACE > addargs(&args, "-v"); > +#endif > verbose_mode = 1; > break; > case 'q': > @@ -393,9 +382,6 @@ > argc -= optind; > argv += optind; > > - if ((pwd = getpwuid(userid = getuid())) == NULL) > - fatal("unknown user %u", (u_int) userid); > - > if (!isatty(STDERR_FILENO)) > showprogress = 0; > > @@ -459,7 +445,8 @@ > toremote(char *targ, int argc, char **argv) > { > int i, len; > - char *bp, *host, *src, *suser, *thost, *tuser, *arg; > + const char *targ_user_at_host; > + char *bp, *src; > arglist alist; > > memset(&alist, '\0', sizeof(alist)); > @@ -469,30 +456,17 @@ > if (*targ == 0) > targ = "."; > > - arg = xstrdup(argv[argc - 1]); > - if ((thost = strrchr(arg, '@'))) { > - /* user@host */ > - *thost++ = 0; > - tuser = arg; > - if (*tuser == '\0') > - tuser = NULL; > - } else { > - thost = arg; > - tuser = NULL; > - } > - > - if (tuser != NULL && !okname(tuser)) { > - xfree(arg); > - return; > - } > + targ_user_at_host = argv[argc - 1]; > > for (i = 0; i < argc - 1; i++) { > src = colon(argv[i]); > if (src) { /* remote to remote */ > freeargs(&alist); > addargs(&alist, "%s", ssh_program); > +#ifdef DEBUG_TRACE > if (verbose_mode) > addargs(&alist, "-v"); > +#endif > #if 0 > /* Disabled since dbclient won't understand them > and scp works fine without them. */ > @@ -504,27 +478,11 @@ > *src++ = 0; > if (*src == 0) > src = "."; > - host = strrchr(argv[i], '@'); > - > - if (host) { > - *host++ = 0; > - host = cleanhostname(host); > - suser = argv[i]; > - if (*suser == '\0') > - suser = pwd->pw_name; > - else if (!okname(suser)) > - continue; > - addargs(&alist, "-l"); > - addargs(&alist, "%s", suser); > - } else { > - host = cleanhostname(argv[i]); > - } > - addargs(&alist, "%s", host); > + > + addargs(&alist, "%s", argv[i]); /* [user@]host */ > addargs(&alist, "%s", cmd); > addargs(&alist, "%s", src); > - addargs(&alist, "%s%s%s:%s", > - tuser ? tuser : "", tuser ? "@" : "", > - thost, targ); > + addargs(&alist, "%s:%s", targ_user_at_host, targ); > if (do_local_cmd(&alist) != 0) > errs = 1; > } else { /* local to remote */ > @@ -532,8 +490,7 @@ > len = strlen(targ) + CMDNEEDS + 20; > bp = xmalloc(len); > (void) snprintf(bp, len, "%s -t %s", cmd, > targ); > - host = cleanhostname(thost); > - if (do_cmd(host, tuser, bp, &remin, > + if (do_cmd(targ_user_at_host, bp, &remin, > &remout, argc) < 0) > exit(1); > if (response() < 0) > @@ -549,7 +506,7 @@ > tolocal(int argc, char **argv) > { > int i, len; > - char *bp, *host, *src, *suser; > + char *bp, *src; > arglist alist; > > memset(&alist, '\0', sizeof(alist)); > @@ -572,20 +529,10 @@ > *src++ = 0; > if (*src == 0) > src = "."; > - if ((host = strrchr(argv[i], '@')) == NULL) { > - host = argv[i]; > - suser = NULL; > - } else { > - *host++ = 0; > - suser = argv[i]; > - if (*suser == '\0') > - suser = pwd->pw_name; > - } > - host = cleanhostname(host); > len = strlen(src) + CMDNEEDS + 20; > bp = xmalloc(len); > (void) snprintf(bp, len, "%s -f %s", cmd, src); > - if (do_cmd(host, suser, bp, &remin, &remout, argc) < 0) { > + if (do_cmd(argv[i], bp, &remin, &remout, argc) < 0) { > (void) xfree(bp); > ++errs; > continue; > @@ -1190,36 +1137,6 @@ > killchild(0); > } > > -int > -okname(char *cp0) > -{ > - int c; > - char *cp; > - > - cp = cp0; > - do { > - c = (int)*cp; > - if (c & 0200) > - goto bad; > - if (!isalpha(c) && !isdigit(c)) { > - switch (c) { > - case '\'': > - case '"': > - case '`': > - case ' ': > - case '#': > - goto bad; > - default: > - break; > - } > - } > - } while (*++cp); > - return (1); > - > -bad: fprintf(stderr, "%s: invalid user name\n", cp0); > - return (0); > -} > - > BUF * > allocbuf(BUF *bp, int fd, int blksize) > { > --- a/scpmisc.c > +++ b/scpmisc.c > @@ -104,16 +104,6 @@ > } > > char * > -cleanhostname(char *host) > -{ > - if (*host == '[' && host[strlen(host) - 1] == ']') { > - host[strlen(host) - 1] = '\0'; > - return (host + 1); > - } else > - return host; > -} > - > -char * > colon(char *cp) > { > int flag = 0; > @@ -165,26 +155,6 @@ > } > > void > -replacearg(arglist *args, u_int which, char *fmt, ...) > -{ > - va_list ap; > - char *cp; > - int r; > - > - va_start(ap, fmt); > - r = vasprintf(&cp, fmt, ap); > - va_end(ap); > - if (r == -1) > - fatal("replacearg: argument too long"); > - > - if (which >= args->num) > - fatal("replacearg: tried to replace invalid arg %d >= %d", > - which, args->num); > - xfree(args->list[which]); > - args->list[which] = cp; > -} > - > -void > freeargs(arglist *args) > { > u_int i; > @@ -223,6 +193,7 @@ > va_start(args, fmt); > vfprintf(stderr, fmt, args); > va_end(args); > + fputc('\n', stderr); > exit(255); > } > > --- a/scpmisc.h > +++ b/scpmisc.h > @@ -21,7 +21,6 @@ > void unset_nonblock(int); > void set_nodelay(int); > int a2port(const char *); > -char *cleanhostname(char *); > char *colon(char *); > long convtime(const char *); > > @@ -34,7 +33,6 @@ > int nalloc; > }; > void addargs(arglist *, char *, ...); > -void replacearg(arglist *, u_int, char *, ...); > void freeargs(arglist *); > > /* from xmalloc.h */ -- Regards, Konstantin