Re: [PATCH v4 3/3] upload-archive: use start_command instead of fork
Thomas Rast <trast@student.ethz.ch> writes:
Show 27 quoted lines
> 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."
Andreas Schwab <schwab@linux-m68k.org>