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....@web.de>
> Reported-by: Johannes Schindelin <johannes.schinde...@gmx.de>
> Signed-off-by: Carlo Marcelo Arenas Belón <care...@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
