Re: [PATCH 2/2] bundle: use a strbuf to scan the log for boundary commits
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Feb 22, 2012, 20:22 UTC
- Message-ID
- <7vlinuaaab.fsf@alter.siamese.dyndns.org>
- In-Reply-To
- <fa1553d59714fd89fdab1bf54af19ac631a30a8c.1329939233.git.trast@student.ethz.ch>
Thomas Rast <trast@student.ethz.ch> writes:
Show 22 quoted lines
> The first part of the bundle header contains the boundary commits, and > could be approximated by > > # v2 git bundle > $(git rev-list --pretty=oneline --boundary <ARGS> | grep ^-) > > git-bundle actually spawns exactly this rev-list invocation, and does > the grepping internally. > > There was a subtle bug in the latter step: it used fgets() with a > 1024-byte buffer. If the user has sufficiently long subjects (e.g., > by not adhering to the git oneline-subject convention in the first > place), the 'oneline' format can easily overflow the buffer. fgets() > then returns the rest of the line in the next call(s). If one of > these remaining parts started with '-', git-bundle would mistakenly > insert it into the bundle thinking it was a boundary commit. > > Fix it by using strbuf_getwholeline() instead, which handles arbitrary > line lengths correctly. > > Note that on the receiving side in parse_bundle_header() we were > already using strbuf_getwholeline_fd(), so that part is safe.
Thanks for diagnosing this, but I wonder if it even needs --pretty=oneline to begin with, except for debugging purposes.
Do we ever use the subject string read from the rev-list output in any way?
In other words, I am wondering if the right patch to minimally fix the issue starting from older releases is something along this line instead:
diff --git a/bundle.c b/bundle.c index b8acf3c..339dbb0 100644 --- a/bundle.c +++ b/bundle.c @@ -248,7 +248,7 @@ int create_bundle(struct bundle_header *header, const char *path, static struct lock_file lock; int bundle_fd = -1; int bundle_to_stdout; - const char **argv_boundary = xmalloc((argc + 4) * sizeof(const char *)); + const char **argv_boundary = xmalloc((argc + 3) * sizeof(const char *)); const char **argv_pack = xmalloc(6 * sizeof(const char *)); int i, ref_count = 0; char buffer[1024]; @@ -271,11 +271,10 @@ int create_bundle(struct bundle_header *header, const char *path, init_revisions(&revs, NULL); /* write prerequisites */ - memcpy(argv_boundary + 3, argv + 1, argc * sizeof(const char *)); + memcpy(argv_boundary + 2, argv + 1, argc * sizeof(const char *)); argv_boundary[0] = "rev-list"; argv_boundary[1] = "--boundary"; - argv_boundary[2] = "--pretty=oneline"; - argv_boundary[argc + 2] = NULL; + argv_boundary[argc + 1] = NULL; memset(&rls, 0, sizeof(rls)); rls.argv = argv_boundary; rls.out = -1;