Fix a problem with PCRE2 and nedmalloc, found via Azure Pipelines

For a couple of days, maybe even a week, pu fails consistently, in the Windows job where it tests t7816. The symptom is a segmentation fault.

I finally got to diagnose this, and it looked at first as if there was yet another buffer overrun that was so small that valgrind failed to detect it (see https://github.com/gitgitgadget/git/pull/178). The problem is another one, though: we ask PCRE2 to allocate a table (and it uses the system allocator for that), and then try to release it using nedmalloc's free() replacement.

This is squarely a problem with cb/pcre2-chartables-leakfix in conjunction with an overridden allocator.

Junio, for your convenience, I rebased this patch directly on top of ab/pcre-v2 and pushed out the let-pcre2-respect-nedmalloc branch at
https://github.com/dscho/git ready to be pulled. The rebased version is not technically a bug fix, as I do not see any way that ab/pcre-v2 uses mismatched allocators for malloc()/free(), but just in case that you wanted to have it in v2.23.0 and not on top cb/pcre2-chartables-leakfix...

Johannes Schindelin (1):
pcre2: allow overriding the system allocator

 grep.c | 31 +++++++++++++++++++++++++++----
 1 file changed, 27 insertions(+), 4 deletions(-)

base-commit: 7d3bf76999452ff64c84cec25fd95a7a95744b78
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-306%2Fdscho%2Ffix-for-pcre-chartables-leakfix-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-306/dscho/fix-for-pcre-chartables-leakfix-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/306

gitgitgadget

From: Johannes Schindelin <[email protected]>

Since 7d3bf769994 (grep: avoid leak of chartables in PCRE2, 2019-08-01), we try to release the UTF-8 tables obtained via pcre2_maketables(). To do that, we use the function free(). That is all fine and dandy as long as that refers to the system allocator.

However, when we compile Git with USE_NED_ALLOCATOR (notably on Windows), then free() actually calls nedfree(). But pcre2_maketables() allocated the tables using the system allocator because we did not tell it to use nedmalloc instead.

This leads to segmentation faults when the UTF-8 tables are released, most notably in the t7816-grep-binary-pattern.sh test script.

PCRE2 does have an option to override the allocator it should use, and this patch calls upon it.

As there are other ways to override the system allocator than USE_NED_ALLOCATOR, we choose to specify the allocator we want to use specifically, even if we still use the system allocator.

---
 grep.c | 31 +++++++++++++++++++++++++++----
 1 file changed, 27 insertions(+), 4 deletions(-)

index d4c5d464ad..d6d29fc724 100644
@@ -482,6 +482,27 @@ static void free_pcre1_regexp(struct grep_pat *p)
 #endif /* !USE_LIBPCRE1 */
 
 #ifdef USE_LIBPCRE2
+static void *pcre2_malloc(PCRE2_SIZE size, void *memory_data)
+{
+	return malloc(size);
+}
+
+static void pcre2_free(void *pointer, void *memory_data)
+{
+	return free(pointer);
+}
+
+static pcre2_general_context *get_pcre2_context(void)
+{
+	static pcre2_general_context *context;
+
+	if (!context)
+		context = pcre2_general_context_create(pcre2_malloc,
+						       pcre2_free, NULL);
+
+	return context;
+}
+
 static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt)
 {
 	int error;

@@ -498,8 +519,9 @@ static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt
 
 	if (opt->ignore_case) {
 		if (has_non_ascii(p->pattern)) {
-			p->pcre2_tables = pcre2_maketables(NULL);
-			p->pcre2_compile_context = pcre2_compile_context_create(NULL);
+			p->pcre2_tables = pcre2_maketables(get_pcre2_context());
+			p->pcre2_compile_context =
+				pcre2_compile_context_create(get_pcre2_context());
 			pcre2_set_character_tables(p->pcre2_compile_context,
 							p->pcre2_tables);
 		}

@@ -513,7 +535,8 @@ static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt
 					 p->pcre2_compile_context);
 
 	if (p->pcre2_pattern) {
-		p->pcre2_match_data = pcre2_match_data_create_from_pattern(p->pcre2_pattern, NULL);
+		p->pcre2_match_data =
+			pcre2_match_data_create_from_pattern(p->pcre2_pattern, get_pcre2_context());
 		if (!p->pcre2_match_data)
 			die("Couldn't allocate PCRE2 match data");
 	} else {

@@ -550,7 +573,7 @@ static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt
 			return;
 		}
 
-		p->pcre2_jit_stack = pcre2_jit_stack_create(1, 1024 * 1024, NULL);
+		p->pcre2_jit_stack = pcre2_jit_stack_create(1, 1024 * 1024, get_pcre2_context());
 		if (!p->pcre2_jit_stack)
 			die("Couldn't allocate PCRE2 JIT stack");
 		p->pcre2_match_context = pcre2_match_context_create(NULL);

gitgitgadget

Sorry; I should have thought of this, but assumed was safe since it would be broken
the same way with PCRE1.

Presume git in windows only builds against PCRE2?

LGTM except from the suggestion below that might make the code more "standard" and probably be a good base for a similar PCRE1 fix

And forgot to mention that technically these are not UTF-8 tables but single byte tables like for example the ones used with en_US.ISO8859-1

Carlo

Hi Carlo,

Thank you for pointing that out, I completely missed that.

Junio, do you want me to re-send, or can you touch up the commit message while applying?

Ciao,
Dscho

Hi Carlo,

That's right, we only build against PCRE2.

My thinking about that was that this would add more code, and thus more opportunities to introduce bugs. Also, it's not like the general context really has any _state_ per se. It just registers the current allocator, which we want to assume is constant over the life-time of the process.

So does it really make sense to create one general context per grep pattern? (Or even per grep pattern and thread?)

Given that my patch does _less_ than what you suggest (it really only creates at most _one_ general context per process, not one per internal-grep invocation, possibly per-thread, and it also never calls pcre2_general_context_free()), I highly doubt that your proposed version would have a _smaller_ impact on performance.

Ciao,
Dscho

"standard" and "more consistent" are good things, but I am not sure I should agree with the argument without knowing what you are comparing your suggested improvement with. Whose standard practice are we trying to be consistent with? Keeping dynamic resources hooked to "struct grep_pat" so that (1) different patterns could use different settings when they desire and (2) the resources are not hidden behind a function-scope static and can be discarded when we are done with the pattern, which is the standard in our "grep" subsystem?

I think general context probably corresponds to a bit higher level than individual grep_pat. E.g. when running "grep -e foo -e bar", do we expect resources needed by patterns "foo" and "bar" would want to be allocated and freed by potentially separate <alloc,free> function pairs?

I am not sure about the impact on performance, but if it helps us keep the subsystem reusable by avoiding function-scope static that we cannot clear, that would be a good thing. But "struct grep_pat" may be a bit too fine-grained to control general context.

It was my impression that we were abusing the struct grep_pat to avoid having to deal properly with threading and interlocks.

I agree my wording wasn't clear enough and my hinting a little obscure but the original code is racy and it wouldn't be if the "global context" will be initialized/maintained there; as an added benefit it will be straight forward to clear (together with the rest)

I am not advocating that as a good design, but also think the code will be shorter (which was another rationale for the proposed change, to avoid introducing yet more bugs and since it was even suggested for inclusion in the next release)

no with a different design; but currently even if almost all the time we have the same pattern for all workers (ex: -e foo), why are we doing the compilation (plus JIT translation) and creating this table and all other context pointers (plus a jit stack) once per thread?

just so we can move forward with a better design will send a proposed patch that does things
a little be better as an RFC

the "performance" point I was making was that with the current code the chartable is only created when it is strictly needed (meaning the pattern/haystack will do matching in non UTF-8 mode but with characters with code higher than 127 and therefore MUST agree in a codepage)

most of the time (like when using UTF-8) the chartable (and therefore the global context) is not needed (even when using alternate allocators)

there is a chance that PCRE2 might perform better with NED, but not in my system and since we haven't been using NED with PCRE2 until this proposed change it might be better to do that independently anyway.

Carlo

This is an incomplete reroll for cb/pcre2-chartables-leakfix that attempts to address the root cause of the problem reported[1] by Dscho with PCRE2 and that is that until now PCRE and NED never worked together.

The first patch is likely correct but since I'd been unable to replicate the issue I can't be completely sure, if a kind soul with access to a windows developer environment could give it a try we will know for sure but FWIW it runs fine and doesn't introduce any failures in our tests

The second patch is obviously incomplete but should address the problem reported; there are still more things to consider as explained there

The third patch is the original leak patch rebased on top.

Carlo Marcelo Arenas Belón (3):
grep: make PCRE1 aware of custom allocator
grep: make PCRE2 aware of custom allocator
grep: avoid leak of chartables in PCRE2

 Makefile       |  2 +-
 builtin/grep.c |  1 +
 grep.c         | 46 ++++++++++++++++++++++++++++++++++++++++++----
 grep.h         |  2 ++
 4 files changed, 46 insertions(+), 5 deletions(-)

[1] https://public-inbox.org/git/[email protected]/

2.23.0.rc1

63e7e9d8b6 ("git-grep: Learn PCRE", 2011-05-09) didn't include a way to override the system alocator, and so it is incompatible with USE_NED_ALLOCATOR as reported by Dscho[1] (in similar code from PCRE2)

Make the minimum change possible to ensure this combination is supported

[1] https://public-inbox.org/git/[email protected]

---
 Makefile |  2 +-
 grep.c   | 10 ++++++++++
 2 files changed, 11 insertions(+), 1 deletion(-)

index bd246f2989..4b384f3759 100644
@@ -1764,7 +1764,7 @@ ifdef NATIVE_CRLF
 endif
 
 ifdef USE_NED_ALLOCATOR
-	COMPAT_CFLAGS += -Icompat/nedmalloc
+	COMPAT_CFLAGS += -DUSE_NED_ALLOCATOR -Icompat/nedmalloc
 	COMPAT_OBJS += compat/nedmalloc/nedmalloc.o
 	OVERRIDE_STRDUP = YesPlease
 endif

index cd952ef5d3..0154998695 100644
@@ -150,12 +150,22 @@ int grep_config(const char *var, const char *value, void *cb)
  * Initialize one instance of grep_opt and copy the
  * default values from the template we read the configuration
  * information in an earlier call to git_config(grep_config).
+ *
+ * If using PCRE make sure that the library is configured
+ * to use the right allocator (ex: NED)
  */
 void grep_init(struct grep_opt *opt, struct repository *repo, const char *prefix)
 {
 	struct grep_opt *def = &grep_defaults;
 	int i;
 
+#ifdef USE_NED_ALLOCATOR
+#ifdef USE_LIBPCRE1
+	pcre_malloc = malloc;
+	pcre_free = free;
+#endif
+#endif
+
 	memset(opt, 0, sizeof(*opt));
 	opt->repo = repo;
 	opt->prefix = prefix;

2.23.0.rc1

Hi Carlo,

the patch only fixes the case when NED was chosen at build time, which before this patch meant that the system allocator was used by PCRE1 and NED by git.

That combination happened to work unless any pointer crossed over but neglected any benefit of using NED instead of the system allocator with PCRE1.

the PCRE1 API was therefore safer to use, PCRE2 is not and that will be addressed[1] in a future version by making sure this "layering violation" is covered

runtime custom allocator changes are not affected and will keep working as usual, which (at least in Linux when using LD_PRELOAD which is probably what you are refer to by "preload") means that the dynamic linker will replace all calls to malloc/free with the ones you provided at exec time and before it starts the binary (including whichever libraries it loaded) and therefore will make both git and PCRE2 to use the same allocator.

nedmalloc seems (at least in Linux) to be smart enough to cope with pointers that were not allocated by itself and doesn't crash so presume there is something else in Windows (maybe the dynamic linker, if there is one) that might be also part of the reason for this segfault.

of course, the trigger of this is my patch, so I'll take the blame for this bug regardless and I am looking forward to a fix

in the context that the only available (and integrated with our build system) allocator is NED at compile time, I think it is correct. you are correct though that if another allocator will be added as a compile time option, we will need to do something similar as well.

Carlo

[1] https://bugs.exim.org/show_bug.cgi?id=2429

Most of the code stolen from[1] to easy on comparison and including the deficiency of setting the global context even for patterns that won't need it.

Ideally, the call from grep_init could be moved to a place where it could be set without needing a lock and at least with this approach we have a place to clear it (which is obviously missing more callers, but at least shows how it will look for the grep subcommand)

I had also dropped most other users of the global context in a failed attempt to make the change smaller, but also to keep the current behaviour so that we could see the effect of enabling NED for PCRE2 more clearly.

Sadly, that will likely require a Windows box, as NED (at least our version) is horribly broken in macOS (maybe it wasn't 64 bit clean) and in Linux builds, but I can't reproduce your crasher and it is most likely slower than the system malloc.

[1] https://public-inbox.org/git/[email protected]/

---
 builtin/grep.c |  1 +
 grep.c         | 31 +++++++++++++++++++++++++++++--
 grep.h         |  1 +
 3 files changed, 31 insertions(+), 2 deletions(-)

index 560051784e..e49c20df60 100644
@@ -1145,5 +1145,6 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
 		run_pager(&opt, prefix);
 	clear_pathspec(&pathspec);
 	free_grep_patterns(&opt);
+	grep_destroy();
 	return !hit;
 }

index 0154998695..e748a6d68c 100644
@@ -16,6 +16,20 @@ static int grep_source_is_binary(struct grep_source *gs,
 
 static struct grep_opt grep_defaults;
 
+#ifdef USE_LIBPCRE2
+static pcre2_general_context *pcre2_global_context;
+
+static void *pcre2_malloc(PCRE2_SIZE size, void *memory_data)
+{
+	return malloc(size);
+}
+
+static void pcre2_free(void *pointer, void *memory_data)
+{
+	return free(pointer);
+}
+#endif
+
 static const char *color_grep_slots[] = {
 	[GREP_COLOR_CONTEXT]	    = "context",
 	[GREP_COLOR_FILENAME]	    = "filename",

@@ -153,6 +167,7 @@ int grep_config(const char *var, const char *value, void *cb)
  *
  * If using PCRE make sure that the library is configured
  * to use the right allocator (ex: NED)
+ * if any object is created it should be cleaned up in grep_destroy()
  */
 void grep_init(struct grep_opt *opt, struct repository *repo, const char *prefix)
 {

@@ -164,6 +179,10 @@ void grep_init(struct grep_opt *opt, struct repository *repo, const char *prefix
 	pcre_malloc = malloc;
 	pcre_free = free;
 #endif
+#ifdef USE_LIBPCRE2
+	pcre2_global_context = pcre2_general_context_create(pcre2_malloc,
+							pcre2_free, NULL);
+#endif
 #endif
 
 	memset(opt, 0, sizeof(*opt));

@@ -188,6 +207,13 @@ void grep_init(struct grep_opt *opt, struct repository *repo, const char *prefix
 		color_set(opt->colors[i], def->colors[i]);
 }
 
+void grep_destroy(void)
+{
+#ifdef USE_LIBPCRE2
+	pcre2_general_context_free(pcre2_global_context);
+#endif
+}
+
 static void grep_set_pattern_type_option(enum grep_pattern_type pattern_type, struct grep_opt *opt)
 {
 	/*

@@ -509,7 +535,7 @@ static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt
 
 	if (opt->ignore_case) {
 		if (has_non_ascii(p->pattern)) {
-			character_tables = pcre2_maketables(NULL);
+			character_tables = pcre2_maketables(pcre2_global_context);
 			p->pcre2_compile_context = pcre2_compile_context_create(NULL);
 			pcre2_set_character_tables(p->pcre2_compile_context, character_tables);
 		}

@@ -560,7 +586,8 @@ static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt
 			return;
 		}
 
-		p->pcre2_jit_stack = pcre2_jit_stack_create(1, 1024 * 1024, NULL);
+		p->pcre2_jit_stack = pcre2_jit_stack_create(1, 1024 * 1024,
+					pcre2_global_context);
 		if (!p->pcre2_jit_stack)
 			die("Couldn't allocate PCRE2 JIT stack");
 		p->pcre2_match_context = pcre2_match_context_create(NULL);

index 1875880f37..526c2db9ef 100644
@@ -189,6 +189,7 @@ struct grep_opt {
 void init_grep_defaults(struct repository *);
 int grep_config(const char *var, const char *value, void *);
 void grep_init(struct grep_opt *, struct repository *repo, const char *prefix);
+void grep_destroy(void);
 void grep_commit_pattern_type(enum grep_pattern_type, struct grep_opt *opt);
 
 void append_grep_pat(struct grep_opt *opt, const char *pat, size_t patlen, const char *origin, int no, enum grep_pat_token t);

2.23.0.rc1

Hi Carlo,

Actually not so much suggested by me, as your patch still causes crashes (mine didn't):

https://dev.azure.com/gitgitgadget/git/_build/results?buildId=13935&view=ms.vss-test-web.build-test-results-tab

Ciao,
Dscho

the "equivalent" version in v4[1], that is still not on pu shouldn't, if my hunch is correct

it also shouldn't crash if the test wouldn't be doing LC_ALL="C" but we have yet a couple of extra bugs related to that, including one[2] I CC you on which would be nice to get validated as well.

Carlo

[1] https://public-inbox.org/git/[email protected]/
[2] https://public-inbox.org/git/[email protected]/

94da9193a6 ("grep: add support for PCRE v2", 2017-06-01) introduced a small memory leak visible with valgrind in t7813.

Complete the creation of a PCRE2 specific variable that was missing from the original change and free the generated table just like it is done for PCRE1.

---
 grep.c | 7 ++++---
 grep.h | 1 +
 2 files changed, 5 insertions(+), 3 deletions(-)

index e748a6d68c..3e14ec91a6 100644
@@ -524,7 +524,6 @@ static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt
 	PCRE2_UCHAR errbuf[256];
 	PCRE2_SIZE erroffset;
 	int options = PCRE2_MULTILINE;
-	const uint8_t *character_tables = NULL;
 	int jitret;
 	int patinforet;
 	size_t jitsizearg;

@@ -535,9 +534,10 @@ static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt
 
 	if (opt->ignore_case) {
 		if (has_non_ascii(p->pattern)) {
-			character_tables = pcre2_maketables(pcre2_global_context);
+			p->pcre2_tables = pcre2_maketables(pcre2_global_context);
 			p->pcre2_compile_context = pcre2_compile_context_create(NULL);
-			pcre2_set_character_tables(p->pcre2_compile_context, character_tables);
+			pcre2_set_character_tables(p->pcre2_compile_context,
+							p->pcre2_tables);
 		}
 		options |= PCRE2_CASELESS;
 	}

@@ -642,6 +642,7 @@ static void free_pcre2_pattern(struct grep_pat *p)
 	pcre2_match_data_free(p->pcre2_match_data);
 	pcre2_jit_stack_free(p->pcre2_jit_stack);
 	pcre2_match_context_free(p->pcre2_match_context);
+	free((void *)p->pcre2_tables);
 }
 #else /* !USE_LIBPCRE2 */
 static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt)

index 526c2db9ef..533165c21a 100644
@@ -96,6 +96,7 @@ struct grep_pat {
 	pcre2_compile_context *pcre2_compile_context;
 	pcre2_match_context *pcre2_match_context;
 	pcre2_jit_stack *pcre2_jit_stack;
+	const uint8_t *pcre2_tables;
 	uint32_t pcre2_jit_on;
 	kwset_t kws;
 	unsigned fixed:1;

2.23.0.rc1

This series is a candidate reroll for cb/pcre2-chartables-leakfix, that hopefully addresses the root cause of the problem reported by Dscho in Windows, where the PCRE2 library wasn't aware of the custom allocator and was returning a pointer created with the system malloc but passing it to NED's free, resulting in a segfault.

The reason why it was triggered by the original leak fix is the layering violation reported by René and that is exclusive to PCRE2 (hence why it hasn't been reported with PCRE1). The first patch could be droped and then used in a different series that will fully integrate the custom allocator with the PCRE library and that is currently missing with PCRE2.

Eitherway, since I am unable to replicate the original bug or take performance numbers in a representative environment without Windows this is only published as an RFC, eventhough it has been tested and considered mostly complete.

Junio, could you comment in my assumption that the use of grep in revision.c doesn't require initializing a PCRE2 global context and therefore not doing the cleanup?

Carlo Marcelo Arenas Belón (3):
grep: make PCRE1 aware of custom allocator
grep: make PCRE2 aware of custom allocator
grep: avoid leak of chartables in PCRE2

 Makefile       |  2 +-
 builtin/grep.c |  1 +
 grep.c         | 51 +++++++++++++++++++++++++++++++++++++++++++++++---
 grep.h         |  2 ++
 4 files changed, 52 insertions(+), 4 deletions(-)

2.23.0.rc1

63e7e9d8b6 ("git-grep: Learn PCRE", 2011-05-09) didn't include a way to override the system alocator, and so it is incompatible with USE_NED_ALLOCATOR as reported by Dscho[1] (in similar code from PCRE2)

Make the minimum change possible to ensure this combination is supported by extending grep_init to set the PCRE1 specific functions to the NED versions (using the aliased names though) and therefore making sure all alocations are done inside PCRE1 with the same allocator than the rest of git.

This change might have performance impact (hopefully for the best) and so will be good idea to test it in a platform where NED might have a positive impact (ex: Windows 7)

[1] https://public-inbox.org/git/[email protected]

---
 Makefile |  2 +-
 grep.c   | 10 ++++++++++
 2 files changed, 11 insertions(+), 1 deletion(-)

index bd246f2989..4b384f3759 100644
@@ -1764,7 +1764,7 @@ ifdef NATIVE_CRLF
 endif
 
 ifdef USE_NED_ALLOCATOR
-	COMPAT_CFLAGS += -Icompat/nedmalloc
+	COMPAT_CFLAGS += -DUSE_NED_ALLOCATOR -Icompat/nedmalloc
 	COMPAT_OBJS += compat/nedmalloc/nedmalloc.o
 	OVERRIDE_STRDUP = YesPlease
 endif

index cd952ef5d3..0154998695 100644
@@ -150,12 +150,22 @@ int grep_config(const char *var, const char *value, void *cb)
  * Initialize one instance of grep_opt and copy the
  * default values from the template we read the configuration
  * information in an earlier call to git_config(grep_config).
+ *
+ * If using PCRE make sure that the library is configured
+ * to use the right allocator (ex: NED)
  */
 void grep_init(struct grep_opt *opt, struct repository *repo, const char *prefix)
 {
 	struct grep_opt *def = &grep_defaults;
 	int i;
 
+#ifdef USE_NED_ALLOCATOR
+#ifdef USE_LIBPCRE1
+	pcre_malloc = malloc;
+	pcre_free = free;
+#endif
+#endif
+
 	memset(opt, 0, sizeof(*opt));
 	opt->repo = repo;
 	opt->prefix = prefix;

2.23.0.rc1

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 some of the logic that was before done per thread (in the workers) into an earlier phase to avoid degrading performance, but as the use of PCRE2 with NED is better understood it is expected more of its functions will be instructed to use the custom allocator as well as was done in the original code[1] this work was based on.

[1] https://public-inbox.org/git/3397e6797f872aedd18c6d795f4976e1c579514b.1565005867.git.gitgitgadget@gmail.com/

---
 builtin/grep.c |  1 +
 grep.c         | 36 +++++++++++++++++++++++++++++++++++-
 grep.h         |  1 +
 3 files changed, 37 insertions(+), 1 deletion(-)

index 560051784e..e49c20df60 100644
@@ -1145,5 +1145,6 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
 		run_pager(&opt, prefix);
 	clear_pathspec(&pathspec);
 	free_grep_patterns(&opt);
+	grep_destroy();
 	return !hit;
 }

index 0154998695..3e5bdf94a6 100644
@@ -16,6 +16,22 @@ static int grep_source_is_binary(struct grep_source *gs,
 
 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);
+}
+
+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",
 	[GREP_COLOR_FILENAME]	    = "filename",

@@ -153,6 +169,7 @@ int grep_config(const char *var, const char *value, void *cb)
  *
  * If using PCRE make sure that the library is configured
  * to use the right allocator (ex: NED)
+ * if any object is created it should be cleaned up in grep_destroy()
  */
 void grep_init(struct grep_opt *opt, struct repository *repo, const char *prefix)
 {

@@ -188,6 +205,13 @@ void grep_init(struct grep_opt *opt, struct repository *repo, const char *prefix
 		color_set(opt->colors[i], def->colors[i]);
 }
 
+void grep_destroy(void)
+{
+#ifdef USE_LIBPCRE2
+	pcre2_general_context_free(pcre2_global_context);
+#endif
+}
+
 static void grep_set_pattern_type_option(enum grep_pattern_type pattern_type, struct grep_opt *opt)
 {
 	/*

@@ -319,6 +343,11 @@ 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);
 }
 

@@ -507,9 +536,14 @@ static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt
 
 	p->pcre2_compile_context = NULL;
 
+	/* pcre2_global_context is initialized in append_grep_pattern */
 	if (opt->ignore_case) {
 		if (has_non_ascii(p->pattern)) {
-			character_tables = pcre2_maketables(NULL);
+#ifdef USE_NED_ALLOCATOR
+			if (!pcre2_global_context)
+				BUG("pcre2_global_context uninitialized");
+#endif
+			character_tables = pcre2_maketables(pcre2_global_context);
 			p->pcre2_compile_context = pcre2_compile_context_create(NULL);
 			pcre2_set_character_tables(p->pcre2_compile_context, character_tables);
 		}

index 1875880f37..526c2db9ef 100644
@@ -189,6 +189,7 @@ struct grep_opt {
 void init_grep_defaults(struct repository *);
 int grep_config(const char *var, const char *value, void *);
 void grep_init(struct grep_opt *, struct repository *repo, const char *prefix);
+void grep_destroy(void);
 void grep_commit_pattern_type(enum grep_pattern_type, struct grep_opt *opt);
 
 void append_grep_pat(struct grep_opt *opt, const char *pat, size_t patlen, const char *origin, int no, enum grep_pat_token t);

2.23.0.rc1

Am 06.08.19 um 18:36 schrieb Carlo Marcelo Arenas Belón:

Which logic is moved? In the patch I basically only see additions.

I'm not sure I understand that part. Do you mean there are gaps of knowledge about nedmalloc and/or PCRE2 and/or their interaction? And someone is working on closing those gaps, and is going to submit more patches in the process?

Your patch uses a custom global context only if USE_NED_ALLOCATOR is defined, while [1] does it unconditionally. The latter is easier to debug and requires less preprocessor directives. What's the upside of your approach?

the decision of if we want to create a global context or not, which is now being done only once per pattern. "moving" there was to imply the logic now was no longer going to always use state in the struct_pat that then will be evaluated at least once per thread, but could use state globally and reuse some of the work (as I did for the generation of the chartables in a later patch, that I had yet to send)

was hoping will perform better but it seems that testing can be done only in windows

good point, note though that since it is inside a USE_NED_ALLOCATOR ifdef it is really
nedalloc in disguise

this was just for help migrating the code, could go away even now, but though it was nice to keep for safety (as someone mentioned recently)

and a nicer API that allows for cleaning up any global objects (with the chartables being an easy one to tackle next) and might
help in the future to make the worker threads less messy (ex: compile vs match)

correct, was trying to be conservative and minimal (since this code will conflict as well
with other in the fly changes), but considering that the last Azure build from pu still failed
and I have no windows box to debug, might need to do it anyway.

ok

Carlo

Am 07.08.19 um 11:49 schrieb Carlo Arenas:

nedmalloc works on other platforms as well. On Debian Testing with GCC 9.1.0 I need two changes to suppress some compiler warnings, though. Will post them as replies.

"make USE_NED_ALLOCATOR=1 test" then reports these failures:

t7816-grep-binary-pattern.sh (Wstat: 256 Tests: 145 Failed: 5)
Failed tests: 48, 54, 57, 60, 63
Non-zero exit status: 1

And the first one when running that test with --verbose and --immediate is showing:

BUG: grep.c:510: pcre2_global_context uninitialized

Aborted
not ok 48 - LC_ALL='C' git grep -P -f f -i '[æ]<NUL>ð' a #
# printf '[æ]Qð' | q_to_nul >f &&
# LC_ALL='C' git grep -P -f f -i a
#

Avoid the following compiler warning:

In file included from compat/nedmalloc/nedmalloc.c:63:
compat/nedmalloc/malloc.c.h: In function ‘pthread_release_lock’:
compat/nedmalloc/malloc.c.h:1759:5: error: ISO C90 forbids mixed declarations and code [-Werror=declaration-after-statement]

 1759 |     volatile unsigned int* lp = &sl->l;
      |     ^~~~~~~~

---
 compat/nedmalloc/malloc.c.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index b833ff9225..88c131ca93 100644
@@ -1755,10 +1755,10 @@ static FORCEINLINE void pthread_release_lock (MLOCK_T *sl) {
   assert(sl->l != 0);
   assert(sl->threadid == CURRENT_THREAD);
   if (--sl->c == 0) {
-    sl->threadid = 0;
     volatile unsigned int* lp = &sl->l;
     int prev = 0;
     int ret;
+    sl->threadid = 0;
     __asm__ __volatile__ ("lock; xchgl %0, %1"
 			  : "=r" (ret)
 			  : "m" (*(lp)), "0"(prev)

--
2.22.0

Cast the evaluated value of the macro INITIAL_LOCK to void to instruct the compiler that we're not interested in said value nor the following warning:

In file included from compat/nedmalloc/nedmalloc.c:63:
compat/nedmalloc/malloc.c.h: In function ‘init_user_mstate’:
compat/nedmalloc/malloc.c.h:1706:62: error: right-hand operand of comma expression has no effect [-Werror=unused-value]

 1706 | #define INITIAL_LOCK(sl)      (memset(sl, 0, sizeof(MLOCK_T)), 0)
      |                               ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~
compat/nedmalloc/malloc.c.h:5020:3: note: in expansion of macro ‘INITIAL_LOCK’
 5020 |   INITIAL_LOCK(&m->mutex);
      |   ^~~~~~~~~~~~

---
 compat/nedmalloc/malloc.c.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

index 88c131ca93..9134349590 100644
@@ -3066,7 +3066,7 @@ static int init_mparams(void) {
 #if !ONLY_MSPACES
     /* Set up lock for main malloc area */
     gm->mflags = mparams.default_mflags;
-    INITIAL_LOCK(&gm->mutex);
+    (void)INITIAL_LOCK(&gm->mutex);
 #endif
 
 #if (FOOTERS && !INSECURE)

@@ -5017,7 +5017,7 @@ static mstate init_user_mstate(char* tbase, size_t tsize) {
   mchunkptr msp = align_as_chunk(tbase);
   mstate m = (mstate)(chunk2mem(msp));
   memset(m, 0, msize);
-  INITIAL_LOCK(&m->mutex);
+  (void)INITIAL_LOCK(&m->mutex);
   msp->head = (msize|PINUSE_BIT|CINUSE_BIT);
   m->seg.base = m->least_addr = tbase;
   m->seg.size = m->footprint = m->max_footprint = tsize;

--
2.22.0

I meant[1] it works reliably enough to be useful for performance testing.

goes without saying that the fact that I am using a virtualbox with 2 CPUs running Debian 10 on top of macOS (a macbook pro with 4 cores) and the test uses by default 8 threads, doesn't help, but to share my pain here is the result of running p7820 with my last reroll on top of pu, comparing a build of the same code without NED (this tree) with one with it (HEAD)

Test this tree
HEAD
-------------------------------------------------------------------------------------------
7820.1: basic grep -i 'how.to' 0.89(1.12+0.46)
0.95(1.23+0.49) +6.7%
7820.2: extended grep -i 'how.to' 0.90(1.12+0.49)
0.92(1.19+0.46) +2.2%
7820.3: perl grep -i 'how.to' 0.54(0.30+0.52)
0.53(0.39+0.52) -1.9%
7820.5: basic grep -i '^how to' 0.89(1.13+0.47)
0.91(1.13+0.49) +2.2%
7820.6: extended grep -i '^how to' 0.84(1.04+0.49)
0.94(1.27+0.47) +11.9%
7820.7: perl grep -i '^how to' 0.49(0.34+0.47)
0.51(0.36+0.49) +4.1%
7820.9: basic grep -i '[how] to' 1.51(2.31+0.51)
1.55(2.38+0.51) +2.6%
7820.10: extended grep -i '[how] to' 1.50(2.20+0.59)
1.56(2.30+0.62) +4.0%
7820.11: perl grep -i '[how] to' 0.67(0.50+0.52)
0.62(0.50+0.55) -7.5%
7820.13: basic grep -i '\(e.t[^ ]*\|v.ry\) rare' 2.58(4.39+0.56)
2.64(4.45+0.60) +2.3%
7820.14: extended grep -i '(e.t[^ ]*|v.ry) rare' 2.60(4.41+0.56)
2.66(4.58+0.56) +2.3%
7820.15: perl grep -i '(e.t[^ ]*|v.ry) rare' 1.17(1.66+0.53)
1.23(1.84+0.45) +5.1%
7820.17: basic grep -i 'm\(ú\|u\)lt.b\(æ\|y\)te' 1.12(1.54+0.51)
1.14(1.70+0.44) +1.8%
7820.18: extended grep -i 'm(ú|u)lt.b(æ|y)te' 1.09(1.54+0.48)
1.14(1.62+0.49) +4.6%
7820.19: perl grep -i 'm(ú|u)lt.b(æ|y)te' 0.87(1.09+0.46)
0.90(1.20+0.43) +3.4%

and here one comparing two builds (both with NED)

Test origin/pu
HEAD
-------------------------------------------------------------------------------------------
7820.1: basic grep -i 'how.to' 1.00(1.24+0.55)
0.94(1.19+0.52) -6.0%
7820.2: extended grep -i 'how.to' 0.90(1.15+0.49)
0.93(1.23+0.44) +3.3%
7820.3: perl grep -i 'how.to' 0.52(0.37+0.51)
0.59(0.34+0.53) +13.5%
7820.5: basic grep -i '^how to' 0.89(1.16+0.48)
0.90(1.17+0.47) +1.1%
7820.6: extended grep -i '^how to' 0.92(1.17+0.50)
0.92(1.20+0.45) +0.0%
7820.7: perl grep -i '^how to' 0.45(0.33+0.42)
0.54(0.29+0.57) +20.0%
7820.9: basic grep -i '[how] to' 1.60(2.46+0.52)
1.61(2.39+0.62) +0.6%
7820.10: extended grep -i '[how] to' 1.71(2.67+0.56)
1.57(2.41+0.54) -8.2%
7820.11: perl grep -i '[how] to' 0.66(0.61+0.51)
0.59(0.44+0.51) -10.6%
7820.13: basic grep -i '\(e.t[^ ]*\|v.ry\) rare' 2.69(4.49+0.66)
2.67(4.49+0.60) -0.7%
7820.14: extended grep -i '(e.t[^ ]*|v.ry) rare' 2.67(4.49+0.64)
2.64(4.54+0.54) -1.1%
7820.15: perl grep -i '(e.t[^ ]*|v.ry) rare' 1.23(1.80+0.47)
1.25(1.89+0.46) +1.6%
7820.17: basic grep -i 'm\(ú\|u\)lt.b\(æ\|y\)te' 1.13(1.64+0.47)
1.14(1.64+0.48) +0.9%
7820.18: extended grep -i 'm(ú|u)lt.b(æ|y)te' 1.16(1.68+0.46)
1.20(1.60+0.60) +3.4%
7820.19: perl grep -i 'm(ú|u)lt.b(æ|y)te' 0.90(1.16+0.48)
0.88(1.17+0.45) -2.2%

with the only relevant line (for my code) being 7820.19 where it would seem it performs almost the same (eventhough just adding NED made it initially worst)

note though that the fact there are 20% swings in parts of the code that hasn't changed
or that where explicitly #ifdef out of my code changes doesn't give me much confidence, but since the windows guys seem to be using NED by default, I am hoping it works better there.

note also there were no segfaults (which is what was reported originally) so something
else must be off.

you got me so excited I dusted and old PC and was downloading the debian ISO when I realized the error was not a segfault[2] but my bug.

sorry about it; I would swear I run the full test and it was clean, but it was most likely
with PCRE1 or the system malloc, and definitely too late at night.

Thanks for your help so far, and while I know it is VERY ugly v4 at least addresses that
(and uncovered a couple more bugs), thanks also Junio for rerolling it into pu so at least
we have a chance for it to build and run, and hopefully eventually pass.

Carlo

[1] https://public-inbox.org/git/[email protected]/
[2] https://dev.azure.com/git/git/_build/results?buildId=832

Am 08.08.19 um 04:35 schrieb Carlo Arenas:

You mentioned being concerned about performance several times and I wondered why each time. I'd expect no measurable difference between using a custom global context and the internal one of PCRE2 -- setting two function pointers surely can't take very long, can it? But measuring is better than guessing, of course.

nedmalloc is supposed to run on macOS as well.

These measurement results are quite noisy, so I wouldn't trust them too much. nedmalloc being slower than the one from a recent glibc version is not very surprising given this statement from its home page,
https://www.nedprod.com/programs/portable/nedmalloc/:

"Windows 7, Linux 3.x, FreeBSD 8, Mac OS X 10.6 all contain
state-of-the-art allocators and no third party allocator is likely to significantly improve on them in real world results"

In particular I don't think that these results justify coupling the use of nedmalloc to the choice of using a custom global context for PCRE2.

I'd expect:
- Without USE_NED_ALLOCATOR: xmalloc() should be used for all allocations, including for PCRE2. Some special exceptions use malloc(3) directly, but for most uses we want the consistent out-of-memory handling that xmalloc() brings.
- With USE_NED_ALLOCATOR: malloc() and xmalloc() use nedmalloc behind the scenes and free() is similarly overridden, so all allocations are affected.
- If USE_NED_ALLOCATOR performs worse than the system allocator on some system then it's the problem of those that turn on that flag.

Makes sense?

René

setting the allocator is not a concern, but using it; it requires an extra indirect function call which is usually not very friendly to caches in our speculative execution CPU world. our implementation also adds the wrapper call overhead, but in this case it is just the "cost of doing business" with PCRE2.

compilers had gotten a lot better since (mainly because of C++ and the need for it with virtual methods) but I would rather measure.

the last version has some "fix miscompilations in macOS" fixes that might be relevant, and the version we have in tree says it works in the 32-bit version which latest macOS versions are working hard to deprecate (can't even build for it anymore), eitherway trying to run with a nedmalloc enabled git in macOS is not fun.

neither did I either, the only reason I am holding on fully enabling NED with PCRE2 in my series is just because I wan't to make sure we have identified the bug correctly and we are fixing it (specially since I can't reproduce it, and therefore neither debug it)

sorry for not making that clear enough, and as I said before, if we keep seeing segfaults even after v4 then we will have to do that or I might need to do a quick run to the nearest microsoft store hoping for a distracted rep, instead.

that is already in v4 and would expect to carry it forward. this is also what I had in mind when I said we will need some fixes on top of Dscho version if we give up with these.

completely, but note also that Dscho version would make the performance impacts of using a custom allocator (if any) affect everyone using PCRE2.

Carlo

Am 08.08.19 um 14:38 schrieb Carlo Arenas:

PCRE2 needs to allocate memory once per program run for the character table and for each pattern compilation. These are both rare events compared to matching patterns against lines, and I suspect that compilation in particular has much more other work to do, even more so with JIT enabled; I'd expect function call indirection to not make much of a difference.

pcre2_compile() always calls the allocation function through a function pointer in a context struct, by the way (see line 9695 in
https://vcs.pcre.org/pcre2/code/trunk/src/pcre2_compile.c?view=markup
or search for "malloc" in that file).

Importing the latest version of nedmalloc might make sense in general. The last commit in git://github.com/ned14/nedmalloc.git was done five years ago; is it finished? A diffstat with -b looks like this:

 malloc.c.h  | 1193 ++++++++++++++++++++++++++++-------------
 nedmalloc.c | 1720 +++++++++++++++++++++++++++++++++++++++++++++++++++++-------
 nedmalloc.h | 1580 ++++++++++++++++++++++++++++++++++++++++++++++++++++---
 3 files changed, 3840 insertions(+), 653 deletions(-)

Any nedmalloc fans interested in bringing the goodies hidden in there to Git (presumably while retaining our local fixes)?

That's a good reason against #ifdefs in general. Sometimes they are unavoidable, but they can make maintenance a lot harder.

Asking to buy a license for Windows Vista might cause quite a bit of a distraction in there -- Microsoft's support for that version ended two years ago. :) It still seems to be popular enough to be supported by Git for Windows, however.

(You could buy Windows 10 and probably get a downgrade right, but finding legit install media for Vista might be challenging.)

But I'd say do the easy thing: Custom global context for all.

Sounds fine to me, if the performance numbers don't take too much of a hit. I'd be surprised if the needle moved at all (ignoring noise).

René

Hi René,

I had looked into this already over two years ago, and had to stop after investigating a performance regression for two weeks and not getting anywhere.

Also, nedmalloc fell unmaintained, so I don't necessarily think that it would be a good idea to spend a lot of time on it.

In the meantime, there is a much more viable contender: mi-malloc. Preliminary tests suggest that its performance on Windows is at least as good as nedmalloc's, and Windows was the use case for which we integrated nedmalloc into Git's compat/ in the first place.

I have tentative patches to integrate it into Git for Windows, and basically got side-tracked with other things. Expect to see something regarding mi-malloc from me in September.

Ciao,
Dscho

It would be nedalloc() wrapped inside the "die if we cannot allocate, possibly after releasing resources held in various caches" xmalloc() wrapper. So (1) either xmalloc or malloc would end up eventually calling nedalloc that can be freed with nedfree, so from that point of view either can be used without upsetting "free()", and (2) we should use xmalloc() here because we care about consistent OOM behaviour throughout the system.

Thanks.

94da9193a6 ("grep: add support for PCRE v2", 2017-06-01) introduced a small memory leak visible with valgrind in t7813.

Complete the creation of a PCRE2 specific variable that was missing from the original change and free the generated table just like it is done for PCRE1.

---
 grep.c | 7 ++++---
 grep.h | 1 +
 2 files changed, 5 insertions(+), 3 deletions(-)

index 3e5bdf94a6..3d3ea0414e 100644
@@ -527,7 +527,6 @@ static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt
 	PCRE2_UCHAR errbuf[256];
 	PCRE2_SIZE erroffset;
 	int options = PCRE2_MULTILINE;
-	const uint8_t *character_tables = NULL;
 	int jitret;
 	int patinforet;
 	size_t jitsizearg;

@@ -543,9 +542,10 @@ static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt
 			if (!pcre2_global_context)
 				BUG("pcre2_global_context uninitialized");
 #endif
-			character_tables = pcre2_maketables(pcre2_global_context);
+			p->pcre2_tables = pcre2_maketables(pcre2_global_context);
 			p->pcre2_compile_context = pcre2_compile_context_create(NULL);
-			pcre2_set_character_tables(p->pcre2_compile_context, character_tables);
+			pcre2_set_character_tables(p->pcre2_compile_context,
+							p->pcre2_tables);
 		}
 		options |= PCRE2_CASELESS;
 	}

@@ -649,6 +649,7 @@ static void free_pcre2_pattern(struct grep_pat *p)
 	pcre2_match_data_free(p->pcre2_match_data);
 	pcre2_jit_stack_free(p->pcre2_jit_stack);
 	pcre2_match_context_free(p->pcre2_match_context);
+	free((void *)p->pcre2_tables);
 }
 #else /* !USE_LIBPCRE2 */
 static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt)

index 526c2db9ef..533165c21a 100644
@@ -96,6 +96,7 @@ struct grep_pat {
 	pcre2_compile_context *pcre2_compile_context;
 	pcre2_match_context *pcre2_match_context;
 	pcre2_jit_stack *pcre2_jit_stack;
+	const uint8_t *pcre2_tables;
 	uint32_t pcre2_jit_on;
 	kwset_t kws;
 	unsigned fixed:1;

2.23.0.rc1

Many callers of setup_revisions() do so only to run two-thing diffs (e.g. run_diff_files() that compares the index and the working tree), but some do so to traverse the history with the grep_filter (e.g. "git log --grep=<pattern>"). "git log -P" may affect how that pattern is used to match the contents of the log messages, no?

This series is a candidate reroll for cb/pcre2-chartables-leakfix, that hopefully addresses the root cause of the problem reported by Dscho in Windows, where the PCRE2 library wasn't aware of the custom allocator and was returning a pointer created with the system malloc but passing it to NED's free, resulting in a segfault.

The reason why it was triggered by the original leak fix is the layering violation reported by René and that is exclusive to PCRE2 (hence why it hasn't been reported with PCRE1). Additional work might be available in a future release of PCRE2 to address that as detailed in the upstream bug[1] report.

Eitherway, since I am unable to replicate the original bug or take performance numbers in a representative environment without Windows this is only published as an RFC,

Changes since v3 (mostly in patch 2):

* git log also calls the "destructor" for grep API
* no more "bug" being triggered by make test, sorry René
* hopefully no more crashes in windows (I was expecting at most a BUG)

Future work (other than the needed refactoring explained in the second patch) and adjacent bugs, includes:

* tracking more possible users of the grep API that might need to call grep_destroy()
* completely moving PCRE2 to use NED (as is done with PCRE1 and was proposed on the original patch[2] this is based on
* build on top of the new API so that other work could be shared (for example the chartables that started this whole mess)

or (hopefully not)

* ignore the original leak (maybe with an UNLEAK) as René suggested [3]
* discard this work and just use Dscho's fix (probably with some improvements)

Carlo Marcelo Arenas Belón (3):
grep: make PCRE1 aware of custom allocator
grep: make PCRE2 aware of custom allocator
grep: avoid leak of chartables in PCRE2

 Makefile       |  2 +-
 builtin/grep.c |  1 +
 builtin/log.c  |  1 +
 grep.c         | 77 ++++++++++++++++++++++++++++++++++++++++++++------
 grep.h         |  2 ++
 5 files changed, 73 insertions(+), 10 deletions(-)

[1] https://bugs.exim.org/show_bug.cgi?id=2429
[2] https://public-inbox.org/git/3397e6797f872aedd18c6d795f4976e1c579514b.1565005867.git.gitgitgadget@gmail.com/
[3] https://public-inbox.org/git/[email protected]/

base-commit: 51cf315870bbb7254ddf06c84fe03b41bc48eebd

2.23.0.rc1

63e7e9d8b6 ("git-grep: Learn PCRE", 2011-05-09) didn't include a way to override the system alocator, and so it is incompatible with USE_NED_ALLOCATOR as reported by Dscho[1] (in similar code from PCRE2)

Make the minimum change possible to ensure this combination is supported by extending grep_init to set the PCRE1 specific functions to the NED versions (using the aliased names though) and therefore making sure all alocations are done inside PCRE1 with the same allocator than the rest of git.

This change might have performance impact (hopefully for the best) and so will be good idea to test it in a platform where NED might have a positive impact (ex: Windows 7)

[1] https://public-inbox.org/git/[email protected]

---
 Makefile |  2 +-
 grep.c   | 10 ++++++++++
 2 files changed, 11 insertions(+), 1 deletion(-)

index bd246f2989..4b384f3759 100644
@@ -1764,7 +1764,7 @@ ifdef NATIVE_CRLF
 endif
 
 ifdef USE_NED_ALLOCATOR
-	COMPAT_CFLAGS += -Icompat/nedmalloc
+	COMPAT_CFLAGS += -DUSE_NED_ALLOCATOR -Icompat/nedmalloc
 	COMPAT_OBJS += compat/nedmalloc/nedmalloc.o
 	OVERRIDE_STRDUP = YesPlease
 endif

index cd952ef5d3..0154998695 100644
@@ -150,12 +150,22 @@ int grep_config(const char *var, const char *value, void *cb)
  * Initialize one instance of grep_opt and copy the
  * default values from the template we read the configuration
  * information in an earlier call to git_config(grep_config).
+ *
+ * If using PCRE make sure that the library is configured
+ * to use the right allocator (ex: NED)
  */
 void grep_init(struct grep_opt *opt, struct repository *repo, const char *prefix)
 {
 	struct grep_opt *def = &grep_defaults;
 	int i;
 
+#ifdef USE_NED_ALLOCATOR
+#ifdef USE_LIBPCRE1
+	pcre_malloc = malloc;
+	pcre_free = free;
+#endif
+#endif
+
 	memset(opt, 0, sizeof(*opt));
 	opt->repo = repo;
 	opt->prefix = prefix;

2.23.0.rc1

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,log} 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.

---
V4:
* use xmalloc instead as suggested by René and Junio
* "fix" for regression in t7816 as reported by René

 builtin/grep.c |  1 +
 builtin/log.c  |  1 +
 grep.c         | 62 ++++++++++++++++++++++++++++++++++++++++++++------
 grep.h         |  1 +
 4 files changed, 58 insertions(+), 7 deletions(-)

index 560051784e..e49c20df60 100644
@@ -1145,5 +1145,6 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
 		run_pager(&opt, prefix);
 	clear_pathspec(&pathspec);
 	free_grep_patterns(&opt);
+	grep_destroy();
 	return !hit;
 }

index 1cf9e37736..139b8770e7 100644
@@ -2146,6 +2146,7 @@ int cmd_cherry(int argc, const char **argv, const char *prefix)
 		list = list->next;
 	}
 
+	grep_destroy();
 	free_patch_ids(&ids);
 	return 0;
 }

index 1875880f37..526c2db9ef 100644
@@ -189,6 +189,7 @@ struct grep_opt {
 void init_grep_defaults(struct repository *);
 int grep_config(const char *var, const char *value, void *);
 void grep_init(struct grep_opt *, struct repository *repo, const char *prefix);
+void grep_destroy(void);
 void grep_commit_pattern_type(enum grep_pattern_type, struct grep_opt *opt);
 
 void append_grep_pat(struct grep_opt *opt, const char *pat, size_t patlen, const char *origin, int no, enum grep_pat_token t);

2.23.0.rc1

Carlo Marcelo Arenas Belón <[email protected]> writes:

As we already have such an ifdef block here...

	setup_pcre2_as_needed(opt, pat);

	#if defined(USE_LIBPCRE2) && defined(USE_NED_ALLOCATOR)
	static void setup_pcre2_as_needed(struct grep_opt *opt,	const char *pat)
	{
		... the above one ...
	}
	#else
	#define setup_pcre2_as_needed(ignore1, ignore2) /* nothing */
	#endif

	void grep_destroy(void)
	{
		cleanup_pcre2_as_needed();
	}

	#ifdef USE_LIBPCRE2
	static void cleanup_pcre2_as_needed(void)
	{
		pcre2_general_context_free(pcre2_global_context);
	}
	#else
	#define cleanup_pcre2_as_needed() /* nothing */
	#endif

94da9193a6 ("grep: add support for PCRE v2", 2017-06-01) introduced a small memory leak visible with valgrind in t7813.

Complete the creation of a PCRE2 specific variable that was missing from the original change and free the generated table just like it is done for PCRE1.

---
 grep.c | 7 ++++---
 grep.h | 1 +
 2 files changed, 5 insertions(+), 3 deletions(-)

index 8ee982e2e8..ccb6ab38a3 100644
@@ -541,7 +541,6 @@ static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt
 	PCRE2_UCHAR errbuf[256];
 	PCRE2_SIZE erroffset;
 	int options = PCRE2_MULTILINE;
-	const uint8_t *character_tables = NULL;
 	int jitret;
 	int patinforet;
 	size_t jitsizearg;

@@ -557,9 +556,10 @@ static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt
 			if (!pcre2_global_context)
 				BUG("pcre2_global_context uninitialized");
 #endif
-			character_tables = pcre2_maketables(pcre2_global_context);
+			p->pcre2_tables = pcre2_maketables(pcre2_global_context);
 			p->pcre2_compile_context = pcre2_compile_context_create(NULL);
-			pcre2_set_character_tables(p->pcre2_compile_context, character_tables);
+			pcre2_set_character_tables(p->pcre2_compile_context,
+							p->pcre2_tables);
 		}
 		options |= PCRE2_CASELESS;
 	}

@@ -663,6 +663,7 @@ static void free_pcre2_pattern(struct grep_pat *p)
 	pcre2_match_data_free(p->pcre2_match_data);
 	pcre2_jit_stack_free(p->pcre2_jit_stack);
 	pcre2_match_context_free(p->pcre2_match_context);
+	free((void *)p->pcre2_tables);
 }
 #else /* !USE_LIBPCRE2 */
 static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt)

index 526c2db9ef..533165c21a 100644
@@ -96,6 +96,7 @@ struct grep_pat {
 	pcre2_compile_context *pcre2_compile_context;
 	pcre2_match_context *pcre2_match_context;
 	pcre2_jit_stack *pcre2_jit_stack;
+	const uint8_t *pcre2_tables;
 	uint32_t pcre2_jit_on;
 	kwset_t kws;
 	unsigned fixed:1;

2.23.0.rc1

This series is a candidate reroll for cb/pcre2-chartables-leakfix, that hopefully addresses the root cause of the problem reported by Dscho in Windows, where the PCRE2 library wasn't aware of the custom allocator and was returning a pointer created with the system malloc but passing it to NED's free, resulting in a segfault.

The most likely reason why it was triggered by the original leak fix is the layering violation reported by René and that is likely exclusive to PCRE2 (hence why it hasn't been reported with PCRE1). Additional work might be available in a future release of PCRE2 to address that as detailed in an upstream bug[1] report.

Changes since v4 (only in patch 2):

* git log change reverted, still not sure where it will fit better and worst case will leak a few bytes when -P is used.
since the users of this API are doing it indirectly it might be problematic long term though, but luckily since it is most of the tine a NOOP and can be called multiple times might be ok to do it unconditionally
* slightly better looking code

Changes since v3 (mostly in patch 2):

* git log also calls the "destructor" for grep API
* no more "bug" being triggered by make test, sorry René
* hopefully no more crashes in windows (I was expecting at most a BUG)

Future work (other than the needed refactoring explained in the second patch) and adjacent bugs, includes:

* tracking more possible users of the grep API that might need to call grep_destroy()
* completely moving PCRE2 to use NED (as is done with PCRE1 and was proposed on the original patch[2] this is based on
* build on top of the new API so that other work could be shared (for example the chartables that started this whole mess)

or (hopefully not)

* ignore the original leak (maybe with an UNLEAK) as René suggested [3]
* discard this work and just use Dscho's fix (with some improvements, like using xmalloc)

[1] https://bugs.exim.org/show_bug.cgi?id=2429
[2] https://public-inbox.org/git/3397e6797f872aedd18c6d795f4976e1c579514b.1565005867.git.gitgitgadget@gmail.com/
[3] https://public-inbox.org/git/[email protected]/

Carlo Marcelo Arenas Belón (3):
grep: make PCRE1 aware of custom allocator
grep: make PCRE2 aware of custom allocator
grep: avoid leak of chartables in PCRE2

 Makefile       |  2 +-
 builtin/grep.c |  1 +
 grep.c         | 71 +++++++++++++++++++++++++++++++++++++++++++++++---
 grep.h         |  2 ++
 4 files changed, 72 insertions(+), 4 deletions(-)

base-commit: 51cf315870bbb7254ddf06c84fe03b41bc48eebd

2.23.0.rc1

63e7e9d8b6 ("git-grep: Learn PCRE", 2011-05-09) didn't include a way to override the system alocator, and so it is incompatible with USE_NED_ALLOCATOR as reported by Dscho[1] (in similar code from PCRE2)

Make the minimum change possible to ensure this combination is supported by extending grep_init to set the PCRE1 specific functions to the NED versions (using the aliased names though) and therefore making sure all alocations are done inside PCRE1 with the same allocator than the rest of git.

This change might have performance impact (hopefully for the best) and so will be good idea to test it in a platform where NED might have a positive impact (ex: Windows 7)

[1] https://public-inbox.org/git/[email protected]

---
 Makefile |  2 +-
 grep.c   | 10 ++++++++++
 2 files changed, 11 insertions(+), 1 deletion(-)

index bd246f2989..4b384f3759 100644
@@ -1764,7 +1764,7 @@ ifdef NATIVE_CRLF
 endif
 
 ifdef USE_NED_ALLOCATOR
-	COMPAT_CFLAGS += -Icompat/nedmalloc
+	COMPAT_CFLAGS += -DUSE_NED_ALLOCATOR -Icompat/nedmalloc
 	COMPAT_OBJS += compat/nedmalloc/nedmalloc.o
 	OVERRIDE_STRDUP = YesPlease
 endif

index cd952ef5d3..0154998695 100644
@@ -150,12 +150,22 @@ int grep_config(const char *var, const char *value, void *cb)
  * Initialize one instance of grep_opt and copy the
  * default values from the template we read the configuration
  * information in an earlier call to git_config(grep_config).
+ *
+ * If using PCRE make sure that the library is configured
+ * to use the right allocator (ex: NED)
  */
 void grep_init(struct grep_opt *opt, struct repository *repo, const char *prefix)
 {
 	struct grep_opt *def = &grep_defaults;
 	int i;
 
+#ifdef USE_NED_ALLOCATOR
+#ifdef USE_LIBPCRE1
+	pcre_malloc = malloc;
+	pcre_free = free;
+#endif
+#endif
+
 	memset(opt, 0, sizeof(*opt));
 	opt->repo = repo;
 	opt->prefix = prefix;

2.23.0.rc1

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.

---
 builtin/grep.c |  1 +
 grep.c         | 56 +++++++++++++++++++++++++++++++++++++++++++++++++-
 grep.h         |  1 +
 3 files changed, 57 insertions(+), 1 deletion(-)

index 560051784e..e49c20df60 100644
@@ -1145,5 +1145,6 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
 		run_pager(&opt, prefix);
 	clear_pathspec(&pathspec);
 	free_grep_patterns(&opt);
+	grep_destroy();
 	return !hit;
 }

index 1875880f37..526c2db9ef 100644
@@ -189,6 +189,7 @@ struct grep_opt {
 void init_grep_defaults(struct repository *);
 int grep_config(const char *var, const char *value, void *);
 void grep_init(struct grep_opt *, struct repository *repo, const char *prefix);
+void grep_destroy(void);
 void grep_commit_pattern_type(enum grep_pattern_type, struct grep_opt *opt);
 
 void append_grep_pat(struct grep_opt *opt, const char *pat, size_t patlen, const char *origin, int no, enum grep_pat_token t);

2.23.0.rc1

Hi Carlo,

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 --

index ec845141bbb..4242ad0b4ae 100644
@@ -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

I know, and that is why we worked out a v6 RC with Rene than then was pushed to github[0] and validated to work thanks to your integration as detailed in [1], I never got to send an update to the list though because Rene wanted my squashing into his patch to be independent as detailed there and because I assumed that when Junio didn't pull my reroll, it was probably because your tree had a fix already anyway.

the recent discovery that xmalloc wasn't thread safe though, complicates things further and that is why I was expecting to reroll this on top of both ab/pcre-jit-fixes and jk/drop-release-pack-memory (later one already in next) as detailed in [2]

Just to clarify, I think my patch accounts for that (haven't tested that assumption, but will do now that I have a windows box, probably even with mi-alloc) but yes, the only reason why there were references to NEDMALLOC was to isolate the code and make sure the fix was tackling the problem, it was not my intention to do so at the end, specially once we agreed that xmalloc should be used anyway.

IMHO this is probably testing V3 (from pu) and which was hopefully fixed in the last github force push for my branch

ironically only found out about that patch after I got a windows box (running Windows Home though) and had finished testing my own squashed fix[3] that has succeeded being validated in GitHub

apologize for the delays, and will be fine using your squash, mine, the V6 RC (my preference) or dropping this series from pu if that would help clear the ugliness of pu for windows

hopefully this won't be repeated now that I am aware of github's integration and have my own (albeit very slow) windows environment as well.

Carlo

[0] https://github.com/git/git/commit/0ca5d0550c17a68d83b8922b71aeff891958ed0e
[1] https://public-inbox.org/git/CAPUEspiFuvgMQ3W1se1B=8aTTrQsJSZTyQTzG1TpiyN8HTOpkA@mail.gmail.com/
[2] https://public-inbox.org/git/CAPUEspg9F7RutCUCoRAAXmRePjiunq3-zG7cN3uz_t5DVMxP=g@mail.gmail.com/
[3] https://github.com/git/git/pull/627

So,... have we seen any conclusion on this? Can any of you guys give us a pointer to or copies of the candidate to be the final solution of this topic, please?

Thanks.

Hi Junio,

I still need
https://github.com/git-for-windows/git/commit/719beb813e4f27f090696ce583df3e5f3c480545
and
https://github.com/git-for-windows/git/commit/3369c322bbd95820b971701fef7db44b26dd826f
to fix that part in Git for Windows' shears/pu branch (i.e. the continuously rebased patch thicket).

Ciao,
Dscho

or we could drop the current branch in pu and start again from scratch now that all of the required dependencies had been merged.

apologize for the delays otherwise; $DAYJOB has taken a toll lately and even my new shiny windows dev box hasn't seen much action.

will update here and in github shortly (which might mean up to this weekend, being realistic), but should be better code (since it is mostly Rene's) and better tested that way and hopefully won't cause more breakage (specially in Windows, sorry Dscho)

Carlo

Hi Carlo,

I hope that your next iteration won't have any #ifdef NED in it?

I appreciate all your work!

Thanks,
Dscho

That would be the cleanest from my point of view. Thanks, both.

94da9193a6 ("grep: add support for PCRE v2", 2017-06-01) introduced a small memory leak visible with valgrind in t7813.

Complete the creation of a PCRE2 specific variable that was missing from the original change and free the generated table just like it is done for PCRE1.

---
 grep.c | 7 ++++---
 grep.h | 1 +
 2 files changed, 5 insertions(+), 3 deletions(-)

index 8e0b838db0..146891e2bf 100644
@@ -543,7 +543,6 @@ static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt
 	PCRE2_UCHAR errbuf[256];
 	PCRE2_SIZE erroffset;
 	int options = PCRE2_MULTILINE;
-	const uint8_t *character_tables = NULL;
 	int jitret;
 	int patinforet;
 	size_t jitsizearg;

@@ -563,9 +562,10 @@ static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt
 			if (!pcre2_global_context)
 				BUG("pcre2_global_context uninitialized");
 #endif
-			character_tables = pcre2_maketables(pcre2_global_context);
+			p->pcre2_tables = pcre2_maketables(pcre2_global_context);
 			p->pcre2_compile_context = pcre2_compile_context_create(NULL);
-			pcre2_set_character_tables(p->pcre2_compile_context, character_tables);
+			pcre2_set_character_tables(p->pcre2_compile_context,
+							p->pcre2_tables);
 		}
 		options |= PCRE2_CASELESS;
 	}

@@ -669,6 +669,7 @@ static void free_pcre2_pattern(struct grep_pat *p)
 	pcre2_match_data_free(p->pcre2_match_data);
 	pcre2_jit_stack_free(p->pcre2_jit_stack);
 	pcre2_match_context_free(p->pcre2_match_context);
+	free((void *)p->pcre2_tables);
 }
 #else /* !USE_LIBPCRE2 */
 static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt)

index 526c2db9ef..533165c21a 100644
@@ -96,6 +96,7 @@ struct grep_pat {
 	pcre2_compile_context *pcre2_compile_context;
 	pcre2_match_context *pcre2_match_context;
 	pcre2_jit_stack *pcre2_jit_stack;
+	const uint8_t *pcre2_tables;
 	uint32_t pcre2_jit_on;
 	kwset_t kws;
 	unsigned fixed:1;

2.23.0.rc1

disregard this version, it still broken (and wouldn't even build on some cases), the reasons why are still unclear though but at least it might
seem from the last known run in windows that segfaults were prevented at last and something was still off enough to trigger a BUG (shouldn't be a concern with V6 or later that do PCRE2 migration to NED fully, as agreed)

Thanks to Dscho's github integration to Azure pipelines got finally a RC[1] for V6 that at least passes all tests and will need to get in shape for submission.

as René pointed out in three my performance concerns might be a red herring as well, but would be nice if someone with windows would definitely get some numbers so we can't be sure there were truly no regressions

Carlo

[1] https://github.com/carenas/git/tree/pcre2-chartables-leakfix

Am 09.08.19 um 13:24 schrieb Carlo Arenas:

So how about starting stupidly simple? You can test it everywhere, it just needs libpcre2. It works without that library as well (tested with "make USE_LIBPCRE= USE_LIBPCRE2= test"), but doesn't allocate anything in that case, of course. The character tables leak fix should be safe on top. If you detect performance issues then we can address them in additional patches.

-- >8 --
Subject: [PATCH] grep: use xmalloc() for all PCRE2 allocations

Build a PCRE2 global custom context when compiling a pattern and use it to tell the library to use xmalloc() for allocations. This provides consistent out-of-memory handling and makes sure it uses a custom allocator, e.g. with USE_NED_ALLOCATOR.

---
The function names are ridiculously long, I tried to stay within 80 characters per line but gave up in the end and just kept going without line breaks. Fits the "stupidly simple" approach..

 grep.c | 23 +++++++++++++++++------
 grep.h |  2 ++
 2 files changed, 19 insertions(+), 6 deletions(-)

index cd952ef5d3..44f4e38657 100644
@@ -482,6 +482,16 @@ static void free_pcre1_regexp(struct grep_pat *p)
 #endif /* !USE_LIBPCRE1 */
 
 #ifdef USE_LIBPCRE2
+static void *pcre2_malloc(PCRE2_SIZE size, MAYBE_UNUSED void *memory_data)
+{
+	return xmalloc(size);
+}
+
+static void pcre2_free(void *pointer, MAYBE_UNUSED void *memory_data)
+{
+	free(pointer);
+}
+
 static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt)
 {
 	int error;

@@ -495,12 +505,12 @@ static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt
 
 	assert(opt->pcre2);
 
-	p->pcre2_compile_context = NULL;
+	p->pcre2_general_context = pcre2_general_context_create(pcre2_malloc, pcre2_free, NULL);
+	p->pcre2_compile_context = pcre2_compile_context_create(p->pcre2_general_context);
 
 	if (opt->ignore_case) {
 		if (has_non_ascii(p->pattern)) {
-			character_tables = pcre2_maketables(NULL);
-			p->pcre2_compile_context = pcre2_compile_context_create(NULL);
+			character_tables = pcre2_maketables(p->pcre2_general_context);
 			pcre2_set_character_tables(p->pcre2_compile_context, character_tables);
 		}
 		options |= PCRE2_CASELESS;

@@ -513,7 +523,7 @@ static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt
 					 p->pcre2_compile_context);
 
 	if (p->pcre2_pattern) {
-		p->pcre2_match_data = pcre2_match_data_create_from_pattern(p->pcre2_pattern, NULL);
+		p->pcre2_match_data = pcre2_match_data_create_from_pattern(p->pcre2_pattern, p->pcre2_general_context);
 		if (!p->pcre2_match_data)
 			die("Couldn't allocate PCRE2 match data");
 	} else {

@@ -550,10 +560,10 @@ static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt
 			return;
 		}
 
-		p->pcre2_jit_stack = pcre2_jit_stack_create(1, 1024 * 1024, NULL);
+		p->pcre2_jit_stack = pcre2_jit_stack_create(1, 1024 * 1024, p->pcre2_general_context);
 		if (!p->pcre2_jit_stack)
 			die("Couldn't allocate PCRE2 JIT stack");
-		p->pcre2_match_context = pcre2_match_context_create(NULL);
+		p->pcre2_match_context = pcre2_match_context_create(p->pcre2_general_context);
 		if (!p->pcre2_match_context)
 			die("Couldn't allocate PCRE2 match context");
 		pcre2_jit_stack_assign(p->pcre2_match_context, NULL, p->pcre2_jit_stack);

@@ -605,6 +615,7 @@ static void free_pcre2_pattern(struct grep_pat *p)
 	pcre2_match_data_free(p->pcre2_match_data);
 	pcre2_jit_stack_free(p->pcre2_jit_stack);
 	pcre2_match_context_free(p->pcre2_match_context);
+	pcre2_general_context_free(p->pcre2_general_context);
 }
 #else /* !USE_LIBPCRE2 */
 static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt)

index 1875880f37..73b8b87a3a 100644
@@ -28,6 +28,7 @@ typedef int pcre_jit_stack;
 #else
 typedef int pcre2_code;
 typedef int pcre2_match_data;
+typedef int pcre2_general_context;
 typedef int pcre2_compile_context;
 typedef int pcre2_match_context;
 typedef int pcre2_jit_stack;

@@ -93,6 +94,7 @@ struct grep_pat {
 	int pcre1_jit_on;
 	pcre2_code *pcre2_pattern;
 	pcre2_match_data *pcre2_match_data;
+	pcre2_general_context *pcre2_general_context;
 	pcre2_compile_context *pcre2_compile_context;
 	pcre2_match_context *pcre2_match_context;
 	pcre2_jit_stack *pcre2_jit_stack;

--
2.22.0

;-) Thanks for keeping the conversation going.

Hi,

FWIW I am very much in favor of this approach.

Thanks,
Dscho

Make using a general context (that is only needed with NED) to depend on NED being selected at compile time.

the compile_context could be also make conditional but it gets ugly really fasts with #ifdef

---
 Makefile | 2 +-
 grep.c   | 4 ++++
 2 files changed, 5 insertions(+), 1 deletion(-)

index 8a7e235352..995e6c9351 100644
@@ -1774,7 +1774,7 @@ ifdef NATIVE_CRLF
 endif
 
 ifdef USE_NED_ALLOCATOR
-	COMPAT_CFLAGS += -Icompat/nedmalloc
+	COMPAT_CFLAGS += -DUSE_NED_ALLOCATOR -Icompat/nedmalloc
 	COMPAT_OBJS += compat/nedmalloc/nedmalloc.o
 	OVERRIDE_STRDUP = YesPlease
 endif

index 8255ec956e..233072ed80 100644
@@ -482,6 +482,7 @@ static void free_pcre1_regexp(struct grep_pat *p)
 #endif /* !USE_LIBPCRE1 */
 
 #ifdef USE_LIBPCRE2
+#ifdef USE_NED_ALLOCATOR
 static void *pcre2_malloc(PCRE2_SIZE size, MAYBE_UNUSED void *memory_data)
 {
 	return xmalloc(size);

@@ -491,6 +492,7 @@ static void pcre2_free(void *pointer, MAYBE_UNUSED void *memory_data)
 {
 	free(pointer);
 }
+#endif
 
 static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt)
 {

@@ -505,7 +507,9 @@ static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt
 
 	assert(opt->pcre2);
 
+#ifdef USE_NED_ALLOCATOR
 	p->pcre2_general_context = pcre2_general_context_create(pcre2_malloc, pcre2_free, NULL);
+#endif
 	p->pcre2_compile_context = pcre2_compile_context_create(p->pcre2_general_context);
 
 	if (opt->ignore_case) {

2.23.0.rc2

Am 10.08.19 um 05:03 schrieb Carlo Marcelo Arenas Belón:

A custom general context is needed to convince PCRE2 to use xmalloc() instead of mallo(). That is independent of the choice of allocator. So this change would be a step backwards?

René

My bad, you are correct.

Do you mind then if I "adopt" your patch and submit a reroll with it, will also add an "equivalent" one to fix PCRE1 as well then, and we will tackle any performance deficiencies or other issues in a future series.

Carlo

Am 10.08.19 um 10:42 schrieb Carlo Arenas:

I don't mind, sounds good.

René

I had to squash another fix that was reported[1] before but wasn't picked up and that ironically might explain the last segfaults from my old V4

Carlo

[1] https://public-inbox.org/git/[email protected]/

Am 12.08.19 um 09:35 schrieb Carlo Arenas:

That looks like an issue worth its own commit.

I wonder if we'd want to pass pcre2_match_context to pcre2_match() a few lines down as well, for consistency.

René

OK, but then will make my topic interesting; indeed it almost feels like it should be 3 different topics all depending of each other in a chain:

* really use the match context
* move to xmalloc (2 patches, one for each PCRE library)
* fix leak

Which then will conflict with ab/pcre-jit-fixes before it can get merged to pu

yes, that is actually what was tested[1] as can be seen in github where Dscho integration did most of the heavy lifting with Windows

Carlo

[1] https://github.com/git/git/pull/627

Hi Carlo,

FWIW this is (almost) exactly what I had in mind with my first reply, except I wanted to make setting of the global context be conditioned to having NED enabled to avoid possible performance issues in environments were NED is not even usable.

in macOS (obviously testing without NED) the following is the output of (a hacked version) of p7801 for maint (against chromium's repository), with René's patch on top

Test HEAD^ HEAD
--------------------------------------------------------------------------------------
7820.1: perl grep 'how.to' 0.51(0.35+1.11)
0.48(0.33+1.16) -5.9%
7820.2: perl grep '^how to' 0.47(0.33+1.08)
0.45(0.34+1.11) -4.3%
7820.3: perl grep '[how] to' 0.49(0.40+1.11)
0.53(0.41+1.13) +8.2%
7820.4: perl grep '(e.t[^ ]*|v.ry) rare' 68.72(68.77+1.14)
72.10(72.15+1.20) +4.9%
7820.5: perl grep 'm(ú|u)lt.b(æ|y)te' 0.48(0.35+1.12)
0.50(0.40+1.23) +4.2%

and this is with my squashed[2] changed on top of that :

Test HEAD^ HEAD
--------------------------------------------------------------------------------------
7820.1: perl grep 'how.to' 0.48(0.36+1.16)
0.46(0.33+1.09) -4.2%
7820.2: perl grep '^how to' 0.45(0.34+1.12)
0.42(0.29+0.99) -6.7%
7820.3: perl grep '[how] to' 0.48(0.40+1.13)
0.52(0.43+1.16) +8.3%
7820.4: perl grep '(e.t[^ ]*|v.ry) rare' 69.12(69.10+1.07)
69.19(69.19+1.18) +0.1%
7820.5: perl grep 'm(ú|u)lt.b(æ|y)te' 0.49(0.38+1.17)
0.46(0.35+1.13) -6.1%

the degenerate case is not something we can't fix anyway, since it is likely a locking issue inside PCRE2 (I see at most 1 CPU doing work), and the numbers are noisy because of the other problems I mentioned before (hardcoded to 8 threads, running in a laptop with low number of cores), which is why testing for performance regressions in windows is strongly encouraged, regardless

Carlo

[1] https://public-inbox.org/git/CAPUEspgH1v1zo7smzQWCV4rX9pKVKLV84gDSfCPdT7LffQxUWw@mail.gmail.com/
[2] https://public-inbox.org/git/[email protected]/

Am 10.08.19 um 05:05 schrieb Carlo Arenas:

Do you mean p7820? And what did you change? Looking at the results you removed basic and extended from the list of regex engines, right?

Ugh, cloning https://chromium.googlesource.com/chromium/src.git sends more than 16GB across the wire. Is that even the right repository? Not sure if my machine can keep the relevant parts cached while grepping -- I/O times could drown out any difference due to context allocation and memory allocator choice. Let's see...

So I pointed GIT_PERF_LARGE_REPO to the monster mentioned above, ran the test once for warmup and here are the results of the second run:

Test origin/master pcre2-xmalloc pcre2-xmalloc+nedmalloc
---------------------------------------------------------------------------------------------------------------------

7820.1: basic grep 'how.to'                     1.59(2.93+1.75)     1.60(3.04+1.74) +0.6%     1.64(2.87+1.90) +3.1%
7820.2: extended grep 'how.to'                  1.59(2.98+1.66)     1.55(2.83+1.76) -2.5%     1.67(3.15+1.70) +5.0%
7820.3: perl grep 'how.to'                      1.25(1.21+2.13)     1.25(1.24+2.08) +0.0%     1.29(1.32+2.08) +3.2%
7820.5: basic grep '^how to'                    1.52(2.82+1.66)     1.51(2.68+1.77) -0.7%     1.64(3.07+1.69) +7.9%
7820.6: extended grep '^how to'                 1.57(2.84+1.75)     1.51(2.76+1.73) -3.8%     1.61(2.95+1.75) +2.5%
7820.7: perl grep '^how to'                     1.21(1.15+2.10)     1.22(1.26+1.98) +0.8%     1.27(1.22+2.09) +5.0%
7820.9: basic grep '[how] to'                   1.95(4.51+1.68)     1.96(4.48+1.69) +0.5%     2.00(4.66+1.65) +2.6%
7820.10: extended grep '[how] to'               1.96(4.54+1.65)     1.94(4.46+1.70) -1.0%     2.04(4.78+1.65) +4.1%
7820.11: perl grep '[how] to'                   1.29(1.58+1.88)     1.28(1.50+1.94) -0.8%     1.34(1.51+2.06) +3.9%
7820.13: basic grep '\(e.t[^ ]*\|v.ry\) rare'   8.17(13.18+1.50)    8.29(13.36+1.37) +1.5%    8.31(13.33+1.60) +1.7%
7820.14: extended grep '(e.t[^ ]*|v.ry) rare'   8.13(13.03+1.59)    8.14(13.12+1.47) +0.1%    8.30(13.35+1.56) +2.1%
7820.15: perl grep '(e.t[^ ]*|v.ry) rare'       34.96(35.80+1.68)   34.99(35.60+1.91) +0.1%   35.18(35.83+1.90) +0.6%
7820.17: basic grep 'm\(ú\|u\)lt.b\(æ\|y\)te'   1.57(3.03+1.64)     1.53(2.76+1.75) -2.5%     1.60(2.89+1.77) +1.9%
7820.18: extended grep 'm(ú|u)lt.b(æ|y)te'      1.52(2.83+1.69)     1.52(2.89+1.63) +0.0%     1.58(2.80+1.84) +3.9%
7820.19: perl grep 'm(ú|u)lt.b(æ|y)te'          1.20(1.25+2.02)     1.21(1.30+1.96) +0.8%     1.25(1.22+2.11) +4.2%

pcre2-xmalloc is my patch on top of master, +nedmalloc has the warning fixes I sent earlier and sets USE_NED_MALLOC.

I don't understand why my performance is lower by factor 2.5 than yours for all perl regex tests except 7820.15 (your 7820.4), where my system is two times faster. Debian Testing, GCC 9.1.0, i5-9600K, 16GB RAM.

Anyway, nedmalloc is slower across the board, but the impact of my patch is in the noise. Right?

René

correct, that was a weird typo there; apologize for the confusion. you were correct about my changes; ironically, I didn't spell those changes out to avoid confusion.

noticed it was mentioned before by people[1] doing performance testing on grep specifically and
thought it was better than try to come with another one, the linux kernel wouldn't work in macOS
because it breaks "run" as it fails to checkout in a case insensitive filesystem.

;), try setting /proc/sys/vm/swappiness to 0 or get more RAM

interesting; did you also see at most 100% of one CPU being used? yours seem to be faster than mine so this might be representative of single threaded performance.

yes, and there is a lot of noise.

Carlo

[1] https://public-inbox.org/git/e72330c6747218545cce1b6b1edfd1e448141a8f.1563570204.git.matheus.bernardino@usp.br/

Am 10.08.19 um 14:40 schrieb Carlo Arenas:

For 7820.15 yes -- 100% of a single core, the rest idling. The other tests had only brief sequential phases, if at all, and used all cores.

René

Hi Carlo,

Well, this is disappointing.

I worked several weeks on getting Azure Pipelines support in shape, so that you can now open PRs against:

- https://github.com/git/git

- https://github.com/gitgitgadget/git

- https://github.com/git-for-windows/git

to get Windows/macOS/Linux testing for free.

So I guess you'd like fries with that, extra large ones, with extra pepperoni seasoning?

Ciao,
Dscho

Apologies

Thanks, I owe you a drink

https://dev.azure.com/git/git/_build/results?buildId=862

Carlo

PS. I might had broken something here as I can't see the test results that failed
https://dev.azure.com/git/git/_build/results?buildId=857&view=ms.vss-test-web.build-test-results-tab

Hi Carlo,

The test results load dynamically, so you'll probably just have to wait for a couple moments. After that, just click on the lines indicating the failed test cases to pop up a verbose trace of it.

Ciao,
Dscho