# [PATCH] blame.c: Properly initialize strbuf after calling textconv_object(), again 6 messages from 2011-11-07 to 2011-11-07. Participants: Sebastian Schuberth, Junio C Hamano, Jeff King. Thread: https://gitlist.dev/t/28876 ## Sebastian Schuberth, 2011-11-07 15:53 Subject: [PATCH] blame.c: Properly initialize strbuf after calling textconv_object(), again Message-ID: <4EB7FEE6.9000609@gmail.com> URL: https://gitlist.dev/e/4EB7FEE6.9000609%40gmail.com ``` 2564aa4 started to initialize buf.alloc, but that should actually be one more byte than the string length due to the trailing \0. --- builtin/blame.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/builtin/blame.c b/builtin/blame.c index 86c0537..45f0dcc 100644 --- a/builtin/blame.c +++ b/builtin/blame.c @@ -2114,7 +2114,7 @@ static struct commit *fake_working_tree_commit(struct diff_options *opt, case S_IFREG: if (DIFF_OPT_TST(opt, ALLOW_TEXTCONV) && textconv_object(read_from, mode, null_sha1, &buf.buf, &buf_len)) { - buf.alloc = buf_len; + buf.alloc = buf_len + 1; buf.len = buf_len; } else if (strbuf_read_file(&buf, read_from, st.st_size) != st.st_size) -- 1.7.8.rc0.46.g5ae0f.dirty ``` ## Junio C Hamano, 2011-11-07 16:47 Subject: Re: [PATCH] blame.c: Properly initialize strbuf after calling textconv_object(), again Message-ID: <7vy5vr282s.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7vy5vr282s.fsf%40alter.siamese.dyndns.org In-Reply-To: <4EB7FEE6.9000609@gmail.com> ``` Sebastian Schuberth writes: > 2564aa4 started to initialize buf.alloc, but that should actually be one > more byte than the string length due to the trailing \0. Even when the conversion result is a zero-length string? > --- Sign-off? > builtin/blame.c | 2 +- > 1 files changed, 1 insertions(+), 1 deletions(-) > > diff --git a/builtin/blame.c b/builtin/blame.c > index 86c0537..45f0dcc 100644 > --- a/builtin/blame.c > +++ b/builtin/blame.c > @@ -2114,7 +2114,7 @@ static struct commit *fake_working_tree_commit(struct diff_options *opt, > case S_IFREG: > if (DIFF_OPT_TST(opt, ALLOW_TEXTCONV) && > textconv_object(read_from, mode, null_sha1, &buf.buf, &buf_len)) { > - buf.alloc = buf_len; > + buf.alloc = buf_len + 1; > buf.len = buf_len; > } > else if (strbuf_read_file(&buf, read_from, st.st_size) != st.st_size) ``` ## Jeff King, 2011-11-07 16:49 Subject: Re: [PATCH] blame.c: Properly initialize strbuf after calling textconv_object(), again Message-ID: <20111107164946.GD27055@sigill.intra.peff.net> URL: https://gitlist.dev/e/20111107164946.GD27055%40sigill.intra.peff.net In-Reply-To: <4EB7FEE6.9000609@gmail.com> ``` On Mon, Nov 07, 2011 at 04:53:10PM +0100, Sebastian Schuberth wrote: > --- a/builtin/blame.c > +++ b/builtin/blame.c > @@ -2114,7 +2114,7 @@ static struct commit *fake_working_tree_commit(struct diff_options *opt, > case S_IFREG: > if (DIFF_OPT_TST(opt, ALLOW_TEXTCONV) && > textconv_object(read_from, mode, null_sha1, &buf.buf, &buf_len)) { > - buf.alloc = buf_len; > + buf.alloc = buf_len + 1; > buf.len = buf_len; > } > else if (strbuf_read_file(&buf, read_from, st.st_size) != st.st_size) Your patch is correct in the sense that this fixes a bug, but it just seems wrong to be touching the alloc parameter outside of the strbuf code. It looks like the intent is to do the equivalent of: strbuf_add(&buf, some_heap_buf, len); free(some_heap_buf); but avoid the extra allocation and copy. Should there be a "strbuf_attach" function to encapsulate this pattern? -Peff ``` ## Sebastian Schuberth, 2011-11-07 17:31 Subject: Re: [PATCH] blame.c: Properly initialize strbuf after calling textconv_object(), again Message-ID: <4EB815EB.30702@gmail.com> URL: https://gitlist.dev/e/4EB815EB.30702%40gmail.com In-Reply-To: <7vy5vr282s.fsf@alter.siamese.dyndns.org> ``` On 07.11.2011 17:47, Junio C Hamano wrote: >> 2564aa4 started to initialize buf.alloc, but that should actually be one >> more byte than the string length due to the trailing \0. > > Even when the conversion result is a zero-length string? In this case, yes. The string buffer is initialized (and detached) in run_textconv, which calls strbuf_read, which grows the (yet empty) string to 8192 bytes. So alloc is always > 0, and the detach will trim alloc to len + 1. However, Peff made another valid point and I'll send v2 soon. -- Sebastian Schuberth ``` ## Sebastian Schuberth, 2011-11-07 17:33 Subject: [PATCHv2] blame.c: Properly initialize strbuf after calling textconv_object(), again Message-ID: <4EB8166E.9000703@gmail.com> URL: https://gitlist.dev/e/4EB8166E.9000703%40gmail.com In-Reply-To: <20111107164946.GD27055@sigill.intra.peff.net> ``` 2564aa4 started to initialize buf.alloc, but that should actually be one more byte than the string length due to the trailing \0. Also, do not modify buf.alloc out of the strbuf code. Use the existing strbuf_attach instead. Signed-off-by: Sebastian Schuberth --- builtin/blame.c | 7 +++---- 1 files changed, 3 insertions(+), 4 deletions(-) diff --git a/builtin/blame.c b/builtin/blame.c index 86c0537..80febbe 100644 --- a/builtin/blame.c +++ b/builtin/blame.c @@ -2096,6 +2096,7 @@ static struct commit *fake_working_tree_commit(struct diff_options *opt, if (!contents_from || strcmp("-", contents_from)) { struct stat st; const char *read_from; + char *buf_ptr; unsigned long buf_len; if (contents_from) { @@ -2113,10 +2114,8 @@ static struct commit *fake_working_tree_commit(struct diff_options *opt, switch (st.st_mode & S_IFMT) { case S_IFREG: if (DIFF_OPT_TST(opt, ALLOW_TEXTCONV) && - textconv_object(read_from, mode, null_sha1, &buf.buf, &buf_len)) { - buf.alloc = buf_len; - buf.len = buf_len; - } + textconv_object(read_from, mode, null_sha1, &buf_ptr, &buf_len)) + strbuf_attach(&buf, buf_ptr, buf_len, buf_len + 1); else if (strbuf_read_file(&buf, read_from, st.st_size) != st.st_size) die_errno("cannot open or read '%s'", read_from); break; -- 1.7.8.rc0.47.gf2c75.dirty ``` ## Jeff King, 2011-11-07 17:41 Subject: Re: [PATCHv2] blame.c: Properly initialize strbuf after calling textconv_object(), again Message-ID: <20111107174136.GA4047@sigill.intra.peff.net> URL: https://gitlist.dev/e/20111107174136.GA4047%40sigill.intra.peff.net In-Reply-To: <4EB8166E.9000703@gmail.com> ``` On Mon, Nov 07, 2011 at 06:33:34PM +0100, Sebastian Schuberth wrote: > 2564aa4 started to initialize buf.alloc, but that should actually be one > more byte than the string length due to the trailing \0. Also, do not > modify buf.alloc out of the strbuf code. Use the existing strbuf_attach > instead. Heh. When I wrote the previous email I just made up the name "strbuf_attach" as the opposite of "strbuf_detach". I didn't know it actually already existed. > - textconv_object(read_from, mode, null_sha1, &buf.buf, &buf_len)) { > - buf.alloc = buf_len; > - buf.len = buf_len; > - } > + textconv_object(read_from, mode, null_sha1, &buf_ptr, &buf_len)) > + strbuf_attach(&buf, buf_ptr, buf_len, buf_len + 1); Looks OK to me. -Peff ```