Re: [PATCH v4 3/3] upload-archive: use start_command instead of fork
From: Andreas Schwab <hidden>
Date: 2016-06-15 22:52:29
Subsystem:
the rest · Maintainer:
Linus Torvalds
Possibly related (same subject, not in this thread)
- 2016-06-15 · Re: [PATCH v4 3/3] upload-archive: use start_command instead of fork · Thomas Rast <hidden>
- 2016-06-15 · Re: [PATCH v4 3/3] upload-archive: use start_command instead of fork · Jeff King <hidden>
- 2016-06-15 · Re: [PATCH v4 3/3] upload-archive: use start_command instead of fork · Thomas Rast <hidden>
- 2016-06-15 · Re: [PATCH v4 3/3] upload-archive: use start_command instead of fork · Jeff King <hidden>
- 2016-06-15 · Re: [PATCH v4 3/3] upload-archive: use start_command instead of fork · Erik Faye-Lund <hidden>
Thomas Rast [off-list ref] 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."