All of lore.kernel.org
 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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.