Patchwork [1,of,3] chg: pass sensitive command line flags to server

login
register
mail settings
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

Jun Wu - Feb. 16, 2016, 11:12 a.m.
# 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

We are going to make chgserver load repo config, remember what it is, and
load repo config again to detect config change. This is the first step that
passes config, repo, cwd options to server. Traceback is passed as well to
cover errors before hitting chgserver.runcommand.
Yuya Nishihara - Feb. 16, 2016, 4:10 p.m.
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.
Yuya Nishihara - Feb. 16, 2016, 4:16 p.m.
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.
Jun Wu - Feb. 16, 2016, 4:54 p.m.
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;
 }