Re: [RFC PATCH v5 2/3] grep: make PCRE2 aware of custom allocator

From: Johannes Schindelin <Johannes.Schindelin@gmx.de>
To: "Carlo Marcelo Arenas Belón" <carenas@gmail.com>
Cc: git@vger.kernel.org, avarab@gmail.com, gitster@pobox.com,
	l.s.r@web.de, michal.kiedrowicz@gmail.com
Subject: Re: [RFC PATCH v5 2/3] grep: make PCRE2 aware of custom allocator
Date: Tue, 27 Aug 2019 11:07:06 +0200 (CEST)	[thread overview]
Message-ID: <nycvar.QRO.7.76.6.1908271057280.46@tvgsbejvaqbjf.bet> (raw)
In-Reply-To: <20190809030210.18353-3-carenas@gmail.com>

[-- Attachment #1: Type: text/plain, Size: 4486 bytes --]

Hi Carlo,

On Thu, 8 Aug 2019, Carlo Marcelo Arenas Belón wrote:

> 94da9193a6 (grep: add support for PCRE v2, 2017-06-01) didn't include
> a way to override the system allocator, and so it is incompatible with
> USE_NED_ALLOCATOR.  The problem was made visible when an attempt to
> avoid a leak in a data structure that is created by the library was
> passed to NED's free for disposal triggering a segfault in Windows.
>
> PCRE2 requires the use of a general context to override the allocator
> and therefore, there is a lot more code needed than in PCRE1, including
> a couple of wrapper functions.
>
> Extend the grep API with a "destructor" that could be called to cleanup
> any objects that were created and used globally.
>
> Update builtin/grep to use that new API, but any other future
> users should make sure to have matching grep_init/grep_destroy calls
> if they are using the pattern matching functionality (currently only
> relevant when using both NED and PCRE2)
>
> Move the logic to decide if a general context will be needed to an
> earlier phase so it will only be done once per pattern (instead of
> at least once per worker thread) avoiding then the need for locking.
>
> This change does the minimum change required to hopefully solve the
> crash, with the rest of the users of it added later.
>
> Helped-by: René Scharfe <l.s.r@web.de>
> Reported-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
> ---

Unfortunately, this is _still_ incorrect.

I pointed out multiple times that custom allocators can be activated at
run-time rather than compile-time, therefore making the choice at
compile-time is wrong. Besides, there is nothing specific to nedmalloc
about this. So the patch is double-wrong on that account.

The patch has a yet even more immediate problem: t7816.48 is failing in
the CI build for _weeks_ now: it requires that global context to be
initialized, but no code path hits the initialization, resulting in a
very, very ugly:

	BUG: grep.c:516: pcre2_global_context uninitialized

See
https://dev.azure.com/gitgitgadget/git/_build/results?buildId=15151&view=ms.vss-test-web.build-test-results-tab&runId=41282&resultId=101710&paneView=debug
for details.

All of this could be easily avoided. As I had pointed out already, the
obvious fragility is not worth the optimization, and we should just
initialize the global context always, as does this patch
(https://github.com/git-for-windows/git/commit/5e5b959169e6efee73e0b50e464166822b7d2d07).

I don't claim that this is complete, you need to check carefully (for
example, I think you will want to get rid of _all_ the references to
nedmalloc), but this patch is at least a stop-gap measure to fix the CI
build (Junio, would you mind adding this as a SQUASH??? so that this
breakage won't keep the CI build of `pu` in the failing state?):

-- snipsnap --

diff --git a/grep.c b/grep.c
index ec845141bbb..4242ad0b4ae 100644
--- a/grep.c
+++ b/grep.c
@@ -19,7 +19,6 @@ static struct grep_opt grep_defaults;
 #ifdef USE_LIBPCRE2
 static pcre2_general_context *pcre2_global_context;

-#ifdef USE_NED_ALLOCATOR
 static void *pcre2_malloc(PCRE2_SIZE size, MAYBE_UNUSED void *memory_data)
 {
 	return malloc(size);
@@ -30,7 +29,6 @@ static void pcre2_free(void *pointer, MAYBE_UNUSED void *memory_data)
 	return free(pointer);
 }
 #endif
-#endif

 static const char *color_grep_slots[] = {
 	[GREP_COLOR_CONTEXT]	    = "context",
@@ -176,6 +174,12 @@ void grep_init(struct grep_opt *opt, struct repository *repo, const char *prefix
 	struct grep_opt *def = &grep_defaults;
 	int i;

+#if defined(USE_LIBPCRE2)
+	if (!pcre2_global_context)
+		pcre2_global_context = pcre2_general_context_create(
+					pcre2_malloc, pcre2_free, NULL);
+#endif
+
 #ifdef USE_NED_ALLOCATOR
 #ifdef USE_LIBPCRE1
 	pcre_malloc = malloc;
@@ -343,11 +347,6 @@ void append_header_grep_pattern(struct grep_opt *opt,
 void append_grep_pattern(struct grep_opt *opt, const char *pat,
 			 const char *origin, int no, enum grep_pat_token t)
 {
-#if defined(USE_LIBPCRE2) && defined(USE_NED_ALLOCATOR)
-	if (!pcre2_global_context && opt->ignore_case && has_non_ascii(pat))
-		pcre2_global_context = pcre2_general_context_create(
-					pcre2_malloc, pcre2_free, NULL);
-#endif
 	append_grep_pat(opt, pat, strlen(pat), origin, no, t);
 }

--
2.23.0.windows.1
