diff mbox series

[07/20] diff: refactor code to clarify memory ownership of prefixes

Message ID 18dce492df7d4337dd639be767f2fe280083e9f6.1716465556.git.ps@pks.im (mailing list archive)
State Superseded
Headers show
Series Various memory leak fixes | expand

Commit Message

Patrick Steinhardt May 23, 2024, 12:25 p.m. UTC
The source end destination prefixes are tracked in a `const char *`
array, but may at times contain allocated strings. The result is that
those strings may be leaking because we never free them.

Refactor the code to always store allocated strings in those variables,
freeing them as required. This requires us to handle the default values
a bit different compared to before. But given that there is only a
single callsite where we use the variables to `struct diff_options` it's
easy to handle the defaults there.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 diff.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

Comments

Eric Sunshine May 23, 2024, 4:59 p.m. UTC | #1
On Thu, May 23, 2024 at 8:26 AM Patrick Steinhardt <ps@pks.im> wrote:
> The source end destination prefixes are tracked in a `const char *`

s/end/and/

> array, but may at times contain allocated strings. The result is that
> those strings may be leaking because we never free them.
>
> Refactor the code to always store allocated strings in those variables,
> freeing them as required. This requires us to handle the default values
> a bit different compared to before. But given that there is only a
> single callsite where we use the variables to `struct diff_options` it's
> easy to handle the defaults there.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
diff mbox series

Patch

diff --git a/diff.c b/diff.c
index 902df9286a..679ef472f4 100644
--- a/diff.c
+++ b/diff.c
@@ -62,8 +62,8 @@  static char *diff_order_file_cfg;
 int diff_auto_refresh_index = 1;
 static int diff_mnemonic_prefix;
 static int diff_no_prefix;
-static const char *diff_src_prefix = "a/";
-static const char *diff_dst_prefix = "b/";
+static char *diff_src_prefix;
+static char *diff_dst_prefix;
 static int diff_relative;
 static int diff_stat_name_width;
 static int diff_stat_graph_width;
@@ -411,10 +411,12 @@  int git_diff_ui_config(const char *var, const char *value,
 		return 0;
 	}
 	if (!strcmp(var, "diff.srcprefix")) {
-		return git_config_string(&diff_src_prefix, var, value);
+		FREE_AND_NULL(diff_src_prefix);
+		return git_config_string((const char **) &diff_src_prefix, var, value);
 	}
 	if (!strcmp(var, "diff.dstprefix")) {
-		return git_config_string(&diff_dst_prefix, var, value);
+		FREE_AND_NULL(diff_dst_prefix);
+		return git_config_string((const char **) &diff_dst_prefix, var, value);
 	}
 	if (!strcmp(var, "diff.relative")) {
 		diff_relative = git_config_bool(var, value);
@@ -3433,8 +3435,8 @@  void diff_set_noprefix(struct diff_options *options)
 
 void diff_set_default_prefix(struct diff_options *options)
 {
-	options->a_prefix = diff_src_prefix;
-	options->b_prefix = diff_dst_prefix;
+	options->a_prefix = diff_src_prefix ? diff_src_prefix : "a/";
+	options->b_prefix = diff_dst_prefix ? diff_dst_prefix : "b/";
 }
 
 struct userdiff_driver *get_textconv(struct repository *r,
@@ -5371,8 +5373,8 @@  static int diff_opt_default_prefix(const struct option *opt,
 
 	BUG_ON_OPT_NEG(unset);
 	BUG_ON_OPT_ARG(optarg);
-	diff_src_prefix = "a/";
-	diff_dst_prefix = "b/";
+	FREE_AND_NULL(diff_src_prefix);
+	FREE_AND_NULL(diff_dst_prefix);
 	diff_set_default_prefix(options);
 	return 0;
 }