@matheusbsbs / matheus-bagliano / commits / d9eb0205a2
Fork 0

quickfetch(): Prevent overflow of the rev-list command line

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 <johan@herland.net> Improved-by: Johannes Sixt <j6t@kdbg.org> Improved-by: Alex Riesen <raa.lkml@gmail.com> Tested-by: Peter Krefting <peter@softwolves.pp.se> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Johan Herland committed Jul 10, 2009 at 01:52 UTC d9eb0205a217984f3e70bf18ae66c02a22d3d475
2 files changed +60 -27
builtin-fetch.c
+40 -27
@@ -400,14 +400,14 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,
400
401 /*
402 * We would want to bypass the object transfer altogether if
403 - * everything we are going to fetch already exists and connected
403 + * everything we are going to fetch already exists and is connected
404 * locally.
405 *
406 - * The refs we are going to fetch are in to_fetch (nr_heads in
407 - * total). If running
406 + * The refs we are going to fetch are in ref_map. If running
407 *
409 - * $ git rev-list --objects to_fetch[0] to_fetch[1] ... --not --all
408 + * $ git rev-list --objects --stdin --not --all
409 *
410 + * (feeding all the refs in ref_map on its standard input)
411 * does not error out, that means everything reachable from the
412 * refs we are going to fetch exists and is connected to some of
413 * our existing refs.
@@ -416,8 +416,9 @@ static int quickfetch(struct ref *ref_map)
416 {
417 struct child_process revlist;
418 struct ref *ref;
419 - char **argv;
420 - int i, err;
419 + int err;
420 + const char *argv[] = {"rev-list",
421 + "--quiet", "--objects", "--stdin", "--not", "--all", NULL};
422
423 /*
424 * If we are deepening a shallow clone we already have these
@@ -429,34 +430,46 @@ static int quickfetch(struct ref *ref_map)
430 if (depth)
431 return -1;
432
432 - for (i = 0, ref = ref_map; ref; ref = ref->next)
433 - i++;
434 - if (!i)
433 + if (!ref_map)
434 return 0;
435
437 - argv = xmalloc(sizeof(*argv) * (i + 6));
438 - i = 0;
439 - argv[i++] = xstrdup("rev-list");
440 - argv[i++] = xstrdup("--quiet");
441 - argv[i++] = xstrdup("--objects");
442 - for (ref = ref_map; ref; ref = ref->next)
443 - argv[i++] = xstrdup(sha1_to_hex(ref->old_sha1));
444 - argv[i++] = xstrdup("--not");
445 - argv[i++] = xstrdup("--all");
446 - argv[i++] = NULL;
447 -
436 memset(&revlist, 0, sizeof(revlist));
449 - revlist.argv = (const char**)argv;
437 + revlist.argv = argv;
438 revlist.git_cmd = 1;
451 - revlist.no_stdin = 1;
439 revlist.no_stdout = 1;
440 revlist.no_stderr = 1;
454 - err = run_command(&revlist);
441 + revlist.in = -1;
442 +
443 + err = start_command(&revlist);
444 + if (err) {
445 + error("could not run rev-list");
446 + return err;
447 + }
448 +
449 + /*
450 + * If rev-list --stdin encounters an unknown commit, it terminates,
451 + * which will cause SIGPIPE in the write loop below.
452 + */
453 + sigchain_push(SIGPIPE, SIG_IGN);
454 +
455 + for (ref = ref_map; ref; ref = ref->next) {
456 + if (write_in_full(revlist.in, sha1_to_hex(ref->old_sha1), 40) < 0 ||
457 + write_in_full(revlist.in, "\n", 1) < 0) {
458 + if (errno != EPIPE && errno != EINVAL)
459 + error("failed write to rev-list: %s", strerror(errno));
460 + err = -1;
461 + break;
462 + }
463 + }
464 +
465 + if (close(revlist.in)) {
466 + error("failed to close rev-list's stdin: %s", strerror(errno));
467 + err = -1;
468 + }
469 +
470 + sigchain_pop(SIGPIPE);
471
456 - for (i = 0; argv[i]; i++)
457 - free(argv[i]);
458 - free(argv);
459 - return err;
472 + return finish_command(&revlist) || err;
473 }
474
475 static int fetch_refs(struct transport *transport, struct ref *ref_map)
t/t5502-quickfetch.sh
+20
@@ -119,4 +119,24 @@ test_expect_success 'quickfetch should not copy from alternate' '
119
120 '
121
122 +test_expect_success 'quickfetch should handle ~1000 refs (on Windows)' '
123 +
124 + git gc &&
125 + head=$(git rev-parse HEAD) &&
126 + branchprefix="$head refs/heads/branch" &&
127 + for i in 0 1 2 3 4 5 6 7 8 9; do
128 + for j in 0 1 2 3 4 5 6 7 8 9; do
129 + for k in 0 1 2 3 4 5 6 7 8 9; do
130 + echo "$branchprefix$i$j$k" >> .git/packed-refs
131 + done
132 + done
133 + done &&
134 + (
135 + cd cloned &&
136 + git fetch &&
137 + git fetch
138 + )
139 +
140 +'
141 +
142 test_done