Re: [PATCH] quickfetch(): Prevent overflow of the rev-list command line

Johan Herland [off-list ref] writes:

Maybe I need to do something to the close() call as well? What happens
on close() after EPIPE?

You should be OK (you could try this).

-- >8 --
#include <stdio.h>
#include <errno.h>
#include <stdlib.h>
#include <signal.h>
#include <string.h>

int main(int ac, char **av)
{
	int pipefd[2];
	int child;

	if (pipe(pipefd) < 0) {
		fprintf(stderr, "pipe failed: %s\n", strerror(errno));
		exit(1);
	}
	child = fork();
	if (child < 0) {
		fprintf(stderr, "fork failed: %s\n", strerror(errno));
		exit(1);
	} else if (child == 0) {
		char buf[1024];
		ssize_t sz;

		/* the child reads from the parent but does not talk back */
		close(pipefd[1]);

		/* emulate reading a bit, then dying without cleaning up */
		sz = read(pipefd[0], buf, sizeof(buf));
		fprintf(stderr, "read %lu bytes, and will die\n",
			(unsigned long) sz);
		exit(1);
	} else {
		const char data[] = "abcdefg";
		size_t len = sizeof(data);
		size_t written = 0;

		/* the parent writes to the child but does not listen */
		close(pipefd[0]);

		/* we will rite to the pipe even after the child is gone */
		signal(SIGPIPE, SIG_IGN);

		/* write, write, write, ... */
		while (1) {
			ssize_t sz = write(pipefd[1], data, len);
			if (sz < 0) {
				/* error */
				fprintf(stderr,
					"write failed (%s) after writing"
					" %lu bytes\n",
					strerror(errno),
					(unsigned long) written);
				break;
			}
			written += sz;
		}
		errno = 0;
		if (close(pipefd[1]))
			fprintf(stderr, "close failed: %s\n", strerror(errno));
		else
			fprintf(stderr, "close ok\n");
	}
	exit(0);
}

On Wednesday 08 July 2009, Junio C Hamano wrote:

Johan Herland [off-list ref] writes:

quoted

Maybe I need to do something to the close() call as well? What happens
on close() after EPIPE?

You should be OK (you could try this).

-- >8 --

Thanks! The programs works well on my Linux box (close() succeeds), but it 
does not run at all in Windows/MSYS (lacks pipe() and fork()).

Does anybody with Windows/MSYS experience know how this scenario (write() to 
a terminated process, followed by close()) would play out in msysGit?


...Johan

-- 
Johan Herland, [off-list ref]
www.herland.net

On Thu, Jul 9, 2009 at 10:43, Johan Herland[off-list ref] wrote:

On Wednesday 08 July 2009, Junio C Hamano wrote:

quoted

Johan Herland [off-list ref] writes:

quoted

Maybe I need to do something to the close() call as well? What happens
on close() after EPIPE?

Does anybody with Windows/MSYS experience know how this scenario (write() to
a terminated process, followed by close()) would play out in msysGit?

It fails with ERROR_BROKEN_PIPE. See MSDN for WriteFile:

 http://msdn.microsoft.com/en-us/library/aa365747%28VS.85%29.aspx

(look for the error above).

Well, sometimes it just fails, so you can hardly use the error code to detect
if the other process is truly gone or something broke in Windows.

Johan Herland schrieb:

Does anybody with Windows/MSYS experience know how this scenario (write() to 
a terminated process, followed by close()) would play out in msysGit?

The first write() sometimes fails with EPIPE, otherwise it fails with
EINVAL. All subsequent write()s fail with EINVAL. The setting of SIGPIPE
is irrelevant because it is unknown to Windows.

There's precedent already in write_or_die.c. You should not write the
error message for both EPIPE and EINVAL.

-- Hannes

On Thursday 09 July 2009, Johannes Sixt wrote:

Johan Herland schrieb:

quoted

Does anybody with Windows/MSYS experience know how this scenario
(write() to a terminated process, followed by close()) would play out
in msysGit?

The first write() sometimes fails with EPIPE, otherwise it fails with
EINVAL. All subsequent write()s fail with EINVAL. The setting of SIGPIPE
is irrelevant because it is unknown to Windows.

There's precedent already in write_or_die.c. You should not write the
error message for both EPIPE and EINVAL.

Thanks, but what about the subsequent close()? Will it fail with EINVAL? 
EBADF? or will is succeed (like on Linux)?

I will send an updated patch with all fixes, as soon as I know what to do 
about close().


...Johan

-- 
Johan Herland, [off-list ref]
www.herland.net

Johan Herland schrieb:

Thanks, but what about the subsequent close()? Will it fail with EINVAL? 
EBADF? or will is succeed (like on Linux)?

I guess it succeeds, but I don't have a quick test. Send your patch, and
we'll see ;)

-- Hannes

quickfetch() calls rev-list to check whether the objects we are about to
fetch are already present in the repo (if so, we can skip the object fetch).
However, when there are many (~1000) refs to be fetched, the rev-list
command line grows larger than the maximum command line size on some systems
(32K in Windows). This causes rev-list to fail, making quickfetch() return
non-zero, which unnecessarily triggers the transport machinery. This somehow
causes fetch to fail with an exit code.

By using the --stdin option to rev-list (and feeding the object list to its
standard input), we prevent the overflow of the rev-list command line,
which causes quickfetch(), and subsequently the overall fetch, to succeed.

However, using rev-list --stdin is not entirely straightforward: rev-list
terminates immediately when encountering an unknown object, which can
trigger SIGPIPE if we are still writing object's to its standard input.
We therefore ignore SIGPIPE so that the fetch process is not terminated.

Signed-off-by: Johan Herland <redacted>
Improved-by: Johannes Sixt [off-list ref]
Improved-by: Alex Riesen [off-list ref]
Tested-by: Peter Krefting <redacted>
---
 builtin-fetch.c |   62 +++++++++++++++++++++++++++++++------------------------
 1 files changed, 35 insertions(+), 27 deletions(-)

diff --git a/builtin-fetch.c b/builtin-fetch.c
index cd5eb9a..20bcbdd 100644
--- a/builtin-fetch.c
+++ b/builtin-fetch.c

@@ -400,14 +400,14 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,
 
 /*
  * We would want to bypass the object transfer altogether if
- * everything we are going to fetch already exists and connected
+ * everything we are going to fetch already exists and is connected
  * locally.
  *
- * The refs we are going to fetch are in to_fetch (nr_heads in
- * total).  If running
+ * The refs we are going to fetch are in ref_map.  If running
  *
- *  $ git rev-list --objects to_fetch[0] to_fetch[1] ... --not --all
+ *  $ git rev-list --objects --stdin --not --all
  *
+ * (feeding all the refs in ref_map on its standard input)
  * does not error out, that means everything reachable from the
  * refs we are going to fetch exists and is connected to some of
  * our existing refs.

@@ -416,8 +416,9 @@ static int quickfetch(struct ref *ref_map)
 {
 	struct child_process revlist;
 	struct ref *ref;
-	char **argv;
-	int i, err;
+	int err;
+	const char *argv[] = {"rev-list",
+		"--quiet", "--objects", "--stdin", "--not", "--all", NULL};
 
 	/*
 	 * If we are deepening a shallow clone we already have these

@@ -429,34 +430,41 @@ static int quickfetch(struct ref *ref_map)
 	if (depth)
 		return -1;
 
-	for (i = 0, ref = ref_map; ref; ref = ref->next)
-		i++;
-	if (!i)
+	if (!ref_map)
 		return 0;
 
-	argv = xmalloc(sizeof(*argv) * (i + 6));
-	i = 0;
-	argv[i++] = xstrdup("rev-list");
-	argv[i++] = xstrdup("--quiet");
-	argv[i++] = xstrdup("--objects");
-	for (ref = ref_map; ref; ref = ref->next)
-		argv[i++] = xstrdup(sha1_to_hex(ref->old_sha1));
-	argv[i++] = xstrdup("--not");
-	argv[i++] = xstrdup("--all");
-	argv[i++] = NULL;
-
 	memset(&revlist, 0, sizeof(revlist));
-	revlist.argv = (const char**)argv;
+	revlist.argv = argv;
 	revlist.git_cmd = 1;
-	revlist.no_stdin = 1;
 	revlist.no_stdout = 1;
 	revlist.no_stderr = 1;
-	err = run_command(&revlist);
+	revlist.in = -1;
+
+	/* If rev-list --stdin encounters an unknown commit, it terminates,
+	 * which will cause SIGPIPE in the write loop below. */
+	signal(SIGPIPE, SIG_IGN);
+
+	err = start_command(&revlist);
+	if (err) {
+		error("could not run rev-list");
+		return err;
+	}
 
-	for (i = 0; argv[i]; i++)
-		free(argv[i]);
-	free(argv);
-	return err;
+	for (ref = ref_map; ref; ref = ref->next) {
+		if (write_in_full(revlist.in, sha1_to_hex(ref->old_sha1), 40) < 0 ||
+		    write_in_full(revlist.in, "\n", 1) < 0) {
+			err = errno;
+			if (err != EPIPE && err != EINVAL)
+				error("failed write to rev-list");
+			break;
+		}
+	}
+
+	if (close(revlist.in)) {
+		err = errno;
+		error("failed to close rev-list's stdin");
+	}
+	return finish_command(&revlist) || err;
 }
 
 static int fetch_refs(struct transport *transport, struct ref *ref_map)

-- 
1.6.3.rc0.1.gf800

Johan Herland schrieb:

However, using rev-list --stdin is not entirely straightforward: rev-list
terminates immediately when encountering an unknown object, which can
trigger SIGPIPE if we are still writing object's to its standard input.
We therefore ignore SIGPIPE so that the fetch process is not terminated.

I removed the "signal(SIGPIPE, SIG_IGN)", but the test suite still passes.
IOW, there is no test case that has the configuration that you describe
here. Would you please add such a test (perhaps in t5502)? It would also
help me verify the patch works as intended on Windows.

Signed-off-by: Johan Herland <redacted>
Improved-by: Johannes Sixt [off-list ref]

Please make this [off-list ref] despite the email address I'm using right now.

Improved-by: Alex Riesen [off-list ref]
Tested-by: Peter Krefting <redacted>

+	for (ref = ref_map; ref; ref = ref->next) {
+		if (write_in_full(revlist.in, sha1_to_hex(ref->old_sha1), 40) < 0 ||
+		    write_in_full(revlist.in, "\n", 1) < 0) {
+			err = errno;
+			if (err != EPIPE && err != EINVAL)
+				error("failed write to rev-list");
+			break;
+		}
+	}
+
+	if (close(revlist.in)) {
+		err = errno;
+		error("failed to close rev-list's stdin");
+	}
+	return finish_command(&revlist) || err;

The call site of quickfetch() is not interested in the errno, only on
whether the return value is non-zero: You can just assign -1 to err
(that's our convention for failure). OTOH, it would be helpful to include
strerror(errno) in the error message.

Shouldn't you reset signal(SIGPIPE) to its previous value?

-- Hannes

quickfetch() calls rev-list to check whether the objects we are about to
fetch are already present in the repo (if so, we can skip the object fetch).
However, when there are many (~1000) refs to be fetched, the rev-list
command line grows larger than the maximum command line size on some systems
(32K in Windows). This causes rev-list to fail, making quickfetch() return
non-zero, which unnecessarily triggers the transport machinery. This somehow
causes fetch to fail with an exit code.

By using the --stdin option to rev-list (and feeding the object list to its
standard input), we prevent the overflow of the rev-list command line,
which causes quickfetch(), and subsequently the overall fetch, to succeed.

However, using rev-list --stdin is not entirely straightforward: rev-list
terminates immediately when encountering an unknown object, which can
trigger SIGPIPE if we are still writing object's to its standard input.
We therefore temporarily ignore SIGPIPE so that the fetch process is not
terminated.

The patch also contains a testcase to verify the fix (note that before
the patch, the testcase would only fail on msysGit).

Signed-off-by: Johan Herland <redacted>
Improved-by: Johannes Sixt [off-list ref]
Improved-by: Alex Riesen [off-list ref]
Tested-by: Peter Krefting <redacted>
---

On Thursday 09 July 2009, Johannes Sixt wrote:

Would you please add such a test (perhaps in t5502)? It
would also help me verify the patch works as intended on Windows.

Done (although somewhat naively). I don't have an msysgit setup to test
this, but faking the failure condition in quickfetch() (return -1 if
#refs > 800) does trigger the selftest (the second git fetch fails).

I could add a separate pre-patch introducing the selftest with
test_expect_failure, but that would only confuse non-msysgit users
where the test succeeds both before and after the fix.

Please make this [off-list ref] despite the email address I'm using right
now.

Ok.

The call site of quickfetch() is not interested in the errno, only on
whether the return value is non-zero: You can just assign -1 to err
(that's our convention for failure). OTOH, it would be helpful to include
strerror(errno) in the error message.

Fixed.

Shouldn't you reset signal(SIGPIPE) to its previous value?

Done (provided that the sigchain_push/pop infrastructure works the way
I expect).

Thanks a lot for your review and suggestions.


Have fun! :)

...Johan


 builtin-fetch.c       |   65 ++++++++++++++++++++++++++++--------------------
 t/t5502-quickfetch.sh |   20 +++++++++++++++
 2 files changed, 58 insertions(+), 27 deletions(-)

diff --git a/builtin-fetch.c b/builtin-fetch.c
index cd5eb9a..2e3c609 100644
--- a/builtin-fetch.c
+++ b/builtin-fetch.c

@@ -400,14 +400,14 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,
 
 /*
  * We would want to bypass the object transfer altogether if
- * everything we are going to fetch already exists and connected
+ * everything we are going to fetch already exists and is connected
  * locally.
  *
- * The refs we are going to fetch are in to_fetch (nr_heads in
- * total).  If running
+ * The refs we are going to fetch are in ref_map.  If running
  *
- *  $ git rev-list --objects to_fetch[0] to_fetch[1] ... --not --all
+ *  $ git rev-list --objects --stdin --not --all
  *
+ * (feeding all the refs in ref_map on its standard input)
  * does not error out, that means everything reachable from the
  * refs we are going to fetch exists and is connected to some of
  * our existing refs.

@@ -416,8 +416,9 @@ static int quickfetch(struct ref *ref_map)
 {
 	struct child_process revlist;
 	struct ref *ref;
-	char **argv;
-	int i, err;
+	int err;
+	const char *argv[] = {"rev-list",
+		"--quiet", "--objects", "--stdin", "--not", "--all", NULL};
 
 	/*
 	 * If we are deepening a shallow clone we already have these

@@ -429,34 +430,44 @@ static int quickfetch(struct ref *ref_map)
 	if (depth)
 		return -1;
 
-	for (i = 0, ref = ref_map; ref; ref = ref->next)
-		i++;
-	if (!i)
+	if (!ref_map)
 		return 0;
 
-	argv = xmalloc(sizeof(*argv) * (i + 6));
-	i = 0;
-	argv[i++] = xstrdup("rev-list");
-	argv[i++] = xstrdup("--quiet");
-	argv[i++] = xstrdup("--objects");
-	for (ref = ref_map; ref; ref = ref->next)
-		argv[i++] = xstrdup(sha1_to_hex(ref->old_sha1));
-	argv[i++] = xstrdup("--not");
-	argv[i++] = xstrdup("--all");
-	argv[i++] = NULL;
-
 	memset(&revlist, 0, sizeof(revlist));
-	revlist.argv = (const char**)argv;
+	revlist.argv = argv;
 	revlist.git_cmd = 1;
-	revlist.no_stdin = 1;
 	revlist.no_stdout = 1;
 	revlist.no_stderr = 1;
-	err = run_command(&revlist);
+	revlist.in = -1;
+
+	/* If rev-list --stdin encounters an unknown commit, it terminates,
+	 * which will cause SIGPIPE in the write loop below. */
+	sigchain_push(SIGPIPE, SIG_IGN);
+
+	err = start_command(&revlist);
+	if (err) {
+		error("could not run rev-list");
+		return err;
+	}
+
+	for (ref = ref_map; ref; ref = ref->next) {
+		if (write_in_full(revlist.in, sha1_to_hex(ref->old_sha1), 40) < 0 ||
+		    write_in_full(revlist.in, "\n", 1) < 0) {
+			if (err != EPIPE && err != EINVAL)
+				error("failed write to rev-list: %s", strerror(errno));
+			err = -1;
+			break;
+		}
+	}
+
+	if (close(revlist.in)) {
+		error("failed to close rev-list's stdin: %s", strerror(errno));
+		err = -1;
+	}
+
+	sigchain_pop(SIGPIPE);
 
-	for (i = 0; argv[i]; i++)
-		free(argv[i]);
-	free(argv);
-	return err;
+	return finish_command(&revlist) || err;
 }
 
 static int fetch_refs(struct transport *transport, struct ref *ref_map)

diff --git a/t/t5502-quickfetch.sh b/t/t5502-quickfetch.sh
index 16eadd6..1037a72 100755
--- a/t/t5502-quickfetch.sh
+++ b/t/t5502-quickfetch.sh

@@ -119,4 +119,24 @@ test_expect_success 'quickfetch should not copy from alternate' '
 
 '
 
+test_expect_success 'quickfetch should handle ~1000 refs (on Windows)' '
+
+	git gc &&
+	head=$(git rev-parse HEAD) &&
+	branchprefix="$head refs/heads/branch" &&
+	for i in 0 1 2 3 4 5 6 7 8 9; do
+		for j in 0 1 2 3 4 5 6 7 8 9; do
+			for k in 0 1 2 3 4 5 6 7 8 9; do
+				echo "$branchprefix$i$j$k" >> .git/packed-refs
+			done
+		done
+	done &&
+	(
+		cd cloned &&
+		git fetch &&
+		git fetch
+	)
+
+'
+
 test_done

-- 
1.6.3.rc0.1.gf800

Johan Herland schrieb:

On Thursday 09 July 2009, Johannes Sixt wrote:

quoted

Shouldn't you reset signal(SIGPIPE) to its previous value?

Done (provided that the sigchain_push/pop infrastructure works the way
I expect).

I'm not sure, either. Peff?

+test_expect_success 'quickfetch should handle ~1000 refs (on Windows)' '
+
+	git gc &&
+	head=$(git rev-parse HEAD) &&
+	branchprefix="$head refs/heads/branch" &&
+	for i in 0 1 2 3 4 5 6 7 8 9; do
+		for j in 0 1 2 3 4 5 6 7 8 9; do
+			for k in 0 1 2 3 4 5 6 7 8 9; do
+				echo "$branchprefix$i$j$k" >> .git/packed-refs
+			done
+		done
+	done &&
+	(
+		cd cloned &&
+		git fetch &&
+		git fetch
+	)
+
+'

This test fails on Windows without the code change and passes with the
code change. So, it's a good test.

But actually I meant you to make a test that triggers the SIGPIPE that
would kill git-fetch if it were not ignored. This one doesn't trigger it,
either.

-- Hannes

On Thu, Jul 09, 2009 at 04:21:09PM +0200, Johannes Sixt wrote:

Johan Herland schrieb:

quoted

On Thursday 09 July 2009, Johannes Sixt wrote:

quoted

Shouldn't you reset signal(SIGPIPE) to its previous value?

Done (provided that the sigchain_push/pop infrastructure works the way
I expect).

I'm not sure, either. Peff?

I don't think I ever tried explicitly pushing SIG_IGN, but the
infrastructure was designed so that it would Just Work. So yes, I think
it's right, but you may want to test it. :)

That being said, in the patch in question there is an early return after
the push that misses the corresponding pop. That should be fixed.

-Peff

On Thursday 09 July 2009, Johannes Sixt wrote:

But actually I meant you to make a test that triggers the SIGPIPE that
would kill git-fetch if it were not ignored. This one doesn't trigger it,
either.

AFAIU from earlier in this thread (and a mail from Peter linking to 
http://markmail.org/message/dbgdj4csafen65ye), SIGPIPE _never_ triggers on 
Windows, thus ignoring SIGPIPE is not needed for the fix per se. However, as 
a side-effect of the fix, we may now get SIGPIPE on Linux (and other POSIX 
platforms), so although it never triggers on Windows, it's still needed.


...Johan

-- 
Johan Herland, [off-list ref]
www.herland.net

quickfetch() calls rev-list to check whether the objects we are about to
fetch are already present in the repo (if so, we can skip the object fetch).
However, when there are many (~1000) refs to be fetched, the rev-list
command line grows larger than the maximum command line size on some systems
(32K in Windows). This causes rev-list to fail, making quickfetch() return
non-zero, which unnecessarily triggers the transport machinery. This somehow
causes fetch to fail with an exit code.

By using the --stdin option to rev-list (and feeding the object list to its
standard input), we prevent the overflow of the rev-list command line,
which causes quickfetch(), and subsequently the overall fetch, to succeed.

However, using rev-list --stdin is not entirely straightforward: rev-list
terminates immediately when encountering an unknown object, which can
trigger SIGPIPE if we are still writing object's to its standard input.
We therefore temporarily ignore SIGPIPE so that the fetch process is not
terminated.

The patch also contains a testcase to verify the fix (note that before
the patch, the testcase would only fail on msysGit).

Signed-off-by: Johan Herland <redacted>
Improved-by: Johannes Sixt [off-list ref]
Improved-by: Alex Riesen [off-list ref]
Tested-by: Peter Krefting <redacted>
---

On Thursday 09 July 2009, Jeff King wrote:

That being said, in the patch in question there is an early return after
the push that misses the corresponding pop. That should be fixed.

Thanks, fixed.

...Johan

-- 
Johan Herland, [off-list ref]
www.herland.net

 builtin-fetch.c       |   65 ++++++++++++++++++++++++++++--------------------
 t/t5502-quickfetch.sh |   20 +++++++++++++++
 2 files changed, 58 insertions(+), 27 deletions(-)

diff --git a/builtin-fetch.c b/builtin-fetch.c
index cd5eb9a..34ba878 100644
--- a/builtin-fetch.c
+++ b/builtin-fetch.c

@@ -400,14 +400,14 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,
 
 /*
  * We would want to bypass the object transfer altogether if
- * everything we are going to fetch already exists and connected
+ * everything we are going to fetch already exists and is connected
  * locally.
  *
- * The refs we are going to fetch are in to_fetch (nr_heads in
- * total).  If running
+ * The refs we are going to fetch are in ref_map.  If running
  *
- *  $ git rev-list --objects to_fetch[0] to_fetch[1] ... --not --all
+ *  $ git rev-list --objects --stdin --not --all
  *
+ * (feeding all the refs in ref_map on its standard input)
  * does not error out, that means everything reachable from the
  * refs we are going to fetch exists and is connected to some of
  * our existing refs.

@@ -416,8 +416,9 @@ static int quickfetch(struct ref *ref_map)
 {
 	struct child_process revlist;
 	struct ref *ref;
-	char **argv;
-	int i, err;
+	int err;
+	const char *argv[] = {"rev-list",
+		"--quiet", "--objects", "--stdin", "--not", "--all", NULL};
 
 	/*
 	 * If we are deepening a shallow clone we already have these

@@ -429,34 +430,44 @@ static int quickfetch(struct ref *ref_map)
 	if (depth)
 		return -1;
 
-	for (i = 0, ref = ref_map; ref; ref = ref->next)
-		i++;
-	if (!i)
+	if (!ref_map)
 		return 0;
 
-	argv = xmalloc(sizeof(*argv) * (i + 6));
-	i = 0;
-	argv[i++] = xstrdup("rev-list");
-	argv[i++] = xstrdup("--quiet");
-	argv[i++] = xstrdup("--objects");
-	for (ref = ref_map; ref; ref = ref->next)
-		argv[i++] = xstrdup(sha1_to_hex(ref->old_sha1));
-	argv[i++] = xstrdup("--not");
-	argv[i++] = xstrdup("--all");
-	argv[i++] = NULL;
-
 	memset(&revlist, 0, sizeof(revlist));
-	revlist.argv = (const char**)argv;
+	revlist.argv = argv;
 	revlist.git_cmd = 1;
-	revlist.no_stdin = 1;
 	revlist.no_stdout = 1;
 	revlist.no_stderr = 1;
-	err = run_command(&revlist);
+	revlist.in = -1;
+
+	err = start_command(&revlist);
+	if (err) {
+		error("could not run rev-list");
+		return err;
+	}
+
+	/* If rev-list --stdin encounters an unknown commit, it terminates,
+	 * which will cause SIGPIPE in the write loop below. */
+	sigchain_push(SIGPIPE, SIG_IGN);
+
+	for (ref = ref_map; ref; ref = ref->next) {
+		if (write_in_full(revlist.in, sha1_to_hex(ref->old_sha1), 40) < 0 ||
+		    write_in_full(revlist.in, "\n", 1) < 0) {
+			if (err != EPIPE && err != EINVAL)
+				error("failed write to rev-list: %s", strerror(errno));
+			err = -1;
+			break;
+		}
+	}
+
+	if (close(revlist.in)) {
+		error("failed to close rev-list's stdin: %s", strerror(errno));
+		err = -1;
+	}
+
+	sigchain_pop(SIGPIPE);
 
-	for (i = 0; argv[i]; i++)
-		free(argv[i]);
-	free(argv);
-	return err;
+	return finish_command(&revlist) || err;
 }
 
 static int fetch_refs(struct transport *transport, struct ref *ref_map)

diff --git a/t/t5502-quickfetch.sh b/t/t5502-quickfetch.sh
index 16eadd6..1037a72 100755
--- a/t/t5502-quickfetch.sh
+++ b/t/t5502-quickfetch.sh

@@ -119,4 +119,24 @@ test_expect_success 'quickfetch should not copy from alternate' '
 
 '
 
+test_expect_success 'quickfetch should handle ~1000 refs (on Windows)' '
+
+	git gc &&
+	head=$(git rev-parse HEAD) &&
+	branchprefix="$head refs/heads/branch" &&
+	for i in 0 1 2 3 4 5 6 7 8 9; do
+		for j in 0 1 2 3 4 5 6 7 8 9; do
+			for k in 0 1 2 3 4 5 6 7 8 9; do
+				echo "$branchprefix$i$j$k" >> .git/packed-refs
+			done
+		done
+	done &&
+	(
+		cd cloned &&
+		git fetch &&
+		git fetch
+	)
+
+'
+
 test_done

-- 
1.6.3.rc0.1.gf800

Johan Herland schrieb:

On Thursday 09 July 2009, Johannes Sixt wrote:

quoted

But actually I meant you to make a test that triggers the SIGPIPE that
would kill git-fetch if it were not ignored. This one doesn't trigger it,
either.

AFAIU from earlier in this thread (and a mail from Peter linking to 
http://markmail.org/message/dbgdj4csafen65ye), SIGPIPE _never_ triggers on 
Windows, thus ignoring SIGPIPE is not needed for the fix per se. However, as 
a side-effect of the fix, we may now get SIGPIPE on Linux (and other POSIX 
platforms), so although it never triggers on Windows, it's still needed.

I know that, of course. But try this: Remove the signal(SIGPIPE, SIG_IGN)
and run the test suite. There is not a single failure. IOW, we don't have
a single test case that verifies that the signal(SIGPIPE, SIG_IGN) is
needed. I would like to have that test case, and you seem to know how to
construct it (otherwise there wouldn't be so much buzz about it).

-- Hannes

On Thursday 09 July 2009, Johannes Sixt wrote:

Johan Herland schrieb:

quoted

On Thursday 09 July 2009, Johannes Sixt wrote:

quoted

But actually I meant you to make a test that triggers the SIGPIPE that
would kill git-fetch if it were not ignored. This one doesn't trigger
it, either.

AFAIU from earlier in this thread (and a mail from Peter linking to
http://markmail.org/message/dbgdj4csafen65ye), SIGPIPE _never_ triggers
on Windows, thus ignoring SIGPIPE is not needed for the fix per se.
However, as a side-effect of the fix, we may now get SIGPIPE on Linux
(and other POSIX platforms), so although it never triggers on Windows,
it's still needed.

I know that, of course. But try this: Remove the signal(SIGPIPE, SIG_IGN)
and run the test suite. There is not a single failure.

That's not what I'm seeing. When I don't ignore the signal, the testsuite 
fails intermittently for me (on Linux). I see the following tests fail:

- t3409-rebase-preserve-merges.sh (subtest #2)
- t5503-tagfollow.sh (subtests #4, #6, #7)
- t5505-remote.sh (subtests #10, #12, #14 - #20, #27)
- t5510-fetch.sh (subtest #6 or #25)
- probably more (I seldom get this far...)

I assume the intermittent failures are caused by git rev-list sometimes 
terminate before git fetch is finished writing objects to its standard input 
(because of scheduling differences).

When i enable the signal handling, all selftests pass every time.


...Johan

-- 
Johan Herland, [off-list ref]
www.herland.net

Johan Herland schrieb:

On Thursday 09 July 2009, Johannes Sixt wrote:

quoted

But try this: Remove the signal(SIGPIPE, SIG_IGN)
and run the test suite. There is not a single failure.

That's not what I'm seeing. When I don't ignore the signal, the testsuite 
fails intermittently for me (on Linux). I see the following tests fail:

- t3409-rebase-preserve-merges.sh (subtest #2)
- t5503-tagfollow.sh (subtests #4, #6, #7)
- t5505-remote.sh (subtests #10, #12, #14 - #20, #27)
- t5510-fetch.sh (subtest #6 or #25)

I see. If I insert sched_yield() in the for loop, I see many failures (not
100% reproducible, but almost). On Windows, I have to insert Sleep(10) so
that the error exit is taken, and the error code is EINVAL :-/

-- Hannes

With this fixup the patch is good, I think.

-- Hannes

diff --git a/builtin-fetch.c b/builtin-fetch.c
index 34ba878..7547631 100644
--- a/builtin-fetch.c
+++ b/builtin-fetch.c

@@ -446,14 +446,16 @@ static int quickfetch(struct ref *ref_map)
 		return err;
 	}

-	/* If rev-list --stdin encounters an unknown commit, it terminates,
-	 * which will cause SIGPIPE in the write loop below. */
+	/*
+	 * If rev-list --stdin encounters an unknown commit, it terminates,
+	 * which would cause SIGPIPE in the write loop below.
+	 */
 	sigchain_push(SIGPIPE, SIG_IGN);

 	for (ref = ref_map; ref; ref = ref->next) {
 		if (write_in_full(revlist.in, sha1_to_hex(ref->old_sha1), 40) < 0 ||
 		    write_in_full(revlist.in, "\n", 1) < 0) {
-			if (err != EPIPE && err != EINVAL)
+			if (errno != EPIPE && errno != EINVAL)
 				error("failed write to rev-list: %s", strerror(errno));
 			err = -1;
 			break;

quickfetch() calls rev-list to check whether the objects we are about to
fetch are already present in the repo (if so, we can skip the object fetch).
However, when there are many (~1000) refs to be fetched, the rev-list
command line grows larger than the maximum command line size on some systems
(32K in Windows). This causes rev-list to fail, making quickfetch() return
non-zero, which unnecessarily triggers the transport machinery. This somehow
causes fetch to fail with an exit code.

By using the --stdin option to rev-list (and feeding the object list to its
standard input), we prevent the overflow of the rev-list command line,
which causes quickfetch(), and subsequently the overall fetch, to succeed.

However, using rev-list --stdin is not entirely straightforward: rev-list
terminates immediately when encountering an unknown object, which can
trigger SIGPIPE if we are still writing object's to its standard input.
We therefore temporarily ignore SIGPIPE so that the fetch process is not
terminated.

The patch also contains a testcase to verify the fix (note that before
the patch, the testcase would only fail on msysGit).

Signed-off-by: Johan Herland <redacted>
Improved-by: Johannes Sixt [off-list ref]
Improved-by: Alex Riesen [off-list ref]
Tested-by: Peter Krefting <redacted>
---

On Thursday 09 July 2009, Johannes Sixt wrote:

With this fixup the patch is good, I think.

Again, thanks for all your help!

Here's the final (*crossing fingers*) iteration of the patch.


Have fun! :)

...Johan


 builtin-fetch.c       |   67 +++++++++++++++++++++++++++++-------------------
 t/t5502-quickfetch.sh |   20 ++++++++++++++
 2 files changed, 60 insertions(+), 27 deletions(-)

diff --git a/builtin-fetch.c b/builtin-fetch.c
index cd5eb9a..817dd6b 100644
--- a/builtin-fetch.c
+++ b/builtin-fetch.c

@@ -400,14 +400,14 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,
 
 /*
  * We would want to bypass the object transfer altogether if
- * everything we are going to fetch already exists and connected
+ * everything we are going to fetch already exists and is connected
  * locally.
  *
- * The refs we are going to fetch are in to_fetch (nr_heads in
- * total).  If running
+ * The refs we are going to fetch are in ref_map.  If running
  *
- *  $ git rev-list --objects to_fetch[0] to_fetch[1] ... --not --all
+ *  $ git rev-list --objects --stdin --not --all
  *
+ * (feeding all the refs in ref_map on its standard input)
  * does not error out, that means everything reachable from the
  * refs we are going to fetch exists and is connected to some of
  * our existing refs.

@@ -416,8 +416,9 @@ static int quickfetch(struct ref *ref_map)
 {
 	struct child_process revlist;
 	struct ref *ref;
-	char **argv;
-	int i, err;
+	int err;
+	const char *argv[] = {"rev-list",
+		"--quiet", "--objects", "--stdin", "--not", "--all", NULL};
 
 	/*
 	 * If we are deepening a shallow clone we already have these

@@ -429,34 +430,46 @@ static int quickfetch(struct ref *ref_map)
 	if (depth)
 		return -1;
 
-	for (i = 0, ref = ref_map; ref; ref = ref->next)
-		i++;
-	if (!i)
+	if (!ref_map)
 		return 0;
 
-	argv = xmalloc(sizeof(*argv) * (i + 6));
-	i = 0;
-	argv[i++] = xstrdup("rev-list");
-	argv[i++] = xstrdup("--quiet");
-	argv[i++] = xstrdup("--objects");
-	for (ref = ref_map; ref; ref = ref->next)
-		argv[i++] = xstrdup(sha1_to_hex(ref->old_sha1));
-	argv[i++] = xstrdup("--not");
-	argv[i++] = xstrdup("--all");
-	argv[i++] = NULL;
-
 	memset(&revlist, 0, sizeof(revlist));
-	revlist.argv = (const char**)argv;
+	revlist.argv = argv;
 	revlist.git_cmd = 1;
-	revlist.no_stdin = 1;
 	revlist.no_stdout = 1;
 	revlist.no_stderr = 1;
-	err = run_command(&revlist);
+	revlist.in = -1;
+
+	err = start_command(&revlist);
+	if (err) {
+		error("could not run rev-list");
+		return err;
+	}
+
+	/*
+	 * If rev-list --stdin encounters an unknown commit, it terminates,
+	 * which will cause SIGPIPE in the write loop below.
+	 */
+	sigchain_push(SIGPIPE, SIG_IGN);
+
+	for (ref = ref_map; ref; ref = ref->next) {
+		if (write_in_full(revlist.in, sha1_to_hex(ref->old_sha1), 40) < 0 ||
+		    write_in_full(revlist.in, "\n", 1) < 0) {
+			if (errno != EPIPE && errno != EINVAL)
+				error("failed write to rev-list: %s", strerror(errno));
+			err = -1;
+			break;
+		}
+	}
+
+	if (close(revlist.in)) {
+		error("failed to close rev-list's stdin: %s", strerror(errno));
+		err = -1;
+	}
+
+	sigchain_pop(SIGPIPE);
 
-	for (i = 0; argv[i]; i++)
-		free(argv[i]);
-	free(argv);
-	return err;
+	return finish_command(&revlist) || err;
 }
 
 static int fetch_refs(struct transport *transport, struct ref *ref_map)

diff --git a/t/t5502-quickfetch.sh b/t/t5502-quickfetch.sh
index 16eadd6..1037a72 100755
--- a/t/t5502-quickfetch.sh
+++ b/t/t5502-quickfetch.sh

@@ -119,4 +119,24 @@ test_expect_success 'quickfetch should not copy from alternate' '
 
 '
 
+test_expect_success 'quickfetch should handle ~1000 refs (on Windows)' '
+
+	git gc &&
+	head=$(git rev-parse HEAD) &&
+	branchprefix="$head refs/heads/branch" &&
+	for i in 0 1 2 3 4 5 6 7 8 9; do
+		for j in 0 1 2 3 4 5 6 7 8 9; do
+			for k in 0 1 2 3 4 5 6 7 8 9; do
+				echo "$branchprefix$i$j$k" >> .git/packed-refs
+			done
+		done
+	done &&
+	(
+		cd cloned &&
+		git fetch &&
+		git fetch
+	)
+
+'
+
 test_done

-- 
1.6.3.rc0.1.gf800