Re: [PATCH] 2.36 format-patch regression fix

["René Scharfe" <l.s.r@web.de>]

Am 30.04.22 um 18:32 schrieb Carlo Marcelo Arenas Belón:
> On Sat, Apr 30, 2022 at 12:32:44PM +0200, René Scharfe wrote:
>> e900d494dc (diff: add an API for deferred freeing, 2021-02-11) added a
>> way to allow reusing diffopts: the no_free bit.  244c27242f (diff.[ch]:
>> have diff_free() call clear_pathspec(opts.pathspec), 2022-02-16) made
>> that mechanism mandatory.
>>
>> git format-patch only sets no_free when --output is given, causing it to
>> forget pathspecs after the first commit.  Set no_free unconditionally
>> instead.
>
> I remember when I saw the first commit long ago, and thought; well that is
> very round about way to reintroduce the UNLEAK removal that might have made
> it visible.
>
> Haven't looked too closely, but considering that we were warned[1] the
> interface was hard to use and might cause problems later and it did.
>
> wouldn't it a better and more secure solution to UNLEAK and remove all this
> code, at least until it could be refactored cleanly, of course?

Silently self-destructing pathspecs are a safety hazard indeed.

no_free also affects freeing ignore_regex and parseopts, and even
closing the output file.  I don't know about the file, but leaking the
first two is harmless.  So removing the flag is safe as long as we make
sure the output file is closed as needed.

A safe diff_free() would only be called a particular diffopt once, when
it's no longer needed.  It could check for reuse by setting a flag the
first time, like in the patch below.  1426 tests in 163 test scripts
fail for me with it applied on top of the regression fixes from this
thread.

Removing the diff_free() calls from diff.c::diff_flush() and
log-tree.c::log_tree_commit() reduces this to just one or two in t7527
(seems to be flaky).  Perhaps this is still salvageable?

>
> Carlo
>
> [1] https://lore.kernel.org/git/YCUFNVj7qlt9wzlX@coredump.intra.peff.net/


---
 diff.c | 3 +++
 diff.h | 1 +
 2 files changed, 4 insertions(+)

diff --git a/diff.c b/diff.c
index ef7159968b..01296829b5 100644
--- a/diff.c
+++ b/diff.c
@@ -6458,10 +6458,13 @@ void diff_free(struct diff_options *options)
 	if (options->no_free)
 		return;

+	if (options->is_dead)
+		BUG("double diff_free() on %p", (void *)options);
 	diff_free_file(options);
 	diff_free_ignore_regex(options);
 	clear_pathspec(&options->pathspec);
 	FREE_AND_NULL(options->parseopts);
+	options->is_dead = 1;
 }

 void diff_flush(struct diff_options *options)
diff --git a/diff.h b/diff.h
index 8ae18e5ab1..c31d32ba19 100644
--- a/diff.h
+++ b/diff.h
@@ -398,6 +398,7 @@ struct diff_options {
 	struct strmap *additional_path_headers;

 	int no_free;
+	int is_dead;
 };

 unsigned diff_filter_bit(char status);
--
2.35.3