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@xxxxxx> > Reported-by: Johannes Schindelin <johannes.schindelin@xxxxxx> > Signed-off-by: Carlo Marcelo Arenas Belón <carenas@xxxxxxxxx> > --- 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
