# [PATCH 2/2] bundle: use a strbuf to scan the log for boundary commits 11 messages from 2012-02-22 to 2012-02-23. Participants: Thomas Rast, Junio C Hamano, Jeff King, Johannes Sixt. Thread: https://gitlist.dev/t/29696 ## Thomas Rast, 2012-02-22 19:34 Subject: [PATCH 1/2] bundle: put strbuf_readline_fd in strbuf.c with adjustments Message-ID: URL: https://gitlist.dev/e/a795f6dca5e7c3fc5f9212becda4a46116c502b7.1329939233.git.trast%40student.ethz.ch In-Reply-To: <4F451259.7010304@codethink.co.uk> ``` The comment even said that it should eventually go there. While at it, match the calling convention and name of the function to the strbuf_get*line family. So it now is strbuf_getwholeline_fd. Signed-off-by: Thomas Rast --- I was left looking for strbuf_readline_fd() for a while, then tried to look for the readline() from the libc that it would presumably match. Hence this cleanup. bundle.c | 21 ++------------------- strbuf.c | 16 ++++++++++++++++ strbuf.h | 1 + 3 files changed, 19 insertions(+), 19 deletions(-) diff --git a/bundle.c b/bundle.c index b8acf3c..313de42 100644 --- a/bundle.c +++ b/bundle.c @@ -23,23 +23,6 @@ static void add_to_ref_list(const unsigned char *sha1, const char *name, list->nr++; } -/* Eventually this should go to strbuf.[ch] */ -static int strbuf_readline_fd(struct strbuf *sb, int fd) -{ - strbuf_reset(sb); - - while (1) { - char ch; - ssize_t len = xread(fd, &ch, 1); - if (len <= 0) - return len; - strbuf_addch(sb, ch); - if (ch == '\n') - break; - } - return 0; -} - static int parse_bundle_header(int fd, struct bundle_header *header, const char *report_path) { @@ -47,7 +30,7 @@ static int parse_bundle_header(int fd, struct bundle_header *header, int status = 0; /* The bundle header begins with the signature */ - if (strbuf_readline_fd(&buf, fd) || + if (strbuf_getwholeline_fd(&buf, fd, '\n') || strcmp(buf.buf, bundle_signature)) { if (report_path) error("'%s' does not look like a v2 bundle file", @@ -57,7 +40,7 @@ static int parse_bundle_header(int fd, struct bundle_header *header, } /* The bundle header ends with an empty line */ - while (!strbuf_readline_fd(&buf, fd) && + while (!strbuf_getwholeline_fd(&buf, fd, '\n') && buf.len && buf.buf[0] != '\n') { unsigned char sha1[20]; int is_prereq = 0; diff --git a/strbuf.c b/strbuf.c index ff0b96b..5135d59 100644 --- a/strbuf.c +++ b/strbuf.c @@ -383,6 +383,22 @@ int strbuf_getline(struct strbuf *sb, FILE *fp, int term) return 0; } +int strbuf_getwholeline_fd(struct strbuf *sb, int fd, int term) +{ + strbuf_reset(sb); + + while (1) { + char ch; + ssize_t len = xread(fd, &ch, 1); + if (len <= 0) + return EOF; + strbuf_addch(sb, ch); + if (ch == term) + break; + } + return 0; +} + int strbuf_read_file(struct strbuf *sb, const char *path, size_t hint) { int fd, len; diff --git a/strbuf.h b/strbuf.h index fbf059f..3effaa8 100644 --- a/strbuf.h +++ b/strbuf.h @@ -116,6 +116,7 @@ static inline void strbuf_complete_line(struct strbuf *sb) extern int strbuf_getwholeline(struct strbuf *, FILE *, int); extern int strbuf_getline(struct strbuf *, FILE *, int); +extern int strbuf_getwholeline_fd(struct strbuf *, int, int); extern void stripspace(struct strbuf *buf, int skip_comments); extern int launch_editor(const char *path, struct strbuf *buffer, const char *const *env); -- 1.7.9.1.430.g4998543 ``` ## Thomas Rast, 2012-02-22 19:34 Subject: [PATCH 2/2] bundle: use a strbuf to scan the log for boundary commits Message-ID: URL: https://gitlist.dev/e/fa1553d59714fd89fdab1bf54af19ac631a30a8c.1329939233.git.trast%40student.ethz.ch In-Reply-To: ``` 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 | 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. Reported-by: Jannis Pohlmann Signed-off-by: Thomas Rast --- bundle.c | 14 +++++++------- t/t5704-bundle.sh | 17 +++++++++++++++++ 2 files changed, 24 insertions(+), 7 deletions(-) diff --git a/bundle.c b/bundle.c index 313de42..0dbd174 100644 --- a/bundle.c +++ b/bundle.c @@ -234,7 +234,7 @@ int create_bundle(struct bundle_header *header, const char *path, const char **argv_boundary = xmalloc((argc + 4) * sizeof(const char *)); const char **argv_pack = xmalloc(6 * sizeof(const char *)); int i, ref_count = 0; - char buffer[1024]; + struct strbuf buf = STRBUF_INIT; struct rev_info revs; struct child_process rls; FILE *rls_fout; @@ -266,16 +266,16 @@ int create_bundle(struct bundle_header *header, const char *path, if (start_command(&rls)) return -1; rls_fout = xfdopen(rls.out, "r"); - while (fgets(buffer, sizeof(buffer), rls_fout)) { + while (strbuf_getwholeline(&buf, rls_fout, '\n') != EOF) { unsigned char sha1[20]; - if (buffer[0] == '-') { - write_or_die(bundle_fd, buffer, strlen(buffer)); - if (!get_sha1_hex(buffer + 1, sha1)) { + if (buf.len > 0 && buf.buf[0] == '-') { + write_or_die(bundle_fd, buf.buf, buf.len); + if (!get_sha1_hex(buf.buf + 1, sha1)) { struct object *object = parse_object(sha1); object->flags |= UNINTERESTING; - add_pending_object(&revs, object, buffer); + add_pending_object(&revs, object, buf.buf); } - } else if (!get_sha1_hex(buffer, sha1)) { + } else if (!get_sha1_hex(buf.buf, sha1)) { struct object *object = parse_object(sha1); object->flags |= SHOWN; } diff --git a/t/t5704-bundle.sh b/t/t5704-bundle.sh index 4ae127d..7c2f307 100755 --- a/t/t5704-bundle.sh +++ b/t/t5704-bundle.sh @@ -59,4 +59,21 @@ test_expect_success 'empty bundle file is rejected' ' ' +# If "ridiculous" is at least 1004 chars, this traps a bug in old +# versions where the resulting 1025-char line (with --pretty=oneline) +# was longer than a 1024-char buffer +test_expect_success 'ridiculously long subject in boundary' ' + + : > file4 && + test_tick && + git add file4 && + printf "abcdefghijkl %s\n" $(seq 1 100) | git commit -F - && + test_commit fifth && + git bundle create long-subject-bundle.bdl HEAD^..HEAD && + git fetch long-subject-bundle.bdl && + sed -n "/^-/{p;q}" long-subject-bundle.bdl > boundary && + grep "^-$_x40 " boundary + +' + test_done -- 1.7.9.1.430.g4998543 ``` ## Junio C Hamano, 2012-02-22 20:22 Subject: Re: [PATCH 2/2] bundle: use a strbuf to scan the log for boundary commits Message-ID: <7vlinuaaab.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7vlinuaaab.fsf%40alter.siamese.dyndns.org In-Reply-To: ``` Thomas Rast writes: > 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 | 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; ``` ## Thomas Rast, 2012-02-22 20:25 Subject: Re: [PATCH 2/2] bundle: use a strbuf to scan the log for boundary commits Message-ID: <87hayivcmm.fsf@thomas.inf.ethz.ch> URL: https://gitlist.dev/e/87hayivcmm.fsf%40thomas.inf.ethz.ch In-Reply-To: <7vlinuaaab.fsf@alter.siamese.dyndns.org> ``` Junio C Hamano writes: > Thomas Rast writes: > >> # v2 git bundle >> $(git rev-list --pretty=oneline --boundary | 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. > > 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: Not sure. The only use I could think of would be to google for the subjects, in the hope of finding some repository that has the commit you are looking for. Other than that... In any case the --pretty=oneline is very deliberate, as we can see from the commit below. It just doesn't give a reason :-) commit 239296770dae75e21c179733785731ec6ffae1f5 Author: Johannes Schindelin Date: Fri Feb 23 03:17:51 2007 +0100 git-bundle: record commit summary in the prerequisite data diff --git a/builtin-bundle.c b/builtin-bundle.c index 191ec55..d74afaa 100644 --- a/builtin-bundle.c +++ b/builtin-bundle.c @@ -267,7 +267,7 @@ static int create_bundle(struct bundle_header *header, const char *path, int argc, const char **argv) { int bundle_fd = -1; - const char **argv_boundary = xmalloc((argc + 3) * sizeof(const char *)); + const char **argv_boundary = xmalloc((argc + 4) * sizeof(const char *)); const char **argv_pack = xmalloc(4 * sizeof(const char *)); int pid, in, out, i, status; char buffer[1024]; @@ -282,10 +282,11 @@ static int create_bundle(struct bundle_header *header, const char *path, write_or_die(bundle_fd, bundle_signature, strlen(bundle_signature)); /* write prerequisites */ - memcpy(argv_boundary + 2, argv + 1, argc * sizeof(const char *)); + memcpy(argv_boundary + 3, argv + 1, argc * sizeof(const char *)); argv_boundary[0] = "rev-list"; argv_boundary[1] = "--boundary"; - argv_boundary[argc + 1] = NULL; + argv_boundary[2] = "--pretty=oneline"; + argv_boundary[argc + 2] = NULL; out = -1; pid = fork_with_pipe(argv_boundary, NULL, &out); if (pid < 0) -- Thomas Rast trast@{inf,student}.ethz.ch ``` ## Junio C Hamano, 2012-02-22 20:50 Subject: Re: [PATCH 2/2] bundle: use a strbuf to scan the log for boundary commits Message-ID: <7vhayia8yc.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7vhayia8yc.fsf%40alter.siamese.dyndns.org In-Reply-To: <87hayivcmm.fsf@thomas.inf.ethz.ch> ``` Thomas Rast writes: > In any case the --pretty=oneline is very deliberate, as we can see from > the commit below. It just doesn't give a reason :-) Thanks for digging and I hope everybody on this list who would ever send a patch learns the importance of describing _why_ in the log from this lesson. I'll amend your 2/2 with a note that keeping the subject material is not justified and we may want to remove (or at least reduce) it in the future. ``` ## Jeff King, 2012-02-22 20:51 Subject: Re: [PATCH 1/2] bundle: put strbuf_readline_fd in strbuf.c with adjustments Message-ID: <20120222205108.GC6781@sigill.intra.peff.net> URL: https://gitlist.dev/e/20120222205108.GC6781%40sigill.intra.peff.net In-Reply-To: ``` On Wed, Feb 22, 2012 at 08:34:22PM +0100, Thomas Rast wrote: > The comment even said that it should eventually go there. While at > it, match the calling convention and name of the function to the > strbuf_get*line family. So it now is strbuf_getwholeline_fd. > [...] > bundle.c | 21 ++------------------- > strbuf.c | 16 ++++++++++++++++ > strbuf.h | 1 + Nit: no update to Documentation/technical/api-strbuf.txt. You might want to also mention that this will read() one byte at a time, and is therefore only a good idea if you really care about the position of the fd. Otherwise, fdopen() + strbuf_getwholeline() is much more efficient. -Peff ``` ## Jeff King, 2012-02-22 20:55 Subject: Re: [PATCH 2/2] bundle: use a strbuf to scan the log for boundary commits Message-ID: <20120222205500.GD6781@sigill.intra.peff.net> URL: https://gitlist.dev/e/20120222205500.GD6781%40sigill.intra.peff.net In-Reply-To: ``` On Wed, Feb 22, 2012 at 08:34:23PM +0100, Thomas Rast wrote: > +# If "ridiculous" is at least 1004 chars, this traps a bug in old > +# versions where the resulting 1025-char line (with --pretty=oneline) > +# was longer than a 1024-char buffer > +test_expect_success 'ridiculously long subject in boundary' ' > + > + : > file4 && > + test_tick && > + git add file4 && > + printf "abcdefghijkl %s\n" $(seq 1 100) | git commit -F - && Seq is not portable. I usually use either perl -le "print for (1..100)" or just do: z16=zzzzzzzzzzzzzzzz z256=$z16$z16$z16$z16$z16$z16$z16$z16 z1024=$z256$z256$z256$z256$z256$z256$z256$z256 -Peff ``` ## Jeff King, 2012-02-22 21:00 Subject: Re: [PATCH 2/2] bundle: use a strbuf to scan the log for boundary commits Message-ID: <20120222210000.GE6781@sigill.intra.peff.net> URL: https://gitlist.dev/e/20120222210000.GE6781%40sigill.intra.peff.net In-Reply-To: <87hayivcmm.fsf@thomas.inf.ethz.ch> ``` On Wed, Feb 22, 2012 at 09:25:53PM +0100, Thomas Rast wrote: > > 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: > > Not sure. The only use I could think of would be to google for the > subjects, in the hope of finding some repository that has the commit you > are looking for. Other than that... Or because the bundle creator (which may even be you on a different day) did not correctly guess at the negative refs when specifying a cutoff. Assuming you no longer have access to the original repo (not unreasonable, since you are using a bundle), the messages could help you understand where the error occurred. Of course, once you figure out "aha! I expected the bundle to include foo, but it is actually a cutoff point. I should have said XYZ^ as the cutoff", I'm not sure what you do then. It's not like there is an easy way to salvage the data that is in the bundle. So I think it is a debugging aid, but one that ultimately doesn't help you that much. -Peff ``` ## Johannes Sixt, 2012-02-22 22:10 Subject: Re: [PATCH 2/2] bundle: use a strbuf to scan the log for boundary commits Message-ID: <4F4567C8.2030809@kdbg.org> URL: https://gitlist.dev/e/4F4567C8.2030809%40kdbg.org In-Reply-To: <20120222205500.GD6781@sigill.intra.peff.net> ``` Am 22.02.2012 21:55, schrieb Jeff King: > On Wed, Feb 22, 2012 at 08:34:23PM +0100, Thomas Rast wrote: >> + printf "abcdefghijkl %s\n" $(seq 1 100) | git commit -F - && > > Seq is not portable. Thanks for pointing this out. > I usually use either > > perl -le "print for (1..100)" > > or just do: > > z16=zzzzzzzzzzzzzzzz > z256=$z16$z16$z16$z16$z16$z16$z16$z16 > z1024=$z256$z256$z256$z256$z256$z256$z256$z256 If a sequence of ASCII zeros is good enough, you can also do: printf %02134d 0 -- Hannes ``` ## Junio C Hamano, 2012-02-23 03:20 Subject: Re: [PATCH 2/2] bundle: use a strbuf to scan the log for boundary commits Message-ID: <7vy5ru5j7q.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7vy5ru5j7q.fsf%40alter.siamese.dyndns.org In-Reply-To: ``` Thomas Rast writes: > +# If "ridiculous" is at least 1004 chars, this traps a bug in old > +# versions where the resulting 1025-char line (with --pretty=oneline) > +# was longer than a 1024-char buffer > +test_expect_success 'ridiculously long subject in boundary' ' > + > + : > file4 && > + test_tick && > + git add file4 && > + printf "abcdefghijkl %s\n" $(seq 1 100) | git commit -F - && > + test_commit fifth && > + git bundle create long-subject-bundle.bdl HEAD^..HEAD && > + git fetch long-subject-bundle.bdl && > + sed -n "/^-/{p;q}" long-subject-bundle.bdl > boundary && > + grep "^-$_x40 " boundary > + > +' Hmm, aside from the use of seq J6t mentioned, I am not sure what this is testing. I thought the point is to make "--oneline" output produce a long line whose 1024th character is '-' and make the fgets() based parser to mistake it as the beginning of a line that records a boundary commit, but you do not seem to have any '-' there. ``` ## Junio C Hamano, 2012-02-23 03:38 Subject: Re: [PATCH 2/2] bundle: use a strbuf to scan the log for boundary commits Message-ID: <7vty2i5ie2.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7vty2i5ie2.fsf%40alter.siamese.dyndns.org In-Reply-To: ``` Thomas Rast writes: > diff --git a/bundle.c b/bundle.c > index 313de42..0dbd174 100644 > --- a/bundle.c > +++ b/bundle.c > @@ -234,7 +234,7 @@ int create_bundle(struct bundle_header *header, const char *path, > const char **argv_boundary = xmalloc((argc + 4) * sizeof(const char *)); > const char **argv_pack = xmalloc(6 * sizeof(const char *)); > int i, ref_count = 0; > - char buffer[1024]; > + struct strbuf buf = STRBUF_INIT; > struct rev_info revs; > struct child_process rls; > FILE *rls_fout; > @@ -266,16 +266,16 @@ int create_bundle(struct bundle_header *header, const char *path, > if (start_command(&rls)) > return -1; > rls_fout = xfdopen(rls.out, "r"); > - while (fgets(buffer, sizeof(buffer), rls_fout)) { > + while (strbuf_getwholeline(&buf, rls_fout, '\n') != EOF) { I'd add strbuf_release(&buf) after this loop. Perhaps we would want to squash something like this to the test to avoid "seq", using J6t's idea. The issue is that we do not write the end of line for the long boundary (because it is hidden from us), keep reading and rejecting bogus letters, and then the tip is written without terminating the boundary line. So checking boundary line is not a very useful test, but "fetch" and "list-heads" are. t/t5704-bundle.sh | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/t/t5704-bundle.sh b/t/t5704-bundle.sh index 7c2f307..5319b84 100755 --- a/t/t5704-bundle.sh +++ b/t/t5704-bundle.sh @@ -59,19 +59,20 @@ test_expect_success 'empty bundle file is rejected' ' ' -# If "ridiculous" is at least 1004 chars, this traps a bug in old -# versions where the resulting 1025-char line (with --pretty=oneline) -# was longer than a 1024-char buffer +# This triggers a bug in older versions where the resulting line (with +# --pretty=oneline) was longer than a 1024-char buffer. test_expect_success 'ridiculously long subject in boundary' ' - : > file4 && + : >file4 && test_tick && git add file4 && - printf "abcdefghijkl %s\n" $(seq 1 100) | git commit -F - && + printf "%0982d\n" 0 | git commit -F - && test_commit fifth && git bundle create long-subject-bundle.bdl HEAD^..HEAD && + git bundle list-heads long-subject-bundle.bdl >heads && + test -s heads && git fetch long-subject-bundle.bdl && - sed -n "/^-/{p;q}" long-subject-bundle.bdl > boundary && + sed -n "/^-/{p;q}" long-subject-bundle.bdl >boundary && grep "^-$_x40 " boundary ' -- 1.7.9.2.258.g4407a ```