Re: [PATCH v4 3/3] upload-archive: use start_command instead of fork

[Andreas Schwab <schwab@linux-m68k.org>]

Thomas Rast <trast@student.ethz.ch> writes:

> But after a closer look I think this patch just prodded it enough to
> unearth long-existing undefined behaviour: prepare_argv() summarizes
> to something like
>
> static void prepare_argv(const char **sent_argv, const char **argv)
> {
> 	char *p, buf[4096];
>
> 	for (p = buf;;) {
> 		len = packet_read_line(0, p, (buf + sizeof buf) - p);
> 		/* ... p always points into buf ... */
> 		sent_argv[sent_argc++] = p;
> 		p += len;
> 		*p++ = 0;
> 	}
> 	sent_argv[sent_argc] = NULL;
> }
>
> The code appears to have looked like this ever since the addition of
> that file back in 39345a2 (Add git-upload-archive, 2006-09-07).  So
> the elements of sent_argv have apparently always pointed into the
> stack-allocated 'buf'.
>
> (This correlates with the "Address 0x7feffe7d0 is not stack'd", even
> though it's pretty clearly an address into the stack.)
>
> A quick band-aid would be to heap-allocate it instead:

Or allocate it in the caller:

diff --git a/builtin/upload-archive.c b/builtin/upload-archive.c
index c57e8bd..f0f843e 100644
--- a/builtin/upload-archive.c
+++ b/builtin/upload-archive.c
@@ -18,11 +18,12 @@ static const char lostchild[] =
 "git upload-archive: archiver process was lost";
 
 #define MAX_ARGS (64)
+#define ARGV_BUF_SIZE 4096
 
-static void prepare_argv(const char **sent_argv, const char **argv)
+static void prepare_argv(const char **sent_argv, char *buf, const char **argv)
 {
 	const char *arg_cmd = "argument ";
-	char *p, buf[4096];
+	char *p;
 	int sent_argc;
 	int len;
 
@@ -32,7 +33,7 @@ static void prepare_argv(const char **sent_argv, const char **argv)
 	sent_argv[1] = "--remote-request";
 	for (p = buf;;) {
 		/* This will die if not enough free space in buf */
-		len = packet_read_line(0, p, (buf + sizeof buf) - p);
+		len = packet_read_line(0, p, (buf + ARGV_BUF_SIZE) - p);
 		if (len == 0)
 			break;	/* got a flush */
 		if (sent_argc > MAX_ARGS - 2)
@@ -85,6 +86,7 @@ int cmd_upload_archive(int argc, const char **argv, const char *prefix)
 {
 	const char *sent_argv[MAX_ARGS];
 	struct child_process cld = { sent_argv };
+	char argv_buf[ARGV_BUF_SIZE];
 	cld.out = cld.err = -1;
 	cld.git_cmd = 1;
 
@@ -94,7 +96,7 @@ int cmd_upload_archive(int argc, const char **argv, const char *prefix)
 	if (!enter_repo(argv[1], 0))
 		die("'%s' does not appear to be a git repository", argv[1]);
 
-	prepare_argv(sent_argv, argv);
+	prepare_argv(sent_argv, argv_buf, argv);
 	if (start_command(&cld)) {
 		int err = errno;
 		packet_write(1, "NACK fork failed on the remote side\n");
-- 
1.7.7.3


Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."