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

login
register
mail settings
Submitter Jun Wu
Date Feb. 17, 2016, 3:08 p.m.
Message ID <f9a49a0f18b29248fb86.1455721739@x1c>
Download mbox | patch
Permalink /patch/13240/
State Accepted
Delegated to: Yuya Nishihara
Headers show

Comments

Jun Wu - Feb. 17, 2016, 3:08 p.m.
# HG changeset patch
# User Jun Wu <quark@fb.com>
# Date 1455721247 0
#      Wed Feb 17 15:00:47 2016 +0000
# Node ID f9a49a0f18b29248fb86f3b1b1693b77207ef4ac
# Parent  a87f14c869a38406eab0595b514b915e6b6391a6
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.
timeless - Feb. 17, 2016, 8:04 p.m.
Jun Wu wrote:
> 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.

Do you want to pass Debug as well?
(Debugger can't be easily)
Jun Wu - Feb. 17, 2016, 10:39 p.m.
On 02/17/2016 08:04 PM, timeless wrote:
> Do you want to pass Debug as well? (Debugger can't be easily)

I don't see it necessary for now. If the code runs normally without
issue, then dispatch will be called by commandserver runcommand with
--debug set. Therefore, command depending on --debug is expected to work
well.

--traceback is special since it will cover some code path before hitting
runcommand. A quick search indicates --debug does not affect the
behavior here.

Anyway, the current ui/config code may need some refactoring to make
things more clear (and chg to be more confident). I'm thinking about
this recently and may write a proposal.
Yuya Nishihara - Feb. 19, 2016, 2 p.m.
On Wed, 17 Feb 2016 15:08:59 +0000, Jun Wu wrote:
> # HG changeset patch
> # User Jun Wu <quark@fb.com>
> # Date 1455721247 0
> #      Wed Feb 17 15:00:47 2016 +0000
> # Node ID f9a49a0f18b29248fb86f3b1b1693b77207ef4ac
> # Parent  a87f14c869a38406eab0595b514b915e6b6391a6
> 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.

Pushed revised version to the clowncopter, thanks.

>  	char sockname[UNIX_PATH_MAX];
>  	char lockfile[UNIX_PATH_MAX];
>  	char pidfile[UNIX_PATH_MAX];
> +	size_t argsize;
> +	const char **args;

Since you've changed the type of argsize, I updated most of "int"s to size_t.

> + * 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)

s/sensetive/sensitive/g :)

> +static void setcmdserverargs(struct cmdserveropts *opts,
> +	int argc, const char *argv[])
> +{
> +	int i, step;
> +	opts->argsize = 0;
> +	for (i = 0, step = 1; i < argc; i += step, step = 1) {
> +		if (!argv[i]) continue;  /* pass clang-analyse */
> +		if (strcmp(argv[i], "--") == 0) break;
> +		int n = testsensetiveflag(argv[i]);
> +		if (n == 0 || i + n > argc) continue;
> +		opts->args = reallocx(opts->args, (n + opts->argsize) * sizeof(char*));
> +		memcpy(opts->args + opts->argsize, argv + i, sizeof(char*) * n);
> +		opts->argsize += n;
> +		step = n;
> +	}

Hmm, it will actually reallocate 'args' buffer incrementally. Though glibc
malloc() seems to reuse old 'args' location as there are no other malloc/free,
I don't think it is a good design.
Jun Wu - Feb. 19, 2016, 2:14 p.m.
Thanks and sorry for the spell mistakes etc. I'll be more careful when
sending remaining patches.

On 02/19/2016 02:00 PM, Yuya Nishihara wrote:
> s/sensetive/sensitive/g :)
Time to setup a (special?) vim spell checker!

> Hmm, it will actually reallocate 'args' buffer incrementally. Though
> glibc malloc() seems to reuse old 'args' location as there are no
> other malloc/free, I don't think it is a good design.
I was aware of this issue and the glibc behavior. Considered there are
not many flags in most cases, I think it's okay to not have an
additional pass calculating the length.
Yuya Nishihara - Feb. 19, 2016, 2:40 p.m.
On Fri, 19 Feb 2016 14:14:48 +0000, Jun Wu wrote:
> > Hmm, it will actually reallocate 'args' buffer incrementally. Though
> > glibc malloc() seems to reuse old 'args' location as there are no
> > other malloc/free, I don't think it is a good design.
>
> I was aware of this issue and the glibc behavior. Considered there are
> not many flags in most cases, I think it's okay to not have an
> additional pass calculating the length.

Yep, it's okay and simple. FWIW, avoiding unnecessary realloc() won't be
that hard.

     cap = 128
     if (req > cap) {
         cap *= 2
         realloc(args, cap)
     }

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,75 @@ 
 	char sockname[UNIX_PATH_MAX];
 	char lockfile[UNIX_PATH_MAX];
 	char pidfile[UNIX_PATH_MAX];
+	size_t argsize;
+	const char **args;
 };
 
+static void initcmdserveropts(struct cmdserveropts *opts) {
+	memset(opts, 0, sizeof(struct cmdserveropts));
+}
+
+static void freecmdserveropts(struct cmdserveropts *opts) {
+	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 const struct {
+		const char *name;
+		const size_t 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) {
+		size_t len = strlen(flags[i].name);
+		size_t narg = flags[i].narg;
+		if (memcmp(arg, flags[i].name, len) == 0) {
+			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;
+}
+
+/*
+ * Parse argv[] and put sensitive flags to opts->args
+ */
+static void setcmdserverargs(struct cmdserveropts *opts,
+	int argc, const char *argv[])
+{
+	int i, step;
+	opts->argsize = 0;
+	for (i = 0, step = 1; i < argc; i += step, step = 1) {
+		if (!argv[i]) continue;  /* pass clang-analyse */
+		if (strcmp(argv[i], "--") == 0) break;
+		int n = testsensetiveflag(argv[i]);
+		if (n == 0 || i + n > argc) continue;
+		opts->args = reallocx(opts->args, (n + opts->argsize) * sizeof(char*));
+		memcpy(opts->args + opts->argsize, argv + i, sizeof(char*) * n);
+		opts->argsize += n;
+		step = n;
+	}
+}
+
 static void preparesockdir(const char *sockdir)
 {
 	int r;
@@ -111,7 +178,7 @@ 
 	if (!hgcmd || hgcmd[0] == '\0')
 		hgcmd = "hg";
 
-	const char *argv[] = {
+	const char *baseargv[] = {
 		hgcmd,
 		"serve",
 		"--cwd", "/",
@@ -122,10 +189,18 @@ 
 		"--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 = mallocx(sizeof(char*) * argsize);
+	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 +427,9 @@ 
 		enabledebugmsg();
 
 	struct cmdserveropts opts;
+	initcmdserveropts(&opts);
 	setcmdserveropts(&opts);
+	setcmdserverargs(&opts, argc, argv);
 
 	if (argc == 2) {
 		int sig = 0;
@@ -379,5 +456,6 @@ 
 	setuppager(hgc, argv + 1, argc - 1);
 	int exitcode = hgc_runcommand(hgc, argv + 1, argc - 1);
 	hgc_close(hgc);
+	freecmdserveropts(&opts);
 	return exitcode;
 }