| Submitter | Jun Wu |
|---|---|
| Date | Feb. 16, 2016, 11:12 a.m. |
| Message ID | <3fc701a86323ae896371.1455621120@x1c> |
| Download | mbox | patch |
| Permalink | /patch/13223/ |
| State | Superseded |
| Commit | 66f6dad20c19f9a27551ecd2f0ecb816939fdf3c |
| Delegated to: | Yuya Nishihara |
| Headers | show |
Comments
On Tue, 16 Feb 2016 11:12:00 +0000, Jun Wu wrote: > # HG changeset patch > # User Jun Wu <quark@fb.com> > # Date 1455618922 0 > # Tue Feb 16 10:35:22 2016 +0000 > # Node ID 3fc701a86323ae896371ec3df375a52bce06b748 > # Parent 923500ca4f9a294db25318db07239359ad131348 > chg: pass sensitive command line flags to server A couple of comments follow, but they are mostly nitpicking. > +static void freecmdserveropts(struct cmdserveropts *opts) { > + if (opts->args) { > + free(opts->args); > + opts->args = NULL; > + opts->argsize = 0; > + } free(NULL) is valid, and chg does it at several places. > +static int testsensetiveflag(const char *arg) > +{ > + static struct { > + const char *name; > + const int narg; Perhaps you meant static const struct { const char *name; int narg; } > + size_t i; > + for (i = 0; i < sizeof(flags) / sizeof(flags[0]); ++i) { > + int len = strlen(flags[i].name); "len" can be size_t. > + int narg = flags[i].narg; > + if (!memcmp(arg, flags[i].name, len)) { I tend to write it as memcmp() == 0 because the return value is not boolean. But I'm fine with that if you prefer !memcmp(). > +static void *reallocx(void *ptr, size_t size) > +{ > + void *result = realloc(ptr, size); > + if (!result) abortmsg("failed to realloc"); > + return result; > +} Can it be moved util.c? hgclient.c has the same pattern. > +static void setcmdserverargs(struct cmdserveropts *opts, > + int argc, const char *argv[]) > +{ > + int i, argsize = 0; > + const char **args = NULL; > + for (i = 0; i < argc; ++i) { > + if (!argv[i]) continue; /* pass clang-analyse */ > + if (!strcmp(argv[i], "--")) break; > + int n = testsensetiveflag(argv[i]); > + if (n == 0 || i + n > argc) continue; > + args = reallocx(args, (n + argsize) * sizeof(char*)); > + memcpy(args + argsize, argv + i, sizeof(char*) * n); > + argsize += n; > + i += n - 1; Hmm, isn't it clearer to use "while" than "for" ? while (i < argc) { ... i += n; } How can we handle "ci -m --config foo" ? Sorry, I didn't notice it before. > + } > + if (opts->args) free(opts->args); Perhaps, we can realloc() old opts->args.
On Wed, 17 Feb 2016 01:10:05 +0900, Yuya Nishihara wrote: > How can we handle "ci -m --config foo" ? > Sorry, I didn't notice it before. Never mind. It doesn't work on hg anyway.
Thanks for the review! Replied inline. On 02/16/2016 04:10 PM, Yuya Nishihara wrote: > free(NULL) is valid, and chg does it at several places. Ah, will remove "if" then. > Perhaps you meant > "len" can be size_t. Will update. > I tend to write it as memcmp() == 0 because the return value is not > boolean. But I'm fine with that if you prefer !memcmp(). I agree == 0 is more clear. Using '!' is just to make code shorter. Will do the change. > Can it be moved util.c? hgclient.c has the same pattern. Good idea. Then it will be in a separate patch. > Hmm, isn't it clearer to use "while" than "for" ? I don't have a preference on this. Will change then. > How can we handle "ci -m --config foo" ? Sorry, I didn't notice it > before. It scared me a bit. But I soon discovered from debugshell: dispatch._earlygetopt(['--config'], ['ci', '-m', '--config', 'foo']) will return ['foo'] so it's not an issue. Side note, if we have to implement full command line parser logic in C, we can have a fast path to fallback to original hg if we saw 'serve', '--time'. > Perhaps, we can realloc() old opts->args. Yes.
Patch
diff --git a/contrib/chg/chg.c b/contrib/chg/chg.c --- a/contrib/chg/chg.c +++ b/contrib/chg/chg.c @@ -32,8 +32,87 @@ char sockname[UNIX_PATH_MAX]; char lockfile[UNIX_PATH_MAX]; char pidfile[UNIX_PATH_MAX]; + int argsize; + const char **args; }; +static void initcmdserveropts(struct cmdserveropts *opts) { + memset(opts, 0, sizeof(struct cmdserveropts)); +} + +static void freecmdserveropts(struct cmdserveropts *opts) { + if (opts->args) { + free(opts->args); + opts->args = NULL; + opts->argsize = 0; + } +} + +/* + * Test if an argument is a sensitive flag that should be passed to the server. + * Return 0 if not, otherwise the number of arguments starting from the current + * one that should be passed to the server. + */ +static int testsensetiveflag(const char *arg) +{ + static struct { + const char *name; + const int narg; + } flags[] = { + {"--config", 1}, + {"--cwd", 1}, + {"--repo", 1}, + {"--repository", 1}, + {"--traceback", 0}, + {"-R", 1}, + }; + size_t i; + for (i = 0; i < sizeof(flags) / sizeof(flags[0]); ++i) { + int len = strlen(flags[i].name); + int narg = flags[i].narg; + if (!memcmp(arg, flags[i].name, len)) { + if (arg[len] == '\0') { /* --flag (value) */ + return narg + 1; + } else if (arg[len] == '=' && narg > 0) { /* --flag=value */ + return 1; + } else if (flags[i].name[1] != '-') { /* short flag */ + return 1; + } + } + } + return 0; +} + +static void *reallocx(void *ptr, size_t size) +{ + void *result = realloc(ptr, size); + if (!result) abortmsg("failed to realloc"); + return result; +} + +/* + * Parse argv[] and put sensitive flags to opts->args + */ +static void setcmdserverargs(struct cmdserveropts *opts, + int argc, const char *argv[]) +{ + int i, argsize = 0; + const char **args = NULL; + for (i = 0; i < argc; ++i) { + if (!argv[i]) continue; /* pass clang-analyse */ + if (!strcmp(argv[i], "--")) break; + int n = testsensetiveflag(argv[i]); + if (n == 0 || i + n > argc) continue; + args = reallocx(args, (n + argsize) * sizeof(char*)); + memcpy(args + argsize, argv + i, sizeof(char*) * n); + argsize += n; + i += n - 1; + } + if (opts->args) free(opts->args); + opts->args = args; + opts->argsize = argsize; +} + static void preparesockdir(const char *sockdir) { int r; @@ -111,7 +190,7 @@ if (!hgcmd || hgcmd[0] == '\0') hgcmd = "hg"; - const char *argv[] = { + const char *baseargv[] = { hgcmd, "serve", "--cwd", "/", @@ -122,10 +201,19 @@ "--config", "extensions.chgserver=", /* wrap root ui so that it can be disabled/enabled by config */ "--config", "progress.assume-tty=1", - NULL, }; + int baseargvsize = sizeof(baseargv) / sizeof(baseargv[0]); + int argsize = baseargvsize + opts->argsize + 1; + + const char **argv = malloc(sizeof(char*) * argsize); + if (!argv) abortmsg("failed to malloc"); + memcpy(argv, baseargv, sizeof(baseargv)); + memcpy(argv + baseargvsize, opts->args, sizeof(char*) * opts->argsize); + argv[argsize - 1] = NULL; + if (execvp(hgcmd, (char **)argv) < 0) abortmsg("failed to exec cmdserver (errno = %d)", errno); + free(argv); } /* @@ -352,7 +440,9 @@ enabledebugmsg(); struct cmdserveropts opts; + initcmdserveropts(&opts); setcmdserveropts(&opts); + setcmdserverargs(&opts, argc, argv); if (argc == 2) { int sig = 0; @@ -379,5 +469,6 @@ setuppager(hgc, argv + 1, argc - 1); int exitcode = hgc_runcommand(hgc, argv + 1, argc - 1); hgc_close(hgc); + freecmdserveropts(&opts); return exitcode; }