From: Eric Sunshine <sunshine@sunshineco.com>
To: Jeff King <peff@peff.net>
Cc: "Stefan Frühwirth" <stefan.fruehwirth@uni-graz.at>, git@vger.kernel.org
Subject: Re: [PATCH] merge_blobs: use strbuf instead of manually-sized mmfile_t
Date: Tue, 16 Feb 2016 00:09:15 -0500 [thread overview]
Message-ID: <20160216050915.GA5765@flurp.local> (raw)
In-Reply-To: <20160216011258.GA11961@sigill.intra.peff.net>
On Mon, Feb 15, 2016 at 08:12:58PM -0500, Jeff King wrote:
> On Mon, Feb 15, 2016 at 10:39:39PM +0100, Stefan Frühwirth wrote:
> > in one specific circumstance, git-merge-tree exits with a segfault caused by
> > "*** Error in `git': malloc(): memory corruption (fast)":
> >
> > There is a test case[1] kindly provided by chrisrossi, which he crafted
> > after I discovered the problem[2] in the context of Pylons/acidfs.
>
> -- >8 --
> Subject: merge_blobs: use strbuf instead of manually-sized mmfile_t
>
> [...]
> It does so by calling xdiff with XDIFF_EMIT_COMMON, and
> stores the result in a buffer that is as big as the smaller
> of "ours" and "theirs".
>
> In theory, this is right; we cannot have more common content
> than is in the smaller of the two blobs. But in practice,
> xdiff may give us more: if neither file ends in a newline,
> we get the "\nNo newline at end of file" marker.
> [...]
>
> Reported-by: Stefan Frühwirth <stefan.fruehwirth@uni-graz.at>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> diff --git a/merge-blobs.c b/merge-blobs.c
> @@ -51,19 +51,16 @@ static void *three_way_filemerge(const char *path, mmfile_t *base, mmfile_t *our
> static int common_outf(void *priv_, mmbuffer_t *mb, int nbuf)
> {
> int i;
> - mmfile_t *dst = priv_;
> + struct strbuf *dst = priv_;
>
> - for (i = 0; i < nbuf; i++) {
> - memcpy(dst->ptr + dst->size, mb[i].ptr, mb[i].size);
> - dst->size += mb[i].size;
> - }
> + for (i = 0; i < nbuf; i++)
> + strbuf_add(dst, mb[i].ptr, mb[i].size);
> return 0;
> }
>
> static int generate_common_file(mmfile_t *res, mmfile_t *f1, mmfile_t *f2)
> {
> - unsigned long size = f1->size < f2->size ? f1->size : f2->size;
> - void *ptr = xmalloc(size);
> + struct strbuf out = STRBUF_INIT;
> xpparam_t xpp;
> xdemitconf_t xecfg;
> xdemitcb_t ecb;
> @@ -75,11 +72,15 @@ static int generate_common_file(mmfile_t *res, mmfile_t *f1, mmfile_t *f2)
> xecfg.flags = XDL_EMIT_COMMON;
> ecb.outf = common_outf;
>
> - res->ptr = ptr;
> - res->size = 0;
> + ecb.priv = &out;
> + if (xdi_diff(f1, f2, &xpp, &xecfg, &ecb) < 0) {
> + strbuf_release(&out);
> + return -1;
> + }
>
> - ecb.priv = res;
> - return xdi_diff(f1, f2, &xpp, &xecfg, &ecb);
> + res->size = out.len; /* avoid long/size_t pointer mismatch below */
It took a minute or two for me to realize that "mismatch below" was
talking about the second argument to strbuf_detach(). I tried
rewriting the comment to mention the second argument explicitly, but
couldn't come up with one sufficiently succinct. Oh well.
> + res->ptr = strbuf_detach(&out, NULL);
> + return 0;
> }
My reviewed-by may not be worth much since this code is new to me
too, but this patch looks "obviously correct" to me, so:
Reviewed-by: Eric Sunshine <sunshine@sunshineco.com>
Perhaps squash in the following test which I adapted from the
reproduction recipe provided by Chris Rossi[1]?
[1] https://gist.github.com/chrisrossi/f09c8bed70b364f9f12e
--- 8< ---
diff --git a/t/t4300-merge-tree.sh b/t/t4300-merge-tree.sh
index 9015e47..1f2aa74 100755
--- a/t/t4300-merge-tree.sh
+++ b/t/t4300-merge-tree.sh
@@ -352,4 +352,22 @@ test_expect_success 'turn tree to file' '
test_cmp expect actual
'
+test_expect_success "don't underallocate result buffer" '
+ test_when_finished "git checkout master" &&
+ git checkout --orphan some &&
+ git rm -rf . &&
+ printf "b\n" >a &&
+ git add a &&
+ git commit -m "first commit" &&
+ printf "\na" >b &&
+ git add b &&
+ git commit -m "second commit, first branch" &&
+ git checkout @^ &&
+ git checkout -b other &&
+ printf "a" >b &&
+ git add b &&
+ git commit -m "second commit, second branch" &&
+ git merge-tree @^ some other
+'
+
test_done
--- 8< ---
next prev parent reply other threads:[~2016-02-16 5:09 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-02-15 21:39 malloc memory corruption on merge-tree with leading newline Stefan Frühwirth
2016-02-15 21:54 ` Stefan Frühwirth
2016-02-16 1:12 ` [PATCH] merge_blobs: use strbuf instead of manually-sized mmfile_t Jeff King
2016-02-16 5:09 ` Eric Sunshine [this message]
2016-02-16 5:50 ` Jeff King
2016-02-16 12:14 ` Stefan Frühwirth
2016-02-16 20:35 ` Jeff King
2016-02-19 12:43 ` Stefan Frühwirth
2016-02-16 21:27 ` Junio C Hamano
2016-02-19 12:48 ` Stefan Frühwirth
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20160216050915.GA5765@flurp.local \
--to=sunshine@sunshineco.com \
--cc=git@vger.kernel.org \
--cc=peff@peff.net \
--cc=stefan.fruehwirth@uni-graz.at \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).