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

Jeff King wrote:

On Tue, Nov 15, 2011 at 11:22:48AM +0100, Thomas Rast wrote:

quoted

I see valgrind failures bisecting to this commit, like so:

==19125== Syscall param execve(argv[i]) points to unaddressable byte(s)
==19125==    at 0x5303CB7: execve (in /lib64/libc-2.11.3.so)
==19125==    by 0x53045A5: execvpe (in /lib64/libc-2.11.3.so)
==19125==    by 0x4B183C: execv_git_cmd (exec_cmd.c:137)
==19125==    by 0x4F305E: start_command (run-command.c:277)
==19125==    by 0x47F5C9: cmd_upload_archive (upload-archive.c:98)
==19125==    by 0x4051F4: run_builtin (git.c:308)
==19125==    by 0x40538F: handle_internal_command (git.c:466)
==19125==    by 0x405556: main (git.c:553)
==19125==  Address 0x7feffe7d0 is not stack'd, malloc'd or (recently) free'd

when running 'make valgrind' in current master.  Let me know if you
need more information.

With which test, and on what OS? I couldn't replicate running
t5000 on Linux.

11, and many others after it, on a pretty vanilla opensuse 11.4 VM I
use almost exclusively for the valgrind runs.  I used a pre-3.7 SVN
valgrind build, but 3.6.1 (shipped with opensuse) finds the same.

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:

diff --git i/builtin/upload-archive.c w/builtin/upload-archive.c
index c57e8bd..6ab50d3 100644
--- i/builtin/upload-archive.c
+++ w/builtin/upload-archive.c

@@ -22,9 +22,10 @@ static const char lostchild[] =
 static void prepare_argv(const char **sent_argv, const char **argv)
 {
 	const char *arg_cmd = "argument ";
-	char *p, buf[4096];
+	char *p, *buf;
 	int sent_argc;
 	int len;
+	buf = xmalloc(4096);
 
 	/* put received options in sent_argv[] */
 	sent_argc = 2;

@@ -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 + 4096) - p);
 		if (len == 0)
 			break;	/* got a flush */
 		if (sent_argc > MAX_ARGS - 2)

-- 
Thomas Rast
trast@{inf,student}.ethz.ch