git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: "Nguyễn Thái Ngọc Duy" <pclouds@gmail.com>
To: git@vger.kernel.org
Cc: "Nguyễn Thái Ngọc Duy" <pclouds@gmail.com>
Subject: [PATCH 3/3] Correct conditions to free textconv result data
Date: Mon, 22 Feb 2016 19:52:25 +0700	[thread overview]
Message-ID: <1456145545-5374-3-git-send-email-pclouds@gmail.com> (raw)
In-Reply-To: <1456145545-5374-1-git-send-email-pclouds@gmail.com>

fill_textconv() have four code paths to return a new buffer. Two of
them, run_textconv() and notes_cache_get(), return a newly allocated
buffer. The other two return either a constant string or an existing
buffer (mmfile_t). We can only free the result buffer if it's allocated
by fill_textconv(). Correct all call sites.

Changes in combine-diff.c are not clear from diff output. We need to
force not running fill_textconv() unless the function returns a new
buffer so that it falls to the "else" case and allocates a new buffer
with no conversion.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 Frankly I don't like the way fill_textconv() returns buffer at all.
 But I don't know much about textconv, or want to spend more time on
 it. This patch can be dropped if people come up with a better one.

 builtin/blame.c    | 2 +-
 combine-diff.c     | 4 ++--
 diff.c             | 8 ++++----
 diffcore-pickaxe.c | 5 +++--
 4 files changed, 10 insertions(+), 9 deletions(-)

diff --git a/builtin/blame.c b/builtin/blame.c
index cb6f2fb..b5477ad 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -166,7 +166,7 @@ int textconv_object(const char *path,
 	df = alloc_filespec(path);
 	fill_filespec(df, sha1, sha1_valid, mode);
 	textconv = get_textconv(df);
-	if (!textconv) {
+	if (!textconv || !textconv->textconv) {
 		free_filespec(df);
 		return 0;
 	}
diff --git a/combine-diff.c b/combine-diff.c
index 5571304..c57cefd 100644
--- a/combine-diff.c
+++ b/combine-diff.c
@@ -299,7 +299,7 @@ static char *grab_blob(const struct object_id *oid, unsigned int mode,
 		/* deleted blob */
 		*size = 0;
 		return xcalloc(1, 1);
-	} else if (textconv) {
+	} else if (textconv && textconv->textconv) {
 		struct diff_filespec *df = alloc_filespec(path);
 		fill_filespec(df, oid->hash, 1, mode);
 		*size = fill_textconv(textconv, df, &blob);
@@ -1022,7 +1022,7 @@ static void show_patch_diff(struct combine_diff_path *elem, int num_parent,
 			else
 				result = grab_blob(&oid, elem->mode,
 						   &result_size, NULL, NULL);
-		} else if (textconv) {
+		} else if (textconv && textconv->textconv) {
 			struct diff_filespec *df = alloc_filespec(elem->path);
 			fill_filespec(df, null_sha1, 0, st.st_mode);
 			result_size = fill_textconv(textconv, df, &result);
diff --git a/diff.c b/diff.c
index 5bdc3c0..173ec5b 100644
--- a/diff.c
+++ b/diff.c
@@ -744,9 +744,9 @@ static void emit_rewrite_diff(const char *name_a,
 		emit_rewrite_lines(&ecbdata, '-', data_one, size_one);
 	if (lc_b)
 		emit_rewrite_lines(&ecbdata, '+', data_two, size_two);
-	if (textconv_one)
+	if (textconv_one && textconv_one->textconv)
 		free((char *)data_one);
-	if (textconv_two)
+	if (textconv_two && textconv_two->textconv)
 		free((char *)data_two);
 }
 
@@ -2454,9 +2454,9 @@ static void builtin_diff(const char *name_a,
 			die("unable to generate diff for %s", one->path);
 		if (o->word_diff)
 			free_diff_words_data(&ecbdata);
-		if (textconv_one)
+		if (textconv_one && textconv_one->textconv)
 			free(mf1.ptr);
-		if (textconv_two)
+		if (textconv_two && textconv_two->textconv)
 			free(mf2.ptr);
 		xdiff_clear_find_func(&xecfg);
 	}
diff --git a/diffcore-pickaxe.c b/diffcore-pickaxe.c
index 7715c13..f89e106 100644
--- a/diffcore-pickaxe.c
+++ b/diffcore-pickaxe.c
@@ -7,6 +7,7 @@
 #include "diffcore.h"
 #include "xdiff-interface.h"
 #include "kwset.h"
+#include "userdiff.h"
 
 typedef int (*pickaxe_fn)(mmfile_t *one, mmfile_t *two,
 			  struct diff_options *o,
@@ -150,9 +151,9 @@ static int pickaxe_match(struct diff_filepair *p, struct diff_options *o,
 		 DIFF_FILE_VALID(p->two) ? &mf2 : NULL,
 		 o, regexp, kws);
 
-	if (textconv_one)
+	if (textconv_one && textconv_one->textconv)
 		free(mf1.ptr);
-	if (textconv_two)
+	if (textconv_two && textconv_two->textconv)
 		free(mf2.ptr);
 	diff_free_filespec_data(p->one);
 	diff_free_filespec_data(p->two);
-- 
2.7.1.532.gd9e3aaa

  parent reply	other threads:[~2016-02-22 12:52 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <1456145545-5374-1-git-send-email-pclouds@gmail.com>
2016-02-22 12:52 ` [PATCH 2/3] diff.c: remove unnecessary typecast Nguyễn Thái Ngọc Duy
2016-02-22 12:52 ` Nguyễn Thái Ngọc Duy [this message]
2016-02-22 13:00   ` [PATCH 3/3] Correct conditions to free textconv result data Duy Nguyen
2016-02-22 18:06   ` Jeff King
2016-02-22 18:12     ` Jeff King
2016-02-22 23:03       ` Duy Nguyen
2016-02-22 23:08         ` Junio C Hamano
2016-02-22 23:25           ` Jeff King
2016-02-22 18:28     ` [PATCH] diff: clarify textconv interface Jeff King
2016-02-22 23:01       ` Duy Nguyen

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=1456145545-5374-3-git-send-email-pclouds@gmail.com \
    --to=pclouds@gmail.com \
    --cc=git@vger.kernel.org \
    /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).