{"thread":{"id":"52046","subject":"[PATCH 2/3] grep: make PCRE2 aware of custom allocator","startedAt":"2019-10-16T12:06:59Z","lastAt":"2021-03-04T01:09:44Z","messageCount":42,"participants":["Carlo Marcelo Arenas Belón via GitGitGadget","Johannes Schindelin via GitGitGadget","Junio C Hamano","Ævar Arnfjörð Bjarmason","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"384176","messageId":"191d3a2280232ff98964fd42bfe0bc85ee3708f5.1571227613.git.gitgitgadget@gmail.com","threadId":"52046","inReplyTo":"pull.402.git.1571227613.gitgitgadget@gmail.com","subject":"[PATCH 2/3] grep: make PCRE2 aware of custom allocator","fromName":"Carlo Marcelo Arenas Belón via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-10-16T12:06:52Z","receivedAt":"2019-10-16T12:06:59Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"From: =?UTF-8?q?Carlo=20Marcelo=20Arenas=20Bel=C3=B3n?= \n\n94da9193a6 (grep: add support for PCRE v2, 2017-06-01) didn't include\na way to override the system allocator, and so it is incompatible with\ncustom allocators (e.g. nedmalloc). This problem became obvious when we\ntried to plug a memory leak by `free()`ing a data structure allocated by\nPCRE2, triggering a segfault in Windows (where we use nedmalloc by\ndefault).\n\nPCRE2 requires the use of a general context to override the allocator\nand therefore, there is a lot more code needed than in PCRE1, including\na couple of wrapper functions.\n\nExtend the grep API with a \"destructor\" that could be called to cleanup\nany objects that were created and used globally.\n\nUpdate `builtin/grep.c` to use that new API, but any other future users\nshould make sure to have matching `grep_init()`/`grep_destroy()` calls\nif they are using the pattern matching functionality.\n\nMove some of the logic that was before done per thread (in the workers)\ninto an earlier phase to avoid degrading performance, but as the use of\nPCRE2 with custom allocators is better understood it is expected more of\nits functions will be instructed to use the custom allocator as well as\nwas done in the original code[1] this work was based on.\n\n[1] https://public-inbox.org/git/3397e6797f872aedd18c6d795f4976e1c579514b.1565005867.git.gitgitgadget@gmail.com/\n\nReported-by: Johannes Schindelin \nSigned-off-by: Carlo Marcelo Arenas Belón \nSigned-off-by: Johannes Schindelin \n---\n builtin/grep.c | 1 +\n grep.c | 34 +++++++++++++++++++++++++++++++++-\n grep.h | 1 +\n 3 files changed, 35 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex 560051784e..e49c20df60 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -1145,5 +1145,6 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n \t\trun_pager(&opt, prefix);\n \tclear_pathspec(&pathspec);\n \tfree_grep_patterns(&opt);\n+\tgrep_destroy();\n \treturn !hit;\n }\ndiff --git a/grep.c b/grep.c\nindex db6e0d895f..296edbb56f 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -16,6 +16,20 @@ static int grep_source_is_binary(struct grep_source *gs,\n \n static struct grep_opt grep_defaults;\n \n+#ifdef USE_LIBPCRE2\n+static pcre2_general_context *pcre2_global_context;\n+\n+static void *pcre2_malloc(PCRE2_SIZE size, MAYBE_UNUSED void *memory_data)\n+{\n+\treturn malloc(size);\n+}\n+\n+static void pcre2_free(void *pointer, MAYBE_UNUSED void *memory_data)\n+{\n+\treturn free(pointer);\n+}\n+#endif\n+\n static const char *color_grep_slots[] = {\n \t[GREP_COLOR_CONTEXT]\t = \"context\",\n \t[GREP_COLOR_FILENAME]\t = \"filename\",\n@@ -153,12 +167,20 @@ int grep_config(const char *var, const char *value, void *cb)\n *\n * If using PCRE, make sure that the library is configured\n * to use the same allocator as Git (e.g. nedmalloc on Windows).\n+ *\n+ * Any allocated memory needs to be released in grep_destroy().\n */\n void grep_init(struct grep_opt *opt, struct repository *repo, const char *prefix)\n {\n \tstruct grep_opt *def = &grep_defaults;\n \tint i;\n \n+#if defined(USE_LIBPCRE2)\n+\tif (!pcre2_global_context)\n+\t\tpcre2_global_context = pcre2_general_context_create(\n+\t\t\t\t\tpcre2_malloc, pcre2_free, NULL);\n+#endif\n+\n #ifdef USE_LIBPCRE1\n \tpcre_malloc = malloc;\n \tpcre_free = free;\n@@ -186,6 +208,13 @@ void grep_init(struct grep_opt *opt, struct repository *repo, const char *prefix\n \t\tcolor_set(opt->colors[i], def->colors[i]);\n }\n \n+void grep_destroy(void)\n+{\n+#ifdef USE_LIBPCRE2\n+\tpcre2_general_context_free(pcre2_global_context);\n+#endif\n+}\n+\n static void grep_set_pattern_type_option(enum grep_pattern_type pattern_type, struct grep_opt *opt)\n {\n \t/*\n@@ -505,9 +534,12 @@ static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt\n \n \tp->pcre2_compile_context = NULL;\n \n+\t/* pcre2_global_context is initialized in append_grep_pattern */\n \tif (opt->ignore_case) {\n \t\tif (has_non_ascii(p->pattern)) {\n-\t\t\tcharacter_tables = pcre2_maketables(NULL);\n+\t\t\tif (!pcre2_global_context)\n+\t\t\t\tBUG(\"pcre2_global_context uninitialized\");\n+\t\t\tcharacter_tables = pcre2_maketables(pcre2_global_context);\n \t\t\tp->pcre2_compile_context = pcre2_compile_context_create(NULL);\n \t\t\tpcre2_set_character_tables(p->pcre2_compile_context, character_tables);\n \t\t}\ndiff --git a/grep.h b/grep.h\nindex 1875880f37..526c2db9ef 100644\n--- a/grep.h\n+++ b/grep.h\n@@ -189,6 +189,7 @@ struct grep_opt {\n void init_grep_defaults(struct repository *);\n int grep_config(const char *var, const char *value, void *);\n void grep_init(struct grep_opt *, struct repository *repo, const char *prefix);\n+void grep_destroy(void);\n void grep_commit_pattern_type(enum grep_pattern_type, struct grep_opt *opt);\n \n void append_grep_pat(struct grep_opt *opt, const char *pat, size_t patlen, const char *origin, int no, enum grep_pat_token t);\n-- \ngitgitgadget\n\n"},{"id":"384177","messageId":"4feb8cc83a6d0b73bd7e47a513b91ce193c6ed6f.1571227613.git.gitgitgadget@gmail.com","threadId":"52046","inReplyTo":"pull.402.git.1571227613.gitgitgadget@gmail.com","subject":"[PATCH 1/3] grep: make PCRE1 aware of custom allocator","fromName":"Carlo Marcelo Arenas Belón via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-10-16T12:06:51Z","receivedAt":"2019-10-16T12:07:00Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"From: =?UTF-8?q?Carlo=20Marcelo=20Arenas=20Bel=C3=B3n?= \n\n63e7e9d8b6 (\"git-grep: Learn PCRE\", 2011-05-09) didn't include a way\nto override the system alocator, and so it is incompatible with\nUSE_NED_ALLOCATOR as reported by Dscho[1] (in similar code from PCRE2)\n\nNote that nedmalloc, as well as other custom allocators like jemalloc\nand mi-malloc, can be configured at runtime (via `LD_PRELOAD`),\ntherefore we cannot know at compile time whether a custom allocator is\nused or not.\n\nMake the minimum change possible to ensure this combination is supported\nby extending `grep_init()` to set the PCRE1 specific functions to Git's\nidea of `malloc()` and `free()` and therefore making sure all\nallocations are done inside PCRE1 with the same allocator than the rest\nof Git.\n\nThis change has negligible performance impact: PCRE needs to allocate\nmemory once per program run for the character table and for each pattern\ncompilation. These are both rare events compared to matching patterns\nagainst lines. Actual measurements[2] show that the impact is lost in\nthe noise.\n\n[1] https://public-inbox.org/git/pull.306.git.gitgitgadget@gmail.com\n[2] https://public-inbox.org/git/7f42007f-911b-c570-17f6-1c6af0429586@web.de\n\nSigned-off-by: Carlo Marcelo Arenas Belón \nSigned-off-by: Johannes Schindelin \n---\n grep.c | 8 ++++++++\n 1 file changed, 8 insertions(+)\n\ndiff --git a/grep.c b/grep.c\nindex cd952ef5d3..db6e0d895f 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -150,12 +150,20 @@ int grep_config(const char *var, const char *value, void *cb)\n * Initialize one instance of grep_opt and copy the\n * default values from the template we read the configuration\n * information in an earlier call to git_config(grep_config).\n+ *\n+ * If using PCRE, make sure that the library is configured\n+ * to use the same allocator as Git (e.g. nedmalloc on Windows).\n */\n void grep_init(struct grep_opt *opt, struct repository *repo, const char *prefix)\n {\n \tstruct grep_opt *def = &grep_defaults;\n \tint i;\n \n+#ifdef USE_LIBPCRE1\n+\tpcre_malloc = malloc;\n+\tpcre_free = free;\n+#endif\n+\n \tmemset(opt, 0, sizeof(*opt));\n \topt->repo = repo;\n \topt->prefix = prefix;\n-- \ngitgitgadget\n\n"},{"id":"384178","messageId":"pull.402.git.1571227613.gitgitgadget@gmail.com","threadId":"52046","inReplyTo":null,"subject":"[PATCH 0/3] Revive 'pcre2-chartables-leakfix'","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-10-16T12:06:50Z","receivedAt":"2019-10-16T12:07:01Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"As I had stated several times, I was really unhappy how the original fix\nharped on nedmalloc and totally ignored runtime-configured custom\nallocators.\n\nSo this is, at long last, my attempt to give this a new life. It is based\noff of maint and needs trivial merge conflict resolutions relative to master\n.\n\nRange-diff relative to cb/pcre2-chartables-leakfix:\n\n1: 770584d697e ! 1: 4feb8cc83a6 grep: make PCRE1 aware of custom allocator\n @@ Commit message\n to override the system alocator, and so it is incompatible with\n USE_NED_ALLOCATOR as reported by Dscho[1] (in similar code from PCRE2)\n\n + Note that nedmalloc, as well as other custom allocators like jemalloc\n + and mi-malloc, can be configured at runtime (via `LD_PRELOAD`),\n + therefore we cannot know at compile time whether a custom allocator is\n + used or not.\n +\n Make the minimum change possible to ensure this combination is supported\n - by extending grep_init to set the PCRE1 specific functions to the NED\n - versions (using the aliased names though) and therefore making sure all\n - alocations are done inside PCRE1 with the same allocator than the rest\n - of git.\n + by extending `grep_init()` to set the PCRE1 specific functions to Git's\n + idea of `malloc()` and `free()` and therefore making sure all\n + allocations are done inside PCRE1 with the same allocator than the rest\n + of Git.\n\n - This change might have performance impact (hopefully for the best) and\n - so will be good idea to test it in a platform where NED might have a\n - positive impact (ex: Windows 7)\n + This change has negligible performance impact: PCRE needs to allocate\n + memory once per program run for the character table and for each pattern\n + compilation. These are both rare events compared to matching patterns\n + against lines. Actual measurements[2] show that the impact is lost in\n + the noise.\n\n [1] https://public-inbox.org/git/pull.306.git.gitgitgadget@gmail.com\n + [2] https://public-inbox.org/git/7f42007f-911b-c570-17f6-1c6af0429586@web.de\n\n Signed-off-by: Carlo Marcelo Arenas Bel├│n \n - Signed-off-by: Junio C Hamano \n -\n - ## Makefile ##\n -@@ Makefile: ifdef NATIVE_CRLF\n - endif\n - \n - ifdef USE_NED_ALLOCATOR\n -- COMPAT_CFLAGS += -Icompat/nedmalloc\n -+ COMPAT_CFLAGS += -DUSE_NED_ALLOCATOR -Icompat/nedmalloc\n - COMPAT_OBJS += compat/nedmalloc/nedmalloc.o\n - OVERRIDE_STRDUP = YesPlease\n - endif\n + Signed-off-by: Johannes Schindelin \n\n ## grep.c ##\n @@ grep.c: int grep_config(const char *var, const char *value, void *cb)\n @@ grep.c: int grep_config(const char *var, const char *value, void *cb)\n * default values from the template we read the configuration\n * information in an earlier call to git_config(grep_config).\n + *\n -+ * If using PCRE make sure that the library is configured\n -+ * to use the right allocator (ex: NED)\n ++ * If using PCRE, make sure that the library is configured\n ++ * to use the same allocator as Git (e.g. nedmalloc on Windows).\n */\n void grep_init(struct grep_opt *opt, struct repository *repo, const char *prefix)\n {\n struct grep_opt *def = &grep_defaults;\n int i;\n\n -+#ifdef USE_NED_ALLOCATOR\n +#ifdef USE_LIBPCRE1\n + pcre_malloc = malloc;\n + pcre_free = free;\n +#endif\n -+#endif\n +\n memset(opt, 0, sizeof(*opt));\n opt->repo = repo;\n2: cf8d36f34a2 ! 2: 39ad220c18b grep: make PCRE2 aware of custom allocator\n @@\n ## Metadata ##\n -Author: Carlo Marcelo Arenas Bel├│n \n +Author: Johannes Schindelin \n\n ## Commit message ##\n grep: make PCRE2 aware of custom allocator\n\n 94da9193a6 (grep: add support for PCRE v2, 2017-06-01) didn't include\n a way to override the system allocator, and so it is incompatible with\n - USE_NED_ALLOCATOR. The problem was made visible when an attempt to\n - avoid a leak in a data structure that is created by the library was\n - passed to NED's free for disposal triggering a segfault in Windows.\n + custom allocators (e.g. nedmalloc). This problem became obvious when we\n + tried to plug a memory leak by `free()`ing a data structure allocated by\n + PCRE2, triggering a segfault in Windows (where we use nedmalloc by\n + default).\n\n PCRE2 requires the use of a general context to override the allocator\n and therefore, there is a lot more code needed than in PCRE1, including\n @@ Commit message\n Extend the grep API with a \"destructor\" that could be called to cleanup\n any objects that were created and used globally.\n\n - Update builtin/grep to use that new API, but any other future\n - users should make sure to have matching grep_init/grep_destroy calls\n - if they are using the pattern matching functionality (currently only\n - relevant when using both NED and PCRE2)\n + Update `builtin/grep.c` to use that new API, but any other future users\n + should make sure to have matching `grep_init()`/`grep_destroy()` calls\n + if they are using the pattern matching functionality.\n\n Move some of the logic that was before done per thread (in the workers)\n - into an earlier phase to avoid degrading performance, but as the use\n - of PCRE2 with NED is better understood it is expected more of its\n - functions will be instructed to use the custom allocator as well as\n + into an earlier phase to avoid degrading performance, but as the use of\n + PCRE2 with custom allocators is better understood it is expected more of\n + its functions will be instructed to use the custom allocator as well as\n was done in the original code[1] this work was based on.\n\n [1] https://public-inbox.org/git/3397e6797f872aedd18c6d795f4976e1c579514b.1565005867.git.gitgitgadget@gmail.com/\n\n Reported-by: Johannes Schindelin \n Signed-off-by: Carlo Marcelo Arenas Bel├│n \n - Signed-off-by: Junio C Hamano \n + Signed-off-by: Johannes Schindelin \n\n ## builtin/grep.c ##\n @@ builtin/grep.c: int cmd_grep(int argc, const char **argv, const char *prefix)\n @@ grep.c: static int grep_source_is_binary(struct grep_source *gs,\n +#ifdef USE_LIBPCRE2\n +static pcre2_general_context *pcre2_global_context;\n +\n -+#ifdef USE_NED_ALLOCATOR\n +static void *pcre2_malloc(PCRE2_SIZE size, MAYBE_UNUSED void *memory_data)\n +{\n + return malloc(size);\n @@ grep.c: static int grep_source_is_binary(struct grep_source *gs,\n + return free(pointer);\n +}\n +#endif\n -+#endif\n +\n static const char *color_grep_slots[] = {\n [GREP_COLOR_CONTEXT] = \"context\",\n [GREP_COLOR_FILENAME] = \"filename\",\n @@ grep.c: int grep_config(const char *var, const char *value, void *cb)\n *\n - * If using PCRE make sure that the library is configured\n - * to use the right allocator (ex: NED)\n -+ * if any object is created it should be cleaned up in grep_destroy()\n + * If using PCRE, make sure that the library is configured\n + * to use the same allocator as Git (e.g. nedmalloc on Windows).\n ++ *\n ++ * Any allocated memory needs to be released in grep_destroy().\n */\n void grep_init(struct grep_opt *opt, struct repository *repo, const char *prefix)\n {\n + struct grep_opt *def = &grep_defaults;\n + int i;\n + \n ++#if defined(USE_LIBPCRE2)\n ++ if (!pcre2_global_context)\n ++ pcre2_global_context = pcre2_general_context_create(\n ++ pcre2_malloc, pcre2_free, NULL);\n ++#endif\n ++\n + #ifdef USE_LIBPCRE1\n + pcre_malloc = malloc;\n + pcre_free = free;\n @@ grep.c: void grep_init(struct grep_opt *opt, struct repository *repo, const char *prefix\n color_set(opt->colors[i], def->colors[i]);\n }\n @@ grep.c: void grep_init(struct grep_opt *opt, struct repository *repo, const char\n static void grep_set_pattern_type_option(enum grep_pattern_type pattern_type, struct grep_opt *opt)\n {\n /*\n -@@ grep.c: void append_header_grep_pattern(struct grep_opt *opt,\n - void append_grep_pattern(struct grep_opt *opt, const char *pat,\n - const char *origin, int no, enum grep_pat_token t)\n - {\n -+#if defined(USE_LIBPCRE2) && defined(USE_NED_ALLOCATOR)\n -+ if (!pcre2_global_context && opt->ignore_case && has_non_ascii(pat))\n -+ pcre2_global_context = pcre2_general_context_create(\n -+ pcre2_malloc, pcre2_free, NULL);\n -+#endif\n - append_grep_pat(opt, pat, strlen(pat), origin, no, t);\n - }\n - \n @@ grep.c: static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt\n\n p->pcre2_compile_context = NULL;\n @@ grep.c: static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_\n if (opt->ignore_case) {\n if (has_non_ascii(p->pattern)) {\n - character_tables = pcre2_maketables(NULL);\n -+#ifdef USE_NED_ALLOCATOR\n + if (!pcre2_global_context)\n + BUG(\"pcre2_global_context uninitialized\");\n -+#endif\n + character_tables = pcre2_maketables(pcre2_global_context);\n p->pcre2_compile_context = pcre2_compile_context_create(NULL);\n pcre2_set_character_tables(p->pcre2_compile_context, character_tables);\n3: 54aef142151 ! 3: 3ced7c386a0 grep: avoid leak of chartables in PCRE2\n @@\n ## Metadata ##\n -Author: Carlo Marcelo Arenas Bel├│n \n +Author: Johannes Schindelin \n\n ## Commit message ##\n grep: avoid leak of chartables in PCRE2\n @@ Commit message\n for PCRE1.\n\n Signed-off-by: Carlo Marcelo Arenas Bel├│n \n - Signed-off-by: Junio C Hamano \n + Signed-off-by: Johannes Schindelin \n\n ## grep.c ##\n @@ grep.c: static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt\n @@ grep.c: static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_\n int patinforet;\n size_t jitsizearg;\n @@ grep.c: static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt\n + if (has_non_ascii(p->pattern)) {\n if (!pcre2_global_context)\n BUG(\"pcre2_global_context uninitialized\");\n - #endif\n - character_tables = pcre2_maketables(pcre2_global_context);\n + p->pcre2_tables = pcre2_maketables(pcre2_global_context);\n p->pcre2_compile_context = pcre2_compile_context_create(NULL);\n\nMy merge conflict resolution with master\n[https://github.com/dscho/git/commit/pcre2-chartables-leakfix-merge-with-master]\n:\n\ndiff --cc grep.c\nindex 9556d13dc1d,0bb4cbd3d82..b7ae5a442a6\n--- a/grep.c\n+++ b/grep.c\n@@@ -533,15 -470,11 +506,15 @@@ static void compile_pcre2_pattern(struc\n\n p->pcre2_compile_context = NULL;\n\n + /* pcre2_global_context is initialized in append_grep_pattern */\n if (opt->ignore_case) {\n- if (has_non_ascii(p->pattern)) {\n+ if (!opt->ignore_locale && has_non_ascii(p->pattern)) {\n - character_tables = pcre2_maketables(NULL);\n + if (!pcre2_global_context)\n + BUG(\"pcre2_global_context uninitialized\");\n + p->pcre2_tables = pcre2_maketables(pcre2_global_context);\n p->pcre2_compile_context = pcre2_compile_context_create(NULL);\n - pcre2_set_character_tables(p->pcre2_compile_context, character_tables);\n + pcre2_set_character_tables(p->pcre2_compile_context,\n + p->pcre2_tables);\n }\n options |= PCRE2_CASELESS;\n }\n@@@ -643,9 -571,6 +611,7 @@@ static void free_pcre2_pattern(struct g\n pcre2_compile_context_free(p->pcre2_compile_context);\n pcre2_code_free(p->pcre2_pattern);\n pcre2_match_data_free(p->pcre2_match_data);\n- pcre2_jit_stack_free(p->pcre2_jit_stack);\n- pcre2_match_context_free(p->pcre2_match_context);\n + free((void *)p->pcre2_tables);\n }\n #else /* !USE_LIBPCRE2 */\n static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt)\ndiff --cc grep.h\nindex 533165c21a5,05dc1bb98e5..811fd274c95\n--- a/grep.h\n+++ b/grep.h\n@@@ -94,12 -78,9 +78,10 @@@ struct grep_pat \n pcre2_code *pcre2_pattern;\n pcre2_match_data *pcre2_match_data;\n pcre2_compile_context *pcre2_compile_context;\n- pcre2_match_context *pcre2_match_context;\n- pcre2_jit_stack *pcre2_jit_stack;\n + const uint8_t *pcre2_tables;\n uint32_t pcre2_jit_on;\n- kwset_t kws;\n unsigned fixed:1;\n+ unsigned is_fixed:1;\n unsigned ignore_case:1;\n unsigned word_regexp:1;\n };\n\nCarlo Marcelo Arenas Belón (2):\n grep: make PCRE1 aware of custom allocator\n grep: make PCRE2 aware of custom allocator\n\nJohannes Schindelin (1):\n grep: avoid leak of chartables in PCRE2\n\n builtin/grep.c | 1 +\n grep.c | 47 ++++++++++++++++++++++++++++++++++++++++++++---\n grep.h | 2 ++\n 3 files changed, 47 insertions(+), 3 deletions(-)\n\n\nbase-commit: 51cf315870bbb7254ddf06c84fe03b41bc48eebd\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-402%2Fdscho%2Fpcre2-chartables-leakfix-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-402/dscho/pcre2-chartables-leakfix-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/402\n-- \ngitgitgadget\n"},{"id":"384179","messageId":"f21b2c9eb5881396303de4b607631205142cf389.1571227613.git.gitgitgadget@gmail.com","threadId":"52046","inReplyTo":"pull.402.git.1571227613.gitgitgadget@gmail.com","subject":"[PATCH 3/3] grep: avoid leak of chartables in PCRE2","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-10-16T12:06:53Z","receivedAt":"2019-10-16T12:07:03Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\n94da9193a6 (\"grep: add support for PCRE v2\", 2017-06-01) introduced\na small memory leak visible with valgrind in t7813.\n\nComplete the creation of a PCRE2 specific variable that was missing from\nthe original change and free the generated table just like it is done\nfor PCRE1.\n\nSigned-off-by: Carlo Marcelo Arenas Belón \nSigned-off-by: Johannes Schindelin \n---\n grep.c | 7 ++++---\n grep.h | 1 +\n 2 files changed, 5 insertions(+), 3 deletions(-)\n\ndiff --git a/grep.c b/grep.c\nindex 296edbb56f..9556d13dc1 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -525,7 +525,6 @@ static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt\n \tPCRE2_UCHAR errbuf[256];\n \tPCRE2_SIZE erroffset;\n \tint options = PCRE2_MULTILINE;\n-\tconst uint8_t *character_tables = NULL;\n \tint jitret;\n \tint patinforet;\n \tsize_t jitsizearg;\n@@ -539,9 +538,10 @@ static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt\n \t\tif (has_non_ascii(p->pattern)) {\n \t\t\tif (!pcre2_global_context)\n \t\t\t\tBUG(\"pcre2_global_context uninitialized\");\n-\t\t\tcharacter_tables = pcre2_maketables(pcre2_global_context);\n+\t\t\tp->pcre2_tables = pcre2_maketables(pcre2_global_context);\n \t\t\tp->pcre2_compile_context = pcre2_compile_context_create(NULL);\n-\t\t\tpcre2_set_character_tables(p->pcre2_compile_context, character_tables);\n+\t\t\tpcre2_set_character_tables(p->pcre2_compile_context,\n+\t\t\t\t\t\t\tp->pcre2_tables);\n \t\t}\n \t\toptions |= PCRE2_CASELESS;\n \t}\n@@ -645,6 +645,7 @@ static void free_pcre2_pattern(struct grep_pat *p)\n \tpcre2_match_data_free(p->pcre2_match_data);\n \tpcre2_jit_stack_free(p->pcre2_jit_stack);\n \tpcre2_match_context_free(p->pcre2_match_context);\n+\tfree((void *)p->pcre2_tables);\n }\n #else /* !USE_LIBPCRE2 */\n static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt)\ndiff --git a/grep.h b/grep.h\nindex 526c2db9ef..533165c21a 100644\n--- a/grep.h\n+++ b/grep.h\n@@ -96,6 +96,7 @@ struct grep_pat {\n \tpcre2_compile_context *pcre2_compile_context;\n \tpcre2_match_context *pcre2_match_context;\n \tpcre2_jit_stack *pcre2_jit_stack;\n+\tconst uint8_t *pcre2_tables;\n \tuint32_t pcre2_jit_on;\n \tkwset_t kws;\n \tunsigned fixed:1;\n-- \ngitgitgadget\n"},{"id":"384180","messageId":"4feb8cc83a6d0b73bd7e47a513b91ce193c6ed6f.1571227824.git.gitgitgadget@gmail.com","threadId":"52046","inReplyTo":"pull.402.v2.git.1571227824.gitgitgadget@gmail.com","subject":"[PATCH v2 1/3] grep: make PCRE1 aware of custom allocator","fromName":"Carlo Marcelo Arenas Belón via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-10-16T12:10:22Z","receivedAt":"2019-10-16T12:10:29Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"From: =?UTF-8?q?Carlo=20Marcelo=20Arenas=20Bel=C3=B3n?= \n\n63e7e9d8b6 (\"git-grep: Learn PCRE\", 2011-05-09) didn't include a way\nto override the system alocator, and so it is incompatible with\nUSE_NED_ALLOCATOR as reported by Dscho[1] (in similar code from PCRE2)\n\nNote that nedmalloc, as well as other custom allocators like jemalloc\nand mi-malloc, can be configured at runtime (via `LD_PRELOAD`),\ntherefore we cannot know at compile time whether a custom allocator is\nused or not.\n\nMake the minimum change possible to ensure this combination is supported\nby extending `grep_init()` to set the PCRE1 specific functions to Git's\nidea of `malloc()` and `free()` and therefore making sure all\nallocations are done inside PCRE1 with the same allocator than the rest\nof Git.\n\nThis change has negligible performance impact: PCRE needs to allocate\nmemory once per program run for the character table and for each pattern\ncompilation. These are both rare events compared to matching patterns\nagainst lines. Actual measurements[2] show that the impact is lost in\nthe noise.\n\n[1] https://public-inbox.org/git/pull.306.git.gitgitgadget@gmail.com\n[2] https://public-inbox.org/git/7f42007f-911b-c570-17f6-1c6af0429586@web.de\n\nSigned-off-by: Carlo Marcelo Arenas Belón \nSigned-off-by: Johannes Schindelin \n---\n grep.c | 8 ++++++++\n 1 file changed, 8 insertions(+)\n\ndiff --git a/grep.c b/grep.c\nindex cd952ef5d3..db6e0d895f 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -150,12 +150,20 @@ int grep_config(const char *var, const char *value, void *cb)\n * Initialize one instance of grep_opt and copy the\n * default values from the template we read the configuration\n * information in an earlier call to git_config(grep_config).\n+ *\n+ * If using PCRE, make sure that the library is configured\n+ * to use the same allocator as Git (e.g. nedmalloc on Windows).\n */\n void grep_init(struct grep_opt *opt, struct repository *repo, const char *prefix)\n {\n \tstruct grep_opt *def = &grep_defaults;\n \tint i;\n \n+#ifdef USE_LIBPCRE1\n+\tpcre_malloc = malloc;\n+\tpcre_free = free;\n+#endif\n+\n \tmemset(opt, 0, sizeof(*opt));\n \topt->repo = repo;\n \topt->prefix = prefix;\n-- \ngitgitgadget\n\n"},{"id":"384181","messageId":"pull.402.v2.git.1571227824.gitgitgadget@gmail.com","threadId":"52046","inReplyTo":"pull.402.git.1571227613.gitgitgadget@gmail.com","subject":"[PATCH v2 0/3] Revive 'pcre2-chartables-leakfix'","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-10-16T12:10:21Z","receivedAt":"2019-10-16T12:10:29Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"As I had stated several times, I was really unhappy how the original fix\nharped on nedmalloc and totally ignored runtime-configured custom\nallocators.\n\nSo this is, at long last, my attempt to give this a new life. It is based\noff of maint and needs trivial merge conflict resolutions relative to master\n.\n\nChanges since v1:\n\n * I managed to mess up the authorship of 3/3. Sorry for that. I fixed it,\n so that Carlo is shown as author again.\n\nCarlo Marcelo Arenas Belón (3):\n grep: make PCRE1 aware of custom allocator\n grep: make PCRE2 aware of custom allocator\n grep: avoid leak of chartables in PCRE2\n\n builtin/grep.c | 1 +\n grep.c | 47 ++++++++++++++++++++++++++++++++++++++++++++---\n grep.h | 2 ++\n 3 files changed, 47 insertions(+), 3 deletions(-)\n\n\nbase-commit: 51cf315870bbb7254ddf06c84fe03b41bc48eebd\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-402%2Fdscho%2Fpcre2-chartables-leakfix-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-402/dscho/pcre2-chartables-leakfix-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/402\n\nRange-diff vs v1:\n\n 1: 4feb8cc83a = 1: 4feb8cc83a grep: make PCRE1 aware of custom allocator\n 2: 191d3a2280 = 2: 191d3a2280 grep: make PCRE2 aware of custom allocator\n 3: f21b2c9eb5 ! 3: f8724fb267 grep: avoid leak of chartables in PCRE2\n @@ -1,4 +1,4 @@\n -Author: Johannes Schindelin \n +Author: Carlo Marcelo Arenas Belón \n \n grep: avoid leak of chartables in PCRE2\n \n\n-- \ngitgitgadget\n"},{"id":"384182","messageId":"191d3a2280232ff98964fd42bfe0bc85ee3708f5.1571227824.git.gitgitgadget@gmail.com","threadId":"52046","inReplyTo":"pull.402.v2.git.1571227824.gitgitgadget@gmail.com","subject":"[PATCH v2 2/3] grep: make PCRE2 aware of custom allocator","fromName":"Carlo Marcelo Arenas Belón via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-10-16T12:10:23Z","receivedAt":"2019-10-16T12:10:30Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"From: =?UTF-8?q?Carlo=20Marcelo=20Arenas=20Bel=C3=B3n?= \n\n94da9193a6 (grep: add support for PCRE v2, 2017-06-01) didn't include\na way to override the system allocator, and so it is incompatible with\ncustom allocators (e.g. nedmalloc). This problem became obvious when we\ntried to plug a memory leak by `free()`ing a data structure allocated by\nPCRE2, triggering a segfault in Windows (where we use nedmalloc by\ndefault).\n\nPCRE2 requires the use of a general context to override the allocator\nand therefore, there is a lot more code needed than in PCRE1, including\na couple of wrapper functions.\n\nExtend the grep API with a \"destructor\" that could be called to cleanup\nany objects that were created and used globally.\n\nUpdate `builtin/grep.c` to use that new API, but any other future users\nshould make sure to have matching `grep_init()`/`grep_destroy()` calls\nif they are using the pattern matching functionality.\n\nMove some of the logic that was before done per thread (in the workers)\ninto an earlier phase to avoid degrading performance, but as the use of\nPCRE2 with custom allocators is better understood it is expected more of\nits functions will be instructed to use the custom allocator as well as\nwas done in the original code[1] this work was based on.\n\n[1] https://public-inbox.org/git/3397e6797f872aedd18c6d795f4976e1c579514b.1565005867.git.gitgitgadget@gmail.com/\n\nReported-by: Johannes Schindelin \nSigned-off-by: Carlo Marcelo Arenas Belón \nSigned-off-by: Johannes Schindelin \n---\n builtin/grep.c | 1 +\n grep.c | 34 +++++++++++++++++++++++++++++++++-\n grep.h | 1 +\n 3 files changed, 35 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex 560051784e..e49c20df60 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -1145,5 +1145,6 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n \t\trun_pager(&opt, prefix);\n \tclear_pathspec(&pathspec);\n \tfree_grep_patterns(&opt);\n+\tgrep_destroy();\n \treturn !hit;\n }\ndiff --git a/grep.c b/grep.c\nindex db6e0d895f..296edbb56f 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -16,6 +16,20 @@ static int grep_source_is_binary(struct grep_source *gs,\n \n static struct grep_opt grep_defaults;\n \n+#ifdef USE_LIBPCRE2\n+static pcre2_general_context *pcre2_global_context;\n+\n+static void *pcre2_malloc(PCRE2_SIZE size, MAYBE_UNUSED void *memory_data)\n+{\n+\treturn malloc(size);\n+}\n+\n+static void pcre2_free(void *pointer, MAYBE_UNUSED void *memory_data)\n+{\n+\treturn free(pointer);\n+}\n+#endif\n+\n static const char *color_grep_slots[] = {\n \t[GREP_COLOR_CONTEXT]\t = \"context\",\n \t[GREP_COLOR_FILENAME]\t = \"filename\",\n@@ -153,12 +167,20 @@ int grep_config(const char *var, const char *value, void *cb)\n *\n * If using PCRE, make sure that the library is configured\n * to use the same allocator as Git (e.g. nedmalloc on Windows).\n+ *\n+ * Any allocated memory needs to be released in grep_destroy().\n */\n void grep_init(struct grep_opt *opt, struct repository *repo, const char *prefix)\n {\n \tstruct grep_opt *def = &grep_defaults;\n \tint i;\n \n+#if defined(USE_LIBPCRE2)\n+\tif (!pcre2_global_context)\n+\t\tpcre2_global_context = pcre2_general_context_create(\n+\t\t\t\t\tpcre2_malloc, pcre2_free, NULL);\n+#endif\n+\n #ifdef USE_LIBPCRE1\n \tpcre_malloc = malloc;\n \tpcre_free = free;\n@@ -186,6 +208,13 @@ void grep_init(struct grep_opt *opt, struct repository *repo, const char *prefix\n \t\tcolor_set(opt->colors[i], def->colors[i]);\n }\n \n+void grep_destroy(void)\n+{\n+#ifdef USE_LIBPCRE2\n+\tpcre2_general_context_free(pcre2_global_context);\n+#endif\n+}\n+\n static void grep_set_pattern_type_option(enum grep_pattern_type pattern_type, struct grep_opt *opt)\n {\n \t/*\n@@ -505,9 +534,12 @@ static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt\n \n \tp->pcre2_compile_context = NULL;\n \n+\t/* pcre2_global_context is initialized in append_grep_pattern */\n \tif (opt->ignore_case) {\n \t\tif (has_non_ascii(p->pattern)) {\n-\t\t\tcharacter_tables = pcre2_maketables(NULL);\n+\t\t\tif (!pcre2_global_context)\n+\t\t\t\tBUG(\"pcre2_global_context uninitialized\");\n+\t\t\tcharacter_tables = pcre2_maketables(pcre2_global_context);\n \t\t\tp->pcre2_compile_context = pcre2_compile_context_create(NULL);\n \t\t\tpcre2_set_character_tables(p->pcre2_compile_context, character_tables);\n \t\t}\ndiff --git a/grep.h b/grep.h\nindex 1875880f37..526c2db9ef 100644\n--- a/grep.h\n+++ b/grep.h\n@@ -189,6 +189,7 @@ struct grep_opt {\n void init_grep_defaults(struct repository *);\n int grep_config(const char *var, const char *value, void *);\n void grep_init(struct grep_opt *, struct repository *repo, const char *prefix);\n+void grep_destroy(void);\n void grep_commit_pattern_type(enum grep_pattern_type, struct grep_opt *opt);\n \n void append_grep_pat(struct grep_opt *opt, const char *pat, size_t patlen, const char *origin, int no, enum grep_pat_token t);\n-- \ngitgitgadget\n\n"},{"id":"384183","messageId":"f8724fb267a1629712db742f1e06a452f6b69d3d.1571227824.git.gitgitgadget@gmail.com","threadId":"52046","inReplyTo":"pull.402.v2.git.1571227824.gitgitgadget@gmail.com","subject":"[PATCH v2 3/3] grep: avoid leak of chartables in PCRE2","fromName":"Carlo Marcelo Arenas Belón via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-10-16T12:10:24Z","receivedAt":"2019-10-16T12:10:32Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"From: =?UTF-8?q?Carlo=20Marcelo=20Arenas=20Bel=C3=B3n?= \n\n94da9193a6 (\"grep: add support for PCRE v2\", 2017-06-01) introduced\na small memory leak visible with valgrind in t7813.\n\nComplete the creation of a PCRE2 specific variable that was missing from\nthe original change and free the generated table just like it is done\nfor PCRE1.\n\nSigned-off-by: Carlo Marcelo Arenas Belón \nSigned-off-by: Johannes Schindelin \n---\n grep.c | 7 ++++---\n grep.h | 1 +\n 2 files changed, 5 insertions(+), 3 deletions(-)\n\ndiff --git a/grep.c b/grep.c\nindex 296edbb56f..9556d13dc1 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -525,7 +525,6 @@ static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt\n \tPCRE2_UCHAR errbuf[256];\n \tPCRE2_SIZE erroffset;\n \tint options = PCRE2_MULTILINE;\n-\tconst uint8_t *character_tables = NULL;\n \tint jitret;\n \tint patinforet;\n \tsize_t jitsizearg;\n@@ -539,9 +538,10 @@ static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt\n \t\tif (has_non_ascii(p->pattern)) {\n \t\t\tif (!pcre2_global_context)\n \t\t\t\tBUG(\"pcre2_global_context uninitialized\");\n-\t\t\tcharacter_tables = pcre2_maketables(pcre2_global_context);\n+\t\t\tp->pcre2_tables = pcre2_maketables(pcre2_global_context);\n \t\t\tp->pcre2_compile_context = pcre2_compile_context_create(NULL);\n-\t\t\tpcre2_set_character_tables(p->pcre2_compile_context, character_tables);\n+\t\t\tpcre2_set_character_tables(p->pcre2_compile_context,\n+\t\t\t\t\t\t\tp->pcre2_tables);\n \t\t}\n \t\toptions |= PCRE2_CASELESS;\n \t}\n@@ -645,6 +645,7 @@ static void free_pcre2_pattern(struct grep_pat *p)\n \tpcre2_match_data_free(p->pcre2_match_data);\n \tpcre2_jit_stack_free(p->pcre2_jit_stack);\n \tpcre2_match_context_free(p->pcre2_match_context);\n+\tfree((void *)p->pcre2_tables);\n }\n #else /* !USE_LIBPCRE2 */\n static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt)\ndiff --git a/grep.h b/grep.h\nindex 526c2db9ef..533165c21a 100644\n--- a/grep.h\n+++ b/grep.h\n@@ -96,6 +96,7 @@ struct grep_pat {\n \tpcre2_compile_context *pcre2_compile_context;\n \tpcre2_match_context *pcre2_match_context;\n \tpcre2_jit_stack *pcre2_jit_stack;\n+\tconst uint8_t *pcre2_tables;\n \tuint32_t pcre2_jit_on;\n \tkwset_t kws;\n \tunsigned fixed:1;\n-- \ngitgitgadget\n"},{"id":"384335","messageId":"xmqqsgnq6des.fsf@gitster-ct.c.googlers.com","threadId":"52046","inReplyTo":"191d3a2280232ff98964fd42bfe0bc85ee3708f5.1571227824.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 2/3] grep: make PCRE2 aware of custom allocator","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-10-18T01:38:35Z","receivedAt":"2019-10-18T01:38:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Carlo Marcelo Arenas Belón via GitGitGadget\"\n writes:\n\n> builtin/grep.c | 1 +\n> grep.c | 34 +++++++++++++++++++++++++++++++++-\n> grep.h | 1 +\n> 3 files changed, 35 insertions(+), 1 deletion(-)\n\n> \n> +#if defined(USE_LIBPCRE2)\n> +\tif (!pcre2_global_context)\n> +\t\tpcre2_global_context = pcre2_general_context_create(\n> +\t\t\t\t\tpcre2_malloc, pcre2_free, NULL);\n> +#endif\n\nThis part should use the same \"#ifdef\" as the other one which is a\nminor nit, for consistency. I do not care too deeply which way we\nunify, but we should stick to one.\n\n> +\n> #ifdef USE_LIBPCRE1\n> \tpcre_malloc = malloc;\n> \tpcre_free = free;\n\nOther than that, all 3 patches look sensible, and they certainly got\nsimplified by dropping the conditional ;-).\n\n"},{"id":"416196","messageId":"20210204210556.25242-1-avarab@gmail.com","threadId":"52046","inReplyTo":"191d3a2280232ff98964fd42bfe0bc85ee3708f5.1571227824.git.gitgitgadget@gmail.com","subject":"[PATCH 00/10] grep/pcre2: memory allocation fixes","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-02-04T21:05:46Z","receivedAt":"2021-02-04T21:06:54Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"This series fixes up bugs in our PCRE v2 wrapper code and how it\nhandles malloc()/free().\n\nJunio: I'm splitting this off my recently sent 25 patch series, which\nI should probably have sent as an RFC:\nhttps://lore.kernel.org/git/20210203032811.14979-1-avarab@gmail.com/\n\nIt's on top of \"next\", as it would otherwise conflict with my\nin-flight ab/grep-pcre-invalid-utf8, ab/lose-grep-debug and ab/retire-pcre1.\n\n06/10 is a follow-up improvement (not a fix, the in-flight works fine\ntoo) for ab/grep-pcre-invalid-utf8. The latter two just touch adjacent\nlines of code.\n\nThere's no notable new behavior here, just cleanup of existing\nfunctionality. In mid-2019 there was a lot of discussion around the\ncode being fixed here:\n\n https://lore.kernel.org/git/pull.306.git.gitgitgadget@gmail.com/#t\n https://lore.kernel.org/git/pull.402.git.1571227613.gitgitgadget@gmail.com/\n\nAs discussed in 08/10 I believe that fix was so difficult to get right\nbecause it was starting out with a fundamentally incorrect assumption\nabout how PCRE v2's memory handling works. With 08-10/10 we end up\nwith a much easier to reason about end-state, as the API itself is\nactually quite simple.\n\nÆvar Arnfjörð Bjarmason (10):\n grep/pcre2: drop needless assignment + assert() on opt->pcre2\n grep/pcre2: drop needless assignment to NULL\n grep/pcre2: correct reference to grep_init() in comment\n grep/pcre2: prepare to add debugging to pcre2_malloc()\n grep/pcre2: add GREP_PCRE2_DEBUG_MALLOC debug mode\n grep/pcre2: use compile-time PCREv2 version test\n grep/pcre2: use pcre2_maketables_free() function\n grep/pcre2: actually make pcre2 use custom allocator\n grep/pcre2: move back to thread-only PCREv2 structures\n grep/pcre2: move definitions of pcre2_{malloc,free}\n\n builtin/grep.c | 1 -\n grep.c | 99 ++++++++++++++++++++++----------------------------\n grep.h | 9 ++++-\n 3 files changed, 51 insertions(+), 58 deletions(-)\n\n-- \n2.30.0.284.gd98b1dd5eaa7\n\n"},{"id":"416197","messageId":"20210204210556.25242-2-avarab@gmail.com","threadId":"52046","inReplyTo":"191d3a2280232ff98964fd42bfe0bc85ee3708f5.1571227824.git.gitgitgadget@gmail.com","subject":"[PATCH 01/10] grep/pcre2: drop needless assignment + assert() on opt->pcre2","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-02-04T21:05:47Z","receivedAt":"2021-02-04T21:07:01Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Drop an assignment added in b65abcafc7a (grep: use PCRE v2 for\noptimized fixed-string search, 2019-07-01) and the overly cautious\nassert() I added in 94da9193a6e (grep: add support for PCRE v2,\n2017-06-01).\n\nThere was never a good reason for this, it's just a relic from when I\ninitially wrote the PCREv2 support. We're not going to have confusion\nabout compile_pcre2_pattern() being called when it shouldn't just\nbecause we forgot to cargo-cult this opt->pcre2 option, and \"opt\"\nis (mostly) used for the options the user supplied, let's avoid the\npattern of needlessly assigning to it.\n\nWith my in-flight removal of PCREv1 [1] (\"Remove support for v1 of the\nPCRE library\", 2021-01-24) there'll be even less confusion around what\nwe call where in these codepaths, which is one more reason to remove\nthis.\n\n1. https://lore.kernel.org/git/xmqqmtwy29x8.fsf@gitster.c.googlers.com/\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n grep.c | 3 ---\n 1 file changed, 3 deletions(-)\n\ndiff --git a/grep.c b/grep.c\nindex aabfaaa4c3..816e23f17e 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -373,8 +373,6 @@ static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt\n \tint patinforet;\n \tsize_t jitsizearg;\n \n-\tassert(opt->pcre2);\n-\n \tp->pcre2_compile_context = NULL;\n \n \t/* pcre2_global_context is initialized in append_grep_pattern */\n@@ -555,7 +553,6 @@ static void compile_regexp(struct grep_pat *p, struct grep_opt *opt)\n #endif\n \tif (p->fixed || p->is_fixed) {\n #ifdef USE_LIBPCRE2\n-\t\topt->pcre2 = 1;\n \t\tif (p->is_fixed) {\n \t\t\tcompile_pcre2_pattern(p, opt);\n \t\t} else {\n-- \n2.30.0.284.gd98b1dd5eaa7\n\n"},{"id":"416198","messageId":"20210204210556.25242-3-avarab@gmail.com","threadId":"52046","inReplyTo":"191d3a2280232ff98964fd42bfe0bc85ee3708f5.1571227824.git.gitgitgadget@gmail.com","subject":"[PATCH 02/10] grep/pcre2: drop needless assignment to NULL","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-02-04T21:05:48Z","receivedAt":"2021-02-04T21:07:04Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Remove a redundant assignment of pcre2_compile_context dating back to\nmy 94da9193a6e (grep: add support for PCRE v2, 2017-06-01). In\ncreate_grep_pat() we xcalloc() the \"grep_pat\" struct, so there's no\nneed to NULL out individual members here.\n\nI think this was probably something left over from an earlier\ndevelopment version of mine.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n grep.c | 2 --\n 1 file changed, 2 deletions(-)\n\ndiff --git a/grep.c b/grep.c\nindex 816e23f17e..f27c5de7f5 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -373,8 +373,6 @@ static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt\n \tint patinforet;\n \tsize_t jitsizearg;\n \n-\tp->pcre2_compile_context = NULL;\n-\n \t/* pcre2_global_context is initialized in append_grep_pattern */\n \tif (opt->ignore_case) {\n \t\tif (!opt->ignore_locale && has_non_ascii(p->pattern)) {\n-- \n2.30.0.284.gd98b1dd5eaa7\n\n"},{"id":"416199","messageId":"20210204210556.25242-4-avarab@gmail.com","threadId":"52046","inReplyTo":"191d3a2280232ff98964fd42bfe0bc85ee3708f5.1571227824.git.gitgitgadget@gmail.com","subject":"[PATCH 03/10] grep/pcre2: correct reference to grep_init() in comment","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-02-04T21:05:49Z","receivedAt":"2021-02-04T21:07:12Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Correct a comment added in 513f2b0bbd4 (grep: make PCRE2 aware of\ncustom allocator, 2019-10-16). This comment was never correct in\ngit.git, but was consistent with an older version of the patch[1].\n\n1. https://lore.kernel.org/git/20190806163658.66932-3-carenas@gmail.com/\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n grep.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/grep.c b/grep.c\nindex f27c5de7f5..b9adcd83e7 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -373,7 +373,7 @@ static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt\n \tint patinforet;\n \tsize_t jitsizearg;\n \n-\t/* pcre2_global_context is initialized in append_grep_pattern */\n+\t/* pcre2_global_context is initialized in grep_init */\n \tif (opt->ignore_case) {\n \t\tif (!opt->ignore_locale && has_non_ascii(p->pattern)) {\n \t\t\tif (!pcre2_global_context)\n-- \n2.30.0.284.gd98b1dd5eaa7\n\n"},{"id":"416200","messageId":"20210204210556.25242-5-avarab@gmail.com","threadId":"52046","inReplyTo":"191d3a2280232ff98964fd42bfe0bc85ee3708f5.1571227824.git.gitgitgadget@gmail.com","subject":"[PATCH 04/10] grep/pcre2: prepare to add debugging to pcre2_malloc()","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-02-04T21:05:50Z","receivedAt":"2021-02-04T21:07:14Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Change pcre2_malloc() in a way that'll make it easier for a debugging\nfprintf() to spew out the allocated pointer. This doesn't introduce\nany functional change, it just makes a subsequent commit's diff easier\nto read. Changes code added in 513f2b0bbd4 (grep: make PCRE2 aware of\ncustom allocator, 2019-10-16).\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n grep.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/grep.c b/grep.c\nindex b9adcd83e7..f96d86c929 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -45,7 +45,8 @@ static pcre2_general_context *pcre2_global_context;\n \n static void *pcre2_malloc(PCRE2_SIZE size, MAYBE_UNUSED void *memory_data)\n {\n-\treturn malloc(size);\n+\tvoid *pointer = malloc(size);\n+\treturn pointer;\n }\n \n static void pcre2_free(void *pointer, MAYBE_UNUSED void *memory_data)\n-- \n2.30.0.284.gd98b1dd5eaa7\n\n"},{"id":"416201","messageId":"20210204210556.25242-6-avarab@gmail.com","threadId":"52046","inReplyTo":"191d3a2280232ff98964fd42bfe0bc85ee3708f5.1571227824.git.gitgitgadget@gmail.com","subject":"[PATCH 05/10] grep/pcre2: add GREP_PCRE2_DEBUG_MALLOC debug mode","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-02-04T21:05:51Z","receivedAt":"2021-02-04T21:07:38Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Add optional printing of PCREv2 allocations to stderr for a developer\nwho manually changes the GREP_PCRE2_DEBUG_MALLOC definition to\n\"1\".\n\nThis will be referenced a subsequent commit, and is generally useful\nto manually see what's going on with PCREv2 allocations while working\non that code.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n grep.c | 10 ++++++++++\n 1 file changed, 10 insertions(+)\n\ndiff --git a/grep.c b/grep.c\nindex f96d86c929..7d262a23d8 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -42,15 +42,25 @@ static struct grep_opt grep_defaults = {\n \n #ifdef USE_LIBPCRE2\n static pcre2_general_context *pcre2_global_context;\n+#define GREP_PCRE2_DEBUG_MALLOC 0\n \n static void *pcre2_malloc(PCRE2_SIZE size, MAYBE_UNUSED void *memory_data)\n {\n \tvoid *pointer = malloc(size);\n+#if GREP_PCRE2_DEBUG_MALLOC\n+\tstatic int count = 1;\n+\tfprintf(stderr, \"PCRE2:%p -> #%02d: alloc(%lu)\\n\", pointer, count++, size);\n+#endif\n \treturn pointer;\n }\n \n static void pcre2_free(void *pointer, MAYBE_UNUSED void *memory_data)\n {\n+#if GREP_PCRE2_DEBUG_MALLOC\n+\tstatic int count = 1;\n+\tif (pointer)\n+\t\tfprintf(stderr, \"PCRE2:%p -> #%02d: free()\\n\", pointer, count++);\n+#endif\n \tfree(pointer);\n }\n #endif\n-- \n2.30.0.284.gd98b1dd5eaa7\n\n"},{"id":"416202","messageId":"20210204210556.25242-7-avarab@gmail.com","threadId":"52046","inReplyTo":"191d3a2280232ff98964fd42bfe0bc85ee3708f5.1571227824.git.gitgitgadget@gmail.com","subject":"[PATCH 06/10] grep/pcre2: use compile-time PCREv2 version test","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-02-04T21:05:52Z","receivedAt":"2021-02-04T21:07:40Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Replace a use of pcre2_config(PCRE2_CONFIG_VERSION, ...) which I added\nin 95ca1f987ed (grep/pcre2: better support invalid UTF-8 haystacks,\n2021-01-24) with the same test done at compile-time.\n\nIt might be cuter to do this at runtime since we don't have to do the\n\"major >= 11 || (major >= 10 && ...)\" test. But in the next commit\nwe'll add another version comparison that absolutely needs to be done\nat compile-time, so we're better of being consistent across the board.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n grep.c | 18 ++++--------------\n grep.h | 3 +++\n 2 files changed, 7 insertions(+), 14 deletions(-)\n\ndiff --git a/grep.c b/grep.c\nindex 7d262a23d8..e58044474d 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -400,21 +400,11 @@ static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt\n \t !(!opt->ignore_case && (p->fixed || p->is_fixed)))\n \t\toptions |= (PCRE2_UTF | PCRE2_MATCH_INVALID_UTF);\n \n+#ifdef GIT_PCRE2_VERSION_10_36_OR_HIGHER\n \t/* Work around https://bugs.exim.org/show_bug.cgi?id=2642 fixed in 10.36 */\n-\tif (PCRE2_MATCH_INVALID_UTF && options & (PCRE2_UTF | PCRE2_CASELESS)) {\n-\t\tstruct strbuf buf;\n-\t\tint len;\n-\t\tint err;\n-\n-\t\tif ((len = pcre2_config(PCRE2_CONFIG_VERSION, NULL)) < 0)\n-\t\t\tBUG(\"pcre2_config(..., NULL) failed: %d\", len);\n-\t\tstrbuf_init(&buf, len + 1);\n-\t\tif ((err = pcre2_config(PCRE2_CONFIG_VERSION, buf.buf)) < 0)\n-\t\t\tBUG(\"pcre2_config(..., buf.buf) failed: %d\", err);\n-\t\tif (versioncmp(buf.buf, \"10.36\") < 0)\n-\t\t\toptions |= PCRE2_NO_START_OPTIMIZE;\n-\t\tstrbuf_release(&buf);\n-\t}\n+\tif (PCRE2_MATCH_INVALID_UTF && options & (PCRE2_UTF | PCRE2_CASELESS))\n+\t\toptions |= PCRE2_NO_START_OPTIMIZE;\n+#endif\n \n \tp->pcre2_pattern = pcre2_compile((PCRE2_SPTR)p->pattern,\n \t\t\t\t\t p->patternlen, options, &error, &erroffset,\ndiff --git a/grep.h b/grep.h\nindex ae89d6254b..54e52042cb 100644\n--- a/grep.h\n+++ b/grep.h\n@@ -4,6 +4,9 @@\n #ifdef USE_LIBPCRE2\n #define PCRE2_CODE_UNIT_WIDTH 8\n #include \n+#if (PCRE2_MAJOR >= 10 && PCRE2_MINOR >= 36) || PCRE2_MAJOR >= 11\n+#define GIT_PCRE2_VERSION_10_36_OR_HIGHER\n+#endif\n #else\n typedef int pcre2_code;\n typedef int pcre2_match_data;\n-- \n2.30.0.284.gd98b1dd5eaa7\n\n"},{"id":"416203","messageId":"20210204210556.25242-9-avarab@gmail.com","threadId":"52046","inReplyTo":"191d3a2280232ff98964fd42bfe0bc85ee3708f5.1571227824.git.gitgitgadget@gmail.com","subject":"[PATCH 08/10] grep/pcre2: actually make pcre2 use custom allocator","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-02-04T21:05:54Z","receivedAt":"2021-02-04T21:07:43Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Continue work started in 513f2b0bbd4 (grep: make PCRE2 aware of custom\nallocator, 2019-10-16) and make PCREv2 use our pcre2_{malloc,free}().\nfunctions for allocation. We'll now use it for all PCREv2 allocations.\n\nThe reason 513f2b0bbd4 worked as a bugfix for the USE_NED_ALLOCATOR\nissue is because it managed to target pretty much the allocation freed\nvia free(), as opposed to by a pcre2_*free() function. I.e. the\npcre2_maketables() and pcre2_maketables_free() pair. For most of the\nrest we continued allocating with stock malloc() inside PCREv2 itself,\nbut didn't segfault because we'd use its corresponding free().\n\nIn a preceding commit of mine I changed the free() to\npcre2_maketables_free() on versions of PCREv2 10.34 and newer. So as\nfar as fixing the segfault goes we could revert 513f2b0bbd4. But then\nwe wouldn't use the desired allocator, let's just use it instead.\n\nBefore this patch we'd on e.g.:\n\n grep --threads=1 -iP æ.*var.*xyz\n\nOnly use pcre2_{malloc,free}() for 2 malloc() calls and 2\ncorresponding free() call. Now it's 12 calls to each. This can be\nobserved with the GREP_PCRE2_DEBUG_MALLOC debug mode.\n\nReading the history of how this bug got introduced it wasn't present\nin Johannes's original patch[1] to fix the issue.\n\nMy reading of that thread is that the approach the follow-up patches\nto Johannes's original pursued were based on misunderstanding of how\nthe PCREv2 API works. In particular this part of [2]:\n\n \"most of the time (like when using UTF-8) the chartable (and\n therefore the global context) is not needed (even when using\n alternate allocators)\"\n\nThat's simply not how PCREv2 memory allocation works. It's easy to see\nhow the misunderstanding came about. It's because (as noted above) the\nissue was noticed because of our use of free() in our own grep.c for\nfreeing the memory allocated by pcre2_maketables().\n\nThus the misunderstanding that PCREv2's compile context is something\nonly needed for pcre2_maketables(), and e.g. an aborted earlier\nattempt[3] to only set it up when we ourselves called\npcre2_maketables().\n\nThat's not what PCREv2's compile context is. To quote PCREv2's\ndocumentation:\n\n \"This context just contains pointers to (and data for) external\n memory management functions that are called from several places in\n the PCRE2 library.\"\n\nThus the failed attempts to go down the route of only creating the\ngeneral context in cases where we ourselves call pcre2_maketables(),\nbefore finally settling on the approach 513f2b0bbd4 took of always\ncreating it.\n\nInstead we should always create it, and then pass the general context\nto those functions that accept it, so that they'll consistently use\nour preferred memory allocation functions.\n\n1. https://public-inbox.org/git/3397e6797f872aedd18c6d795f4976e1c579514b.1565005867.git.gitgitgadget@gmail.com/\n2. https://lore.kernel.org/git/CAPUEsphMh_ZqcH3M7PXC9jHTfEdQN3mhTAK2JDkdvKBp53YBoA@mail.gmail.com/\n3. https://lore.kernel.org/git/20190806085014.47776-3-carenas@gmail.com/\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n grep.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/grep.c b/grep.c\nindex c63dbff4b2..0116ff5f09 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -390,7 +390,7 @@ static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt\n \t\t\tif (!pcre2_global_context)\n \t\t\t\tBUG(\"pcre2_global_context uninitialized\");\n \t\t\tp->pcre2_tables = pcre2_maketables(pcre2_global_context);\n-\t\t\tp->pcre2_compile_context = pcre2_compile_context_create(NULL);\n+\t\t\tp->pcre2_compile_context = pcre2_compile_context_create(pcre2_global_context);\n \t\t\tpcre2_set_character_tables(p->pcre2_compile_context,\n \t\t\t\t\t\t\tp->pcre2_tables);\n \t\t}\n@@ -411,7 +411,7 @@ static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt\n \t\t\t\t\t p->pcre2_compile_context);\n \n \tif (p->pcre2_pattern) {\n-\t\tp->pcre2_match_data = pcre2_match_data_create_from_pattern(p->pcre2_pattern, NULL);\n+\t\tp->pcre2_match_data = pcre2_match_data_create_from_pattern(p->pcre2_pattern, pcre2_global_context);\n \t\tif (!p->pcre2_match_data)\n \t\t\tdie(\"Couldn't allocate PCRE2 match data\");\n \t} else {\n-- \n2.30.0.284.gd98b1dd5eaa7\n\n"},{"id":"416204","messageId":"20210204210556.25242-8-avarab@gmail.com","threadId":"52046","inReplyTo":"191d3a2280232ff98964fd42bfe0bc85ee3708f5.1571227824.git.gitgitgadget@gmail.com","subject":"[PATCH 07/10] grep/pcre2: use pcre2_maketables_free() function","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-02-04T21:05:53Z","receivedAt":"2021-02-04T21:07:53Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Make use of the pcre2_maketables_free() function to free the memory\nallocated by pcre2_maketables(). At first sight it's strange that\n10da030ab75 (grep: avoid leak of chartables in PCRE2, 2019-10-16)\nwhich added the free() call here doesn't make use of the pcre2_free()\nthe author introduced in the preceding commit in 513f2b0bbd4 (grep:\nmake PCRE2 aware of custom allocator, 2019-10-16).\n\nThe reason is that at the time the function didn't exist. It was first\nintroduced in PCREv2 version 10.34, released on 2019-11-21.\n\nLet's make use of it behind a macro. I don't think this matters for\nanything to do with custom allocators, but it makes our use of PCREv2\nmore discoverable. At some distant point in the future we'll be able\nto drop the version guard, as nobody will be running a version older\nthan 10.34.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n grep.c | 4 ++++\n grep.h | 3 +++\n 2 files changed, 7 insertions(+)\n\ndiff --git a/grep.c b/grep.c\nindex e58044474d..c63dbff4b2 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -490,7 +490,11 @@ static void free_pcre2_pattern(struct grep_pat *p)\n \tpcre2_compile_context_free(p->pcre2_compile_context);\n \tpcre2_code_free(p->pcre2_pattern);\n \tpcre2_match_data_free(p->pcre2_match_data);\n+#ifdef GIT_PCRE2_VERSION_10_34_OR_HIGHER\n+\tpcre2_maketables_free(pcre2_global_context, p->pcre2_tables);\n+#else\n \tfree((void *)p->pcre2_tables);\n+#endif\n }\n #else /* !USE_LIBPCRE2 */\n static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt)\ndiff --git a/grep.h b/grep.h\nindex 54e52042cb..64666e9204 100644\n--- a/grep.h\n+++ b/grep.h\n@@ -7,6 +7,9 @@\n #if (PCRE2_MAJOR >= 10 && PCRE2_MINOR >= 36) || PCRE2_MAJOR >= 11\n #define GIT_PCRE2_VERSION_10_36_OR_HIGHER\n #endif\n+#if (PCRE2_MAJOR >= 10 && PCRE2_MINOR >= 34) || PCRE2_MAJOR >= 11\n+#define GIT_PCRE2_VERSION_10_34_OR_HIGHER\n+#endif\n #else\n typedef int pcre2_code;\n typedef int pcre2_match_data;\n-- \n2.30.0.284.gd98b1dd5eaa7\n\n"},{"id":"416205","messageId":"20210204210556.25242-10-avarab@gmail.com","threadId":"52046","inReplyTo":"191d3a2280232ff98964fd42bfe0bc85ee3708f5.1571227824.git.gitgitgadget@gmail.com","subject":"[PATCH 09/10] grep/pcre2: move back to thread-only PCREv2 structures","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-02-04T21:05:55Z","receivedAt":"2021-02-04T21:07:57Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Change the setup of the \"pcre2_general_context\" to happen per-thread\nin compile_pcre2_pattern() instead of in grep_init(), as happens with\nall the rest of the pcre2_* members of the grep_pat structure.\n\nAs noted in the preceding commit the approach 513f2b0bbd4 (grep: make\nPCRE2 aware of custom allocator, 2019-10-16) took to allocate the\npcre2_general_context seems to have been initially based on a\nmisunderstanding of how PCREv2 memory allocation works.\n\nThis approach of creating a global context is just added complexity\nfor almost zero gain. On my system it's 24 bytes saved per-thread, for\ncontext PCREv2 will then go on to some kilobytes for its own\nthread-local state.\n\nAs noted in 6d423dd542f (grep: don't redundantly compile throwaway\npatterns under threading, 2017-05-25) the grep code is intentionally\nnot trying to micro-optimize allocations by e.g. sharing some PCREv2\nstructures globally, while making others thread-local.\n\nSo let's remove this special case and make all of them thread-local\nfor simplicity again.\n\nSee also the discussion in 94da9193a6 (grep: add support for PCRE v2,\n2017-06-01) about thread safety, and Johannes's comments[1] to the\neffect that we should be doing what this patch is doing.\n\n1. https://lore.kernel.org/git/nycvar.QRO.7.76.6.1908052120302.46@tvgsbejvaqbjf.bet/\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n builtin/grep.c | 1 -\n grep.c | 41 +++++++++++++++--------------------------\n grep.h | 3 ++-\n 3 files changed, 17 insertions(+), 28 deletions(-)\n\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex 55d06c9513..c69fe99340 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -1175,6 +1175,5 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n \t\trun_pager(&opt, prefix);\n \tclear_pathspec(&pathspec);\n \tfree_grep_patterns(&opt);\n-\tgrep_destroy();\n \treturn !hit;\n }\ndiff --git a/grep.c b/grep.c\nindex 0116ff5f09..2599f329cd 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -41,7 +41,6 @@ static struct grep_opt grep_defaults = {\n };\n \n #ifdef USE_LIBPCRE2\n-static pcre2_general_context *pcre2_global_context;\n #define GREP_PCRE2_DEBUG_MALLOC 0\n \n static void *pcre2_malloc(PCRE2_SIZE size, MAYBE_UNUSED void *memory_data)\n@@ -163,20 +162,9 @@ int grep_config(const char *var, const char *value, void *cb)\n * Initialize one instance of grep_opt and copy the\n * default values from the template we read the configuration\n * information in an earlier call to git_config(grep_config).\n- *\n- * If using PCRE, make sure that the library is configured\n- * to use the same allocator as Git (e.g. nedmalloc on Windows).\n- *\n- * Any allocated memory needs to be released in grep_destroy().\n */\n void grep_init(struct grep_opt *opt, struct repository *repo, const char *prefix)\n {\n-#if defined(USE_LIBPCRE2)\n-\tif (!pcre2_global_context)\n-\t\tpcre2_global_context = pcre2_general_context_create(\n-\t\t\t\t\tpcre2_malloc, pcre2_free, NULL);\n-#endif\n-\n \t*opt = grep_defaults;\n \n \topt->repo = repo;\n@@ -186,13 +174,6 @@ void grep_init(struct grep_opt *opt, struct repository *repo, const char *prefix\n \topt->header_tail = &opt->header_list;\n }\n \n-void grep_destroy(void)\n-{\n-#ifdef USE_LIBPCRE2\n-\tpcre2_general_context_free(pcre2_global_context);\n-#endif\n-}\n-\n static void grep_set_pattern_type_option(enum grep_pattern_type pattern_type, struct grep_opt *opt)\n {\n \t/*\n@@ -384,13 +365,20 @@ static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt\n \tint patinforet;\n \tsize_t jitsizearg;\n \n-\t/* pcre2_global_context is initialized in grep_init */\n+\t/*\n+\t * Call pcre2_general_context_create() before calling any\n+\t * other pcre2_*(). It sets up our malloc()/free() functions\n+\t * with which everything else is allocated.\n+\t */\n+\tp->pcre2_general_context = pcre2_general_context_create(\n+\t\tpcre2_malloc, pcre2_free, NULL);\n+\tif (!p->pcre2_general_context)\n+\t\tdie(\"Couldn't allocate PCRE2 general context\");\n+\n \tif (opt->ignore_case) {\n \t\tif (!opt->ignore_locale && has_non_ascii(p->pattern)) {\n-\t\t\tif (!pcre2_global_context)\n-\t\t\t\tBUG(\"pcre2_global_context uninitialized\");\n-\t\t\tp->pcre2_tables = pcre2_maketables(pcre2_global_context);\n-\t\t\tp->pcre2_compile_context = pcre2_compile_context_create(pcre2_global_context);\n+\t\t\tp->pcre2_tables = pcre2_maketables(p->pcre2_general_context);\n+\t\t\tp->pcre2_compile_context = pcre2_compile_context_create(p->pcre2_general_context);\n \t\t\tpcre2_set_character_tables(p->pcre2_compile_context,\n \t\t\t\t\t\t\tp->pcre2_tables);\n \t\t}\n@@ -411,7 +399,7 @@ static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt\n \t\t\t\t\t p->pcre2_compile_context);\n \n \tif (p->pcre2_pattern) {\n-\t\tp->pcre2_match_data = pcre2_match_data_create_from_pattern(p->pcre2_pattern, pcre2_global_context);\n+\t\tp->pcre2_match_data = pcre2_match_data_create_from_pattern(p->pcre2_pattern, p->pcre2_general_context);\n \t\tif (!p->pcre2_match_data)\n \t\t\tdie(\"Couldn't allocate PCRE2 match data\");\n \t} else {\n@@ -491,10 +479,11 @@ static void free_pcre2_pattern(struct grep_pat *p)\n \tpcre2_code_free(p->pcre2_pattern);\n \tpcre2_match_data_free(p->pcre2_match_data);\n #ifdef GIT_PCRE2_VERSION_10_34_OR_HIGHER\n-\tpcre2_maketables_free(pcre2_global_context, p->pcre2_tables);\n+\tpcre2_maketables_free(p->pcre2_general_context, p->pcre2_tables);\n #else\n \tfree((void *)p->pcre2_tables);\n #endif\n+\tpcre2_general_context_free(p->pcre2_general_context);\n }\n #else /* !USE_LIBPCRE2 */\n static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt)\ndiff --git a/grep.h b/grep.h\nindex 64666e9204..72f82b1e30 100644\n--- a/grep.h\n+++ b/grep.h\n@@ -14,6 +14,7 @@\n typedef int pcre2_code;\n typedef int pcre2_match_data;\n typedef int pcre2_compile_context;\n+typedef int pcre2_general_context;\n #endif\n #ifndef PCRE2_MATCH_INVALID_UTF\n /* PCRE2_MATCH_* dummy also with !USE_LIBPCRE2, for test-pcre2-config.c */\n@@ -75,6 +76,7 @@ struct grep_pat {\n \tpcre2_code *pcre2_pattern;\n \tpcre2_match_data *pcre2_match_data;\n \tpcre2_compile_context *pcre2_compile_context;\n+\tpcre2_general_context *pcre2_general_context;\n \tconst uint8_t *pcre2_tables;\n \tuint32_t pcre2_jit_on;\n \tunsigned fixed:1;\n@@ -167,7 +169,6 @@ struct grep_opt {\n \n int grep_config(const char *var, const char *value, void *);\n void grep_init(struct grep_opt *, struct repository *repo, const char *prefix);\n-void grep_destroy(void);\n void grep_commit_pattern_type(enum grep_pattern_type, struct grep_opt *opt);\n \n void append_grep_pat(struct grep_opt *opt, const char *pat, size_t patlen, const char *origin, int no, enum grep_pat_token t);\n-- \n2.30.0.284.gd98b1dd5eaa7\n\n"},{"id":"416206","messageId":"20210204210556.25242-11-avarab@gmail.com","threadId":"52046","inReplyTo":"191d3a2280232ff98964fd42bfe0bc85ee3708f5.1571227824.git.gitgitgadget@gmail.com","subject":"[PATCH 10/10] grep/pcre2: move definitions of pcre2_{malloc,free}","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-02-04T21:05:56Z","receivedAt":"2021-02-04T21:08:01Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Move the definitions of the pcre2_{malloc,free} functions above the\ncompile_pcre2_pattern() function they're used it. Before the preceding\ncommit they used to be needed earlier, but now we can move them to be\nadjacent to the other PCREv2 functions.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n grep.c | 46 ++++++++++++++++++++++------------------------\n 1 file changed, 22 insertions(+), 24 deletions(-)\n\ndiff --git a/grep.c b/grep.c\nindex 2599f329cd..636ac48bf0 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -40,30 +40,6 @@ static struct grep_opt grep_defaults = {\n \t.output = std_output,\n };\n \n-#ifdef USE_LIBPCRE2\n-#define GREP_PCRE2_DEBUG_MALLOC 0\n-\n-static void *pcre2_malloc(PCRE2_SIZE size, MAYBE_UNUSED void *memory_data)\n-{\n-\tvoid *pointer = malloc(size);\n-#if GREP_PCRE2_DEBUG_MALLOC\n-\tstatic int count = 1;\n-\tfprintf(stderr, \"PCRE2:%p -> #%02d: alloc(%lu)\\n\", pointer, count++, size);\n-#endif\n-\treturn pointer;\n-}\n-\n-static void pcre2_free(void *pointer, MAYBE_UNUSED void *memory_data)\n-{\n-#if GREP_PCRE2_DEBUG_MALLOC\n-\tstatic int count = 1;\n-\tif (pointer)\n-\t\tfprintf(stderr, \"PCRE2:%p -> #%02d: free()\\n\", pointer, count++);\n-#endif\n-\tfree(pointer);\n-}\n-#endif\n-\n static const char *color_grep_slots[] = {\n \t[GREP_COLOR_CONTEXT]\t = \"context\",\n \t[GREP_COLOR_FILENAME]\t = \"filename\",\n@@ -355,6 +331,28 @@ static int is_fixed(const char *s, size_t len)\n }\n \n #ifdef USE_LIBPCRE2\n+#define GREP_PCRE2_DEBUG_MALLOC 0\n+\n+static void *pcre2_malloc(PCRE2_SIZE size, MAYBE_UNUSED void *memory_data)\n+{\n+\tvoid *pointer = malloc(size);\n+#if GREP_PCRE2_DEBUG_MALLOC\n+\tstatic int count = 1;\n+\tfprintf(stderr, \"PCRE2:%p -> #%02d: alloc(%lu)\\n\", pointer, count++, size);\n+#endif\n+\treturn pointer;\n+}\n+\n+static void pcre2_free(void *pointer, MAYBE_UNUSED void *memory_data)\n+{\n+#if GREP_PCRE2_DEBUG_MALLOC\n+\tstatic int count = 1;\n+\tif (pointer)\n+\t\tfprintf(stderr, \"PCRE2:%p -> #%02d: free()\\n\", pointer, count++);\n+#endif\n+\tfree(pointer);\n+}\n+\n static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt)\n {\n \tint error;\n-- \n2.30.0.284.gd98b1dd5eaa7\n\n"},{"id":"416612","messageId":"nycvar.QRO.7.76.6.2102101135390.29765@tvgsbejvaqbjf.bet","threadId":"52046","inReplyTo":"20210204210556.25242-6-avarab@gmail.com","subject":"Re: [PATCH 05/10] grep/pcre2: add GREP_PCRE2_DEBUG_MALLOC debug mode","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2021-02-10T10:38:55Z","receivedAt":"2021-02-10T10:43:06Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Ævar,\n\nOn Thu, 4 Feb 2021, Ævar Arnfjörð Bjarmason wrote:\n\n> Add optional printing of PCREv2 allocations to stderr for a developer\n> who manually changes the GREP_PCRE2_DEBUG_MALLOC definition to\n> \"1\".\n\nMaybe clarify in the oneline that this is not an environment variable, but\na Makefile knob? I had to read all the way to the diff to understand that\naspect.\n\nOtherwise, the patch series so far looks really fine to me.\n\nThanks,\nDscho\n\n>\n> This will be referenced a subsequent commit, and is generally useful\n> to manually see what's going on with PCREv2 allocations while working\n> on that code.\n>\n> Signed-off-by: Ævar Arnfjörð Bjarmason \n> ---\n> grep.c | 10 ++++++++++\n> 1 file changed, 10 insertions(+)\n>\n> diff --git a/grep.c b/grep.c\n> index f96d86c929..7d262a23d8 100644\n> --- a/grep.c\n> +++ b/grep.c\n> @@ -42,15 +42,25 @@ static struct grep_opt grep_defaults = {\n>\n> #ifdef USE_LIBPCRE2\n> static pcre2_general_context *pcre2_global_context;\n> +#define GREP_PCRE2_DEBUG_MALLOC 0\n>\n> static void *pcre2_malloc(PCRE2_SIZE size, MAYBE_UNUSED void *memory_data)\n> {\n> \tvoid *pointer = malloc(size);\n> +#if GREP_PCRE2_DEBUG_MALLOC\n> +\tstatic int count = 1;\n> +\tfprintf(stderr, \"PCRE2:%p -> #%02d: alloc(%lu)\\n\", pointer, count++, size);\n> +#endif\n> \treturn pointer;\n> }\n>\n> static void pcre2_free(void *pointer, MAYBE_UNUSED void *memory_data)\n> {\n> +#if GREP_PCRE2_DEBUG_MALLOC\n> +\tstatic int count = 1;\n> +\tif (pointer)\n> +\t\tfprintf(stderr, \"PCRE2:%p -> #%02d: free()\\n\", pointer, count++);\n> +#endif\n> \tfree(pointer);\n> }\n> #endif\n> --\n> 2.30.0.284.gd98b1dd5eaa7\n>\n>\n"},{"id":"416613","messageId":"nycvar.QRO.7.76.6.2102101142020.29765@tvgsbejvaqbjf.bet","threadId":"52046","inReplyTo":"20210204210556.25242-8-avarab@gmail.com","subject":"Re: [PATCH 07/10] grep/pcre2: use pcre2_maketables_free() function","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2021-02-10T10:43:01Z","receivedAt":"2021-02-10T10:47:02Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Ævar,\n\nOn Thu, 4 Feb 2021, Ævar Arnfjörð Bjarmason wrote:\n\n> Make use of the pcre2_maketables_free() function to free the memory\n> allocated by pcre2_maketables(). At first sight it's strange that\n> 10da030ab75 (grep: avoid leak of chartables in PCRE2, 2019-10-16)\n> which added the free() call here doesn't make use of the pcre2_free()\n> the author introduced in the preceding commit in 513f2b0bbd4 (grep:\n> make PCRE2 aware of custom allocator, 2019-10-16).\n>\n> The reason is that at the time the function didn't exist. It was first\n> introduced in PCREv2 version 10.34, released on 2019-11-21.\n\nGit for Windows still uses v10.33. Thanks for the prod, I will update the\nlibrary.\n\nCiao,\nDscho\n\n>\n> Let's make use of it behind a macro. I don't think this matters for\n> anything to do with custom allocators, but it makes our use of PCREv2\n> more discoverable. At some distant point in the future we'll be able\n> to drop the version guard, as nobody will be running a version older\n> than 10.34.\n>\n> Signed-off-by: Ævar Arnfjörð Bjarmason \n> ---\n> grep.c | 4 ++++\n> grep.h | 3 +++\n> 2 files changed, 7 insertions(+)\n>\n> diff --git a/grep.c b/grep.c\n> index e58044474d..c63dbff4b2 100644\n> --- a/grep.c\n> +++ b/grep.c\n> @@ -490,7 +490,11 @@ static void free_pcre2_pattern(struct grep_pat *p)\n> \tpcre2_compile_context_free(p->pcre2_compile_context);\n> \tpcre2_code_free(p->pcre2_pattern);\n> \tpcre2_match_data_free(p->pcre2_match_data);\n> +#ifdef GIT_PCRE2_VERSION_10_34_OR_HIGHER\n> +\tpcre2_maketables_free(pcre2_global_context, p->pcre2_tables);\n> +#else\n> \tfree((void *)p->pcre2_tables);\n> +#endif\n> }\n> #else /* !USE_LIBPCRE2 */\n> static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt)\n> diff --git a/grep.h b/grep.h\n> index 54e52042cb..64666e9204 100644\n> --- a/grep.h\n> +++ b/grep.h\n> @@ -7,6 +7,9 @@\n> #if (PCRE2_MAJOR >= 10 && PCRE2_MINOR >= 36) || PCRE2_MAJOR >= 11\n> #define GIT_PCRE2_VERSION_10_36_OR_HIGHER\n> #endif\n> +#if (PCRE2_MAJOR >= 10 && PCRE2_MINOR >= 34) || PCRE2_MAJOR >= 11\n> +#define GIT_PCRE2_VERSION_10_34_OR_HIGHER\n> +#endif\n> #else\n> typedef int pcre2_code;\n> typedef int pcre2_match_data;\n> --\n> 2.30.0.284.gd98b1dd5eaa7\n>\n>\n"},{"id":"416628","messageId":"nycvar.QRO.7.76.6.2102101338220.29765@tvgsbejvaqbjf.bet","threadId":"52046","inReplyTo":"20210204210556.25242-9-avarab@gmail.com","subject":"Re: [PATCH 08/10] grep/pcre2: actually make pcre2 use custom allocator","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2021-02-10T12:38:37Z","receivedAt":"2021-02-10T12:42:31Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Ævar,\n\nACK!\n\nAnd thank you for this patch,\nDscho\n\nOn Thu, 4 Feb 2021, Ævar Arnfjörð Bjarmason wrote:\n\n> Continue work started in 513f2b0bbd4 (grep: make PCRE2 aware of custom\n> allocator, 2019-10-16) and make PCREv2 use our pcre2_{malloc,free}().\n> functions for allocation. We'll now use it for all PCREv2 allocations.\n>\n> The reason 513f2b0bbd4 worked as a bugfix for the USE_NED_ALLOCATOR\n> issue is because it managed to target pretty much the allocation freed\n> via free(), as opposed to by a pcre2_*free() function. I.e. the\n> pcre2_maketables() and pcre2_maketables_free() pair. For most of the\n> rest we continued allocating with stock malloc() inside PCREv2 itself,\n> but didn't segfault because we'd use its corresponding free().\n>\n> In a preceding commit of mine I changed the free() to\n> pcre2_maketables_free() on versions of PCREv2 10.34 and newer. So as\n> far as fixing the segfault goes we could revert 513f2b0bbd4. But then\n> we wouldn't use the desired allocator, let's just use it instead.\n>\n> Before this patch we'd on e.g.:\n>\n> grep --threads=1 -iP æ.*var.*xyz\n>\n> Only use pcre2_{malloc,free}() for 2 malloc() calls and 2\n> corresponding free() call. Now it's 12 calls to each. This can be\n> observed with the GREP_PCRE2_DEBUG_MALLOC debug mode.\n>\n> Reading the history of how this bug got introduced it wasn't present\n> in Johannes's original patch[1] to fix the issue.\n>\n> My reading of that thread is that the approach the follow-up patches\n> to Johannes's original pursued were based on misunderstanding of how\n> the PCREv2 API works. In particular this part of [2]:\n>\n> \"most of the time (like when using UTF-8) the chartable (and\n> therefore the global context) is not needed (even when using\n> alternate allocators)\"\n>\n> That's simply not how PCREv2 memory allocation works. It's easy to see\n> how the misunderstanding came about. It's because (as noted above) the\n> issue was noticed because of our use of free() in our own grep.c for\n> freeing the memory allocated by pcre2_maketables().\n>\n> Thus the misunderstanding that PCREv2's compile context is something\n> only needed for pcre2_maketables(), and e.g. an aborted earlier\n> attempt[3] to only set it up when we ourselves called\n> pcre2_maketables().\n>\n> That's not what PCREv2's compile context is. To quote PCREv2's\n> documentation:\n>\n> \"This context just contains pointers to (and data for) external\n> memory management functions that are called from several places in\n> the PCRE2 library.\"\n>\n> Thus the failed attempts to go down the route of only creating the\n> general context in cases where we ourselves call pcre2_maketables(),\n> before finally settling on the approach 513f2b0bbd4 took of always\n> creating it.\n>\n> Instead we should always create it, and then pass the general context\n> to those functions that accept it, so that they'll consistently use\n> our preferred memory allocation functions.\n>\n> 1. https://public-inbox.org/git/3397e6797f872aedd18c6d795f4976e1c579514b.1565005867.git.gitgitgadget@gmail.com/\n> 2. https://lore.kernel.org/git/CAPUEsphMh_ZqcH3M7PXC9jHTfEdQN3mhTAK2JDkdvKBp53YBoA@mail.gmail.com/\n> 3. https://lore.kernel.org/git/20190806085014.47776-3-carenas@gmail.com/\n>\n> Signed-off-by: Ævar Arnfjörð Bjarmason \n> ---\n> grep.c | 4 ++--\n> 1 file changed, 2 insertions(+), 2 deletions(-)\n>\n> diff --git a/grep.c b/grep.c\n> index c63dbff4b2..0116ff5f09 100644\n> --- a/grep.c\n> +++ b/grep.c\n> @@ -390,7 +390,7 @@ static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt\n> \t\t\tif (!pcre2_global_context)\n> \t\t\t\tBUG(\"pcre2_global_context uninitialized\");\n> \t\t\tp->pcre2_tables = pcre2_maketables(pcre2_global_context);\n> -\t\t\tp->pcre2_compile_context = pcre2_compile_context_create(NULL);\n> +\t\t\tp->pcre2_compile_context = pcre2_compile_context_create(pcre2_global_context);\n> \t\t\tpcre2_set_character_tables(p->pcre2_compile_context,\n> \t\t\t\t\t\t\tp->pcre2_tables);\n> \t\t}\n> @@ -411,7 +411,7 @@ static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt\n> \t\t\t\t\t p->pcre2_compile_context);\n>\n> \tif (p->pcre2_pattern) {\n> -\t\tp->pcre2_match_data = pcre2_match_data_create_from_pattern(p->pcre2_pattern, NULL);\n> +\t\tp->pcre2_match_data = pcre2_match_data_create_from_pattern(p->pcre2_pattern, pcre2_global_context);\n> \t\tif (!p->pcre2_match_data)\n> \t\t\tdie(\"Couldn't allocate PCRE2 match data\");\n> \t} else {\n> --\n> 2.30.0.284.gd98b1dd5eaa7\n>\n>\n"},{"id":"416629","messageId":"nycvar.QRO.7.76.6.2102101339560.29765@tvgsbejvaqbjf.bet","threadId":"52046","inReplyTo":"20210204210556.25242-11-avarab@gmail.com","subject":"Re: [PATCH 10/10] grep/pcre2: move definitions of pcre2_{malloc,free}","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2021-02-10T12:40:58Z","receivedAt":"2021-02-10T12:44:45Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Ævar,\n\nOn Thu, 4 Feb 2021, Ævar Arnfjörð Bjarmason wrote:\n\n> Move the definitions of the pcre2_{malloc,free} functions above the\n> compile_pcre2_pattern() function they're used it. Before the preceding\n\ns/it/in/\n\nThank you for this entire patch series. I like its intention and its\nexecution.\n\nCiao,\nDscho\n\n> commit they used to be needed earlier, but now we can move them to be\n> adjacent to the other PCREv2 functions.\n>\n> Signed-off-by: Ævar Arnfjörð Bjarmason \n> ---\n> grep.c | 46 ++++++++++++++++++++++------------------------\n> 1 file changed, 22 insertions(+), 24 deletions(-)\n>\n> diff --git a/grep.c b/grep.c\n> index 2599f329cd..636ac48bf0 100644\n> --- a/grep.c\n> +++ b/grep.c\n> @@ -40,30 +40,6 @@ static struct grep_opt grep_defaults = {\n> \t.output = std_output,\n> };\n>\n> -#ifdef USE_LIBPCRE2\n> -#define GREP_PCRE2_DEBUG_MALLOC 0\n> -\n> -static void *pcre2_malloc(PCRE2_SIZE size, MAYBE_UNUSED void *memory_data)\n> -{\n> -\tvoid *pointer = malloc(size);\n> -#if GREP_PCRE2_DEBUG_MALLOC\n> -\tstatic int count = 1;\n> -\tfprintf(stderr, \"PCRE2:%p -> #%02d: alloc(%lu)\\n\", pointer, count++, size);\n> -#endif\n> -\treturn pointer;\n> -}\n> -\n> -static void pcre2_free(void *pointer, MAYBE_UNUSED void *memory_data)\n> -{\n> -#if GREP_PCRE2_DEBUG_MALLOC\n> -\tstatic int count = 1;\n> -\tif (pointer)\n> -\t\tfprintf(stderr, \"PCRE2:%p -> #%02d: free()\\n\", pointer, count++);\n> -#endif\n> -\tfree(pointer);\n> -}\n> -#endif\n> -\n> static const char *color_grep_slots[] = {\n> \t[GREP_COLOR_CONTEXT]\t = \"context\",\n> \t[GREP_COLOR_FILENAME]\t = \"filename\",\n> @@ -355,6 +331,28 @@ static int is_fixed(const char *s, size_t len)\n> }\n>\n> #ifdef USE_LIBPCRE2\n> +#define GREP_PCRE2_DEBUG_MALLOC 0\n> +\n> +static void *pcre2_malloc(PCRE2_SIZE size, MAYBE_UNUSED void *memory_data)\n> +{\n> +\tvoid *pointer = malloc(size);\n> +#if GREP_PCRE2_DEBUG_MALLOC\n> +\tstatic int count = 1;\n> +\tfprintf(stderr, \"PCRE2:%p -> #%02d: alloc(%lu)\\n\", pointer, count++, size);\n> +#endif\n> +\treturn pointer;\n> +}\n> +\n> +static void pcre2_free(void *pointer, MAYBE_UNUSED void *memory_data)\n> +{\n> +#if GREP_PCRE2_DEBUG_MALLOC\n> +\tstatic int count = 1;\n> +\tif (pointer)\n> +\t\tfprintf(stderr, \"PCRE2:%p -> #%02d: free()\\n\", pointer, count++);\n> +#endif\n> +\tfree(pointer);\n> +}\n> +\n> static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt)\n> {\n> \tint error;\n> --\n> 2.30.0.284.gd98b1dd5eaa7\n>\n>\n"},{"id":"416665","messageId":"xmqqlfbv1ub1.fsf@gitster.c.googlers.com","threadId":"52046","inReplyTo":"20210204210556.25242-1-avarab@gmail.com","subject":"Re: [PATCH 00/10] grep/pcre2: memory allocation fixes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-02-10T21:34:10Z","receivedAt":"2021-02-10T21:34:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> This series fixes up bugs in our PCRE v2 wrapper code and how it\n> handles malloc()/free().\n>\n> Junio: I'm splitting this off my recently sent 25 patch series, which\n> I should probably have sent as an RFC:\n> https://lore.kernel.org/git/20210203032811.14979-1-avarab@gmail.com/\n>\n> It's on top of \"next\", as it would otherwise conflict with my\n> in-flight ab/grep-pcre-invalid-utf8, ab/lose-grep-debug and ab/retire-pcre1.\n\nThese three seem to be solid and I am planning to merge them down to\n'master' soonish. I was hoping that the series would get enough\nreviews by the time it happens so that I can expect an update that\ncleanly applies on top of 'master', and the plan seems to be working\nwell ;-)\n\n"},{"id":"417252","messageId":"20210218000728.13995-1-avarab@gmail.com","threadId":"52046","inReplyTo":"20210204210556.25242-1-avarab@gmail.com","subject":"[PATCH v2 00/10] grep/pcre2: memory allocation fixes","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-02-18T00:07:18Z","receivedAt":"2021-02-18T00:08:35Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Now based on \"master\", when I sent v1[1] it dependen on other\nin-flight topics of mine.\n\nAddresses minor issues with the commit messages raised by Johannes on\nv1, and other commit message issues I noticed myself on re-reading\nthis.\n\n1. https://lore.kernel.org/git/20210204210556.25242-1-avarab@gmail.com/\n\nÆvar Arnfjörð Bjarmason (10):\n grep/pcre2: drop needless assignment + assert() on opt->pcre2\n grep/pcre2: drop needless assignment to NULL\n grep/pcre2: correct reference to grep_init() in comment\n grep/pcre2: prepare to add debugging to pcre2_malloc()\n grep/pcre2: add GREP_PCRE2_DEBUG_MALLOC debug mode\n grep/pcre2: use compile-time PCREv2 version test\n grep/pcre2: use pcre2_maketables_free() function\n grep/pcre2: actually make pcre2 use custom allocator\n grep/pcre2: move back to thread-only PCREv2 structures\n grep/pcre2: move definitions of pcre2_{malloc,free}\n\n builtin/grep.c | 1 -\n grep.c | 99 ++++++++++++++++++++++----------------------------\n grep.h | 9 ++++-\n 3 files changed, 51 insertions(+), 58 deletions(-)\n\nRange-diff:\n 1: a11f1e91552 ! 1: 40d2e47c540 grep/pcre2: drop needless assignment + assert() on opt->pcre2\n @@ Commit message\n There was never a good reason for this, it's just a relic from when I\n initially wrote the PCREv2 support. We're not going to have confusion\n about compile_pcre2_pattern() being called when it shouldn't just\n - because we forgot to cargo-cult this opt->pcre2 option, and \"opt\"\n - is (mostly) used for the options the user supplied, let's avoid the\n - pattern of needlessly assigning to it.\n + because we forgot to cargo-cult this opt->pcre2 option.\n \n - With my in-flight removal of PCREv1 [1] (\"Remove support for v1 of the\n - PCRE library\", 2021-01-24) there'll be even less confusion around what\n - we call where in these codepaths, which is one more reason to remove\n - this.\n + Furthermore the \"struct grep_opt\" is (mostly) used for the options the\n + user supplied, let's avoid the pattern of needlessly assigning to it.\n \n - 1. https://lore.kernel.org/git/xmqqmtwy29x8.fsf@gitster.c.googlers.com/\n + With my recent removal of the PCREv1 backend in 7599730b7e2 (Remove\n + support for v1 of the PCRE library, 2021-01-24) there's even less\n + confusion around what we call where in these codepaths, which is one\n + more reason to remove this.\n \n Signed-off-by: Ævar Arnfjörð Bjarmason \n \n 2: db0ef9189e3 ! 2: e02f9b5ab50 grep/pcre2: drop needless assignment to NULL\n @@ Commit message\n grep/pcre2: drop needless assignment to NULL\n \n Remove a redundant assignment of pcre2_compile_context dating back to\n - my 94da9193a6e (grep: add support for PCRE v2, 2017-06-01). In\n - create_grep_pat() we xcalloc() the \"grep_pat\" struct, so there's no\n + my 94da9193a6e (grep: add support for PCRE v2, 2017-06-01).\n +\n + In create_grep_pat() we xcalloc() the \"grep_pat\" struct, so there's no\n need to NULL out individual members here.\n \n I think this was probably something left over from an earlier\n 3: 9c5429f4d96 = 3: 2bcb6c53589 grep/pcre2: correct reference to grep_init() in comment\n 4: e5558f5f0f1 ! 4: 78477193cd4 grep/pcre2: prepare to add debugging to pcre2_malloc()\n @@ Commit message\n grep/pcre2: prepare to add debugging to pcre2_malloc()\n \n Change pcre2_malloc() in a way that'll make it easier for a debugging\n - fprintf() to spew out the allocated pointer. This doesn't introduce\n - any functional change, it just makes a subsequent commit's diff easier\n - to read. Changes code added in 513f2b0bbd4 (grep: make PCRE2 aware of\n - custom allocator, 2019-10-16).\n + fprintf() to spew out the allocated pointer.\n +\n + This doesn't introduce any functional change, it just makes a\n + subsequent commit's diff easier to read. Changes code added in\n + 513f2b0bbd4 (grep: make PCRE2 aware of custom allocator, 2019-10-16).\n \n Signed-off-by: Ævar Arnfjörð Bjarmason \n \n 5: 7968effaa8a ! 5: cbeb521bd65 grep/pcre2: add GREP_PCRE2_DEBUG_MALLOC debug mode\n @@ Commit message\n grep/pcre2: add GREP_PCRE2_DEBUG_MALLOC debug mode\n \n Add optional printing of PCREv2 allocations to stderr for a developer\n - who manually changes the GREP_PCRE2_DEBUG_MALLOC definition to\n - \"1\".\n + who manually changes the GREP_PCRE2_DEBUG_MALLOC definition to \"1\".\n +\n + You need to manually change the definition in the source file similar\n + to the DEBUG_MAILMAP, there's no Makefile knob for this.\n \n This will be referenced a subsequent commit, and is generally useful\n to manually see what's going on with PCREv2 allocations while working\n 6: 604e183c224 = 6: 788929c3de6 grep/pcre2: use compile-time PCREv2 version test\n 7: f864a9aac4c ! 7: 6a4552c6d4e grep/pcre2: use pcre2_maketables_free() function\n @@ Commit message\n grep/pcre2: use pcre2_maketables_free() function\n \n Make use of the pcre2_maketables_free() function to free the memory\n - allocated by pcre2_maketables(). At first sight it's strange that\n - 10da030ab75 (grep: avoid leak of chartables in PCRE2, 2019-10-16)\n - which added the free() call here doesn't make use of the pcre2_free()\n - the author introduced in the preceding commit in 513f2b0bbd4 (grep:\n - make PCRE2 aware of custom allocator, 2019-10-16).\n + allocated by pcre2_maketables().\n +\n + At first sight it's strange that 10da030ab75 (grep: avoid leak of\n + chartables in PCRE2, 2019-10-16) which added the free() call here\n + doesn't make use of the pcre2_free() the author introduced in the\n + preceding commit in 513f2b0bbd4 (grep: make PCRE2 aware of custom\n + allocator, 2019-10-16).\n \n The reason is that at the time the function didn't exist. It was first\n introduced in PCREv2 version 10.34, released on 2019-11-21.\n \n Let's make use of it behind a macro. I don't think this matters for\n anything to do with custom allocators, but it makes our use of PCREv2\n - more discoverable. At some distant point in the future we'll be able\n - to drop the version guard, as nobody will be running a version older\n - than 10.34.\n + more discoverable.\n +\n + At some distant point in the future we'll be able to drop the version\n + guard, as nobody will be running a version older than 10.34.\n \n Signed-off-by: Ævar Arnfjörð Bjarmason \n \n 8: cea9e066951 ! 8: bf58d597a4b grep/pcre2: actually make pcre2 use custom allocator\n @@ Commit message\n functions for allocation. We'll now use it for all PCREv2 allocations.\n \n The reason 513f2b0bbd4 worked as a bugfix for the USE_NED_ALLOCATOR\n - issue is because it managed to target pretty much the allocation freed\n - via free(), as opposed to by a pcre2_*free() function. I.e. the\n - pcre2_maketables() and pcre2_maketables_free() pair. For most of the\n - rest we continued allocating with stock malloc() inside PCREv2 itself,\n - but didn't segfault because we'd use its corresponding free().\n + issue is because it targeted the allocation freed via free(), as\n + opposed to by a pcre2_*free() function. I.e. the pcre2_maketables()\n + and pcre2_maketables_free() pair.\n +\n + For most of the rest we continued allocating with stock malloc()\n + inside PCREv2 itself, but didn't segfault because we'd use its\n + corresponding free().\n \n In a preceding commit of mine I changed the free() to\n pcre2_maketables_free() on versions of PCREv2 10.34 and newer. So as\n @@ Commit message\n grep --threads=1 -iP æ.*var.*xyz\n \n Only use pcre2_{malloc,free}() for 2 malloc() calls and 2\n - corresponding free() call. Now it's 12 calls to each. This can be\n + corresponding free() calls. Now it's 12 calls to each. This can be\n observed with the GREP_PCRE2_DEBUG_MALLOC debug mode.\n \n Reading the history of how this bug got introduced it wasn't present\n @@ Commit message\n Thus the failed attempts to go down the route of only creating the\n general context in cases where we ourselves call pcre2_maketables(),\n before finally settling on the approach 513f2b0bbd4 took of always\n - creating it.\n + creating it, but then mostly not using it.\n \n Instead we should always create it, and then pass the general context\n to those functions that accept it, so that they'll consistently use\n our preferred memory allocation functions.\n \n - 1. https://public-inbox.org/git/3397e6797f872aedd18c6d795f4976e1c579514b.1565005867.git.gitgitgadget@gmail.com/\n + 1. https://lore.kernel.org/git/3397e6797f872aedd18c6d795f4976e1c579514b.1565005867.git.gitgitgadget@gmail.com/\n 2. https://lore.kernel.org/git/CAPUEsphMh_ZqcH3M7PXC9jHTfEdQN3mhTAK2JDkdvKBp53YBoA@mail.gmail.com/\n 3. https://lore.kernel.org/git/20190806085014.47776-3-carenas@gmail.com/\n \n 9: 99eaa918261 ! 9: 129adf872fb grep/pcre2: move back to thread-only PCREv2 structures\n @@ Commit message\n grep/pcre2: move back to thread-only PCREv2 structures\n \n Change the setup of the \"pcre2_general_context\" to happen per-thread\n - in compile_pcre2_pattern() instead of in grep_init(), as happens with\n - all the rest of the pcre2_* members of the grep_pat structure.\n + in compile_pcre2_pattern() instead of in grep_init().\n +\n + This change brings it in line with how the rest of the pcre2_* members\n + in the grep_pat structure are set up.\n \n As noted in the preceding commit the approach 513f2b0bbd4 (grep: make\n PCRE2 aware of custom allocator, 2019-10-16) took to allocate the\n pcre2_general_context seems to have been initially based on a\n misunderstanding of how PCREv2 memory allocation works.\n \n - This approach of creating a global context is just added complexity\n - for almost zero gain. On my system it's 24 bytes saved per-thread, for\n - context PCREv2 will then go on to some kilobytes for its own\n - thread-local state.\n + The approach of creating a global context in grep_init() is just added\n + complexity for almost zero gain. On my system it's 24 bytes saved\n + per-thread. For comparison PCREv2 will then go on to allocate at least\n + a kilobyte for its own thread-local state.\n \n As noted in 6d423dd542f (grep: don't redundantly compile throwaway\n patterns under threading, 2017-05-25) the grep code is intentionally\n @@ Commit message\n structures globally, while making others thread-local.\n \n So let's remove this special case and make all of them thread-local\n - for simplicity again.\n + again for simplicity. With this change we could move the\n + pcre2_{malloc,free} functions around to live closer to their current\n + use. I'm not doing that here to keep this change small, that cleanup\n + will be done in a follow-up commit.\n \n See also the discussion in 94da9193a6 (grep: add support for PCRE v2,\n 2017-06-01) about thread safety, and Johannes's comments[1] to the\n10: 462f12126c8 ! 10: 688d1b00d5d grep/pcre2: move definitions of pcre2_{malloc,free}\n @@ Commit message\n grep/pcre2: move definitions of pcre2_{malloc,free}\n \n Move the definitions of the pcre2_{malloc,free} functions above the\n - compile_pcre2_pattern() function they're used it. Before the preceding\n - commit they used to be needed earlier, but now we can move them to be\n - adjacent to the other PCREv2 functions.\n + compile_pcre2_pattern() function they're used in.\n +\n + Before the preceding commit they used to be needed earlier, but now we\n + can move them to be adjacent to the other PCREv2 functions.\n \n Signed-off-by: Ævar Arnfjörð Bjarmason \n \n-- \n2.30.0.284.gd98b1dd5eaa7\n\n"},{"id":"417253","messageId":"20210218000728.13995-4-avarab@gmail.com","threadId":"52046","inReplyTo":"20210204210556.25242-1-avarab@gmail.com","subject":"[PATCH v2 03/10] grep/pcre2: correct reference to grep_init() in comment","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-02-18T00:07:21Z","receivedAt":"2021-02-18T00:08:36Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Correct a comment added in 513f2b0bbd4 (grep: make PCRE2 aware of\ncustom allocator, 2019-10-16). This comment was never correct in\ngit.git, but was consistent with an older version of the patch[1].\n\n1. https://lore.kernel.org/git/20190806163658.66932-3-carenas@gmail.com/\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n grep.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/grep.c b/grep.c\nindex f27c5de7f56..b9adcd83e7a 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -373,7 +373,7 @@ static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt\n \tint patinforet;\n \tsize_t jitsizearg;\n \n-\t/* pcre2_global_context is initialized in append_grep_pattern */\n+\t/* pcre2_global_context is initialized in grep_init */\n \tif (opt->ignore_case) {\n \t\tif (!opt->ignore_locale && has_non_ascii(p->pattern)) {\n \t\t\tif (!pcre2_global_context)\n-- \n2.30.0.284.gd98b1dd5eaa7\n\n"},{"id":"417254","messageId":"20210218000728.13995-5-avarab@gmail.com","threadId":"52046","inReplyTo":"20210204210556.25242-1-avarab@gmail.com","subject":"[PATCH v2 04/10] grep/pcre2: prepare to add debugging to pcre2_malloc()","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-02-18T00:07:22Z","receivedAt":"2021-02-18T00:08:36Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Change pcre2_malloc() in a way that'll make it easier for a debugging\nfprintf() to spew out the allocated pointer.\n\nThis doesn't introduce any functional change, it just makes a\nsubsequent commit's diff easier to read. Changes code added in\n513f2b0bbd4 (grep: make PCRE2 aware of custom allocator, 2019-10-16).\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n grep.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/grep.c b/grep.c\nindex b9adcd83e7a..f96d86c9293 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -45,7 +45,8 @@ static pcre2_general_context *pcre2_global_context;\n \n static void *pcre2_malloc(PCRE2_SIZE size, MAYBE_UNUSED void *memory_data)\n {\n-\treturn malloc(size);\n+\tvoid *pointer = malloc(size);\n+\treturn pointer;\n }\n \n static void pcre2_free(void *pointer, MAYBE_UNUSED void *memory_data)\n-- \n2.30.0.284.gd98b1dd5eaa7\n\n"},{"id":"417255","messageId":"20210218000728.13995-3-avarab@gmail.com","threadId":"52046","inReplyTo":"20210204210556.25242-1-avarab@gmail.com","subject":"[PATCH v2 02/10] grep/pcre2: drop needless assignment to NULL","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-02-18T00:07:20Z","receivedAt":"2021-02-18T00:08:36Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Remove a redundant assignment of pcre2_compile_context dating back to\nmy 94da9193a6e (grep: add support for PCRE v2, 2017-06-01).\n\nIn create_grep_pat() we xcalloc() the \"grep_pat\" struct, so there's no\nneed to NULL out individual members here.\n\nI think this was probably something left over from an earlier\ndevelopment version of mine.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n grep.c | 2 --\n 1 file changed, 2 deletions(-)\n\ndiff --git a/grep.c b/grep.c\nindex 816e23f17ef..f27c5de7f56 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -373,8 +373,6 @@ static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt\n \tint patinforet;\n \tsize_t jitsizearg;\n \n-\tp->pcre2_compile_context = NULL;\n-\n \t/* pcre2_global_context is initialized in append_grep_pattern */\n \tif (opt->ignore_case) {\n \t\tif (!opt->ignore_locale && has_non_ascii(p->pattern)) {\n-- \n2.30.0.284.gd98b1dd5eaa7\n\n"},{"id":"417256","messageId":"20210218000728.13995-2-avarab@gmail.com","threadId":"52046","inReplyTo":"20210204210556.25242-1-avarab@gmail.com","subject":"[PATCH v2 01/10] grep/pcre2: drop needless assignment + assert() on opt->pcre2","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-02-18T00:07:19Z","receivedAt":"2021-02-18T00:08:36Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Drop an assignment added in b65abcafc7a (grep: use PCRE v2 for\noptimized fixed-string search, 2019-07-01) and the overly cautious\nassert() I added in 94da9193a6e (grep: add support for PCRE v2,\n2017-06-01).\n\nThere was never a good reason for this, it's just a relic from when I\ninitially wrote the PCREv2 support. We're not going to have confusion\nabout compile_pcre2_pattern() being called when it shouldn't just\nbecause we forgot to cargo-cult this opt->pcre2 option.\n\nFurthermore the \"struct grep_opt\" is (mostly) used for the options the\nuser supplied, let's avoid the pattern of needlessly assigning to it.\n\nWith my recent removal of the PCREv1 backend in 7599730b7e2 (Remove\nsupport for v1 of the PCRE library, 2021-01-24) there's even less\nconfusion around what we call where in these codepaths, which is one\nmore reason to remove this.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n grep.c | 3 ---\n 1 file changed, 3 deletions(-)\n\ndiff --git a/grep.c b/grep.c\nindex aabfaaa4c32..816e23f17ef 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -373,8 +373,6 @@ static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt\n \tint patinforet;\n \tsize_t jitsizearg;\n \n-\tassert(opt->pcre2);\n-\n \tp->pcre2_compile_context = NULL;\n \n \t/* pcre2_global_context is initialized in append_grep_pattern */\n@@ -555,7 +553,6 @@ static void compile_regexp(struct grep_pat *p, struct grep_opt *opt)\n #endif\n \tif (p->fixed || p->is_fixed) {\n #ifdef USE_LIBPCRE2\n-\t\topt->pcre2 = 1;\n \t\tif (p->is_fixed) {\n \t\t\tcompile_pcre2_pattern(p, opt);\n \t\t} else {\n-- \n2.30.0.284.gd98b1dd5eaa7\n\n"},{"id":"417257","messageId":"20210218000728.13995-6-avarab@gmail.com","threadId":"52046","inReplyTo":"20210204210556.25242-1-avarab@gmail.com","subject":"[PATCH v2 05/10] grep/pcre2: add GREP_PCRE2_DEBUG_MALLOC debug mode","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-02-18T00:07:23Z","receivedAt":"2021-02-18T00:09:00Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Add optional printing of PCREv2 allocations to stderr for a developer\nwho manually changes the GREP_PCRE2_DEBUG_MALLOC definition to \"1\".\n\nYou need to manually change the definition in the source file similar\nto the DEBUG_MAILMAP, there's no Makefile knob for this.\n\nThis will be referenced a subsequent commit, and is generally useful\nto manually see what's going on with PCREv2 allocations while working\non that code.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n grep.c | 10 ++++++++++\n 1 file changed, 10 insertions(+)\n\ndiff --git a/grep.c b/grep.c\nindex f96d86c9293..7d262a23d88 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -42,15 +42,25 @@ static struct grep_opt grep_defaults = {\n \n #ifdef USE_LIBPCRE2\n static pcre2_general_context *pcre2_global_context;\n+#define GREP_PCRE2_DEBUG_MALLOC 0\n \n static void *pcre2_malloc(PCRE2_SIZE size, MAYBE_UNUSED void *memory_data)\n {\n \tvoid *pointer = malloc(size);\n+#if GREP_PCRE2_DEBUG_MALLOC\n+\tstatic int count = 1;\n+\tfprintf(stderr, \"PCRE2:%p -> #%02d: alloc(%lu)\\n\", pointer, count++, size);\n+#endif\n \treturn pointer;\n }\n \n static void pcre2_free(void *pointer, MAYBE_UNUSED void *memory_data)\n {\n+#if GREP_PCRE2_DEBUG_MALLOC\n+\tstatic int count = 1;\n+\tif (pointer)\n+\t\tfprintf(stderr, \"PCRE2:%p -> #%02d: free()\\n\", pointer, count++);\n+#endif\n \tfree(pointer);\n }\n #endif\n-- \n2.30.0.284.gd98b1dd5eaa7\n\n"},{"id":"417258","messageId":"20210218000728.13995-7-avarab@gmail.com","threadId":"52046","inReplyTo":"20210204210556.25242-1-avarab@gmail.com","subject":"[PATCH v2 06/10] grep/pcre2: use compile-time PCREv2 version test","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-02-18T00:07:24Z","receivedAt":"2021-02-18T00:09:01Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Replace a use of pcre2_config(PCRE2_CONFIG_VERSION, ...) which I added\nin 95ca1f987ed (grep/pcre2: better support invalid UTF-8 haystacks,\n2021-01-24) with the same test done at compile-time.\n\nIt might be cuter to do this at runtime since we don't have to do the\n\"major >= 11 || (major >= 10 && ...)\" test. But in the next commit\nwe'll add another version comparison that absolutely needs to be done\nat compile-time, so we're better of being consistent across the board.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n grep.c | 18 ++++--------------\n grep.h | 3 +++\n 2 files changed, 7 insertions(+), 14 deletions(-)\n\ndiff --git a/grep.c b/grep.c\nindex 7d262a23d88..e58044474dc 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -400,21 +400,11 @@ static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt\n \t !(!opt->ignore_case && (p->fixed || p->is_fixed)))\n \t\toptions |= (PCRE2_UTF | PCRE2_MATCH_INVALID_UTF);\n \n+#ifdef GIT_PCRE2_VERSION_10_36_OR_HIGHER\n \t/* Work around https://bugs.exim.org/show_bug.cgi?id=2642 fixed in 10.36 */\n-\tif (PCRE2_MATCH_INVALID_UTF && options & (PCRE2_UTF | PCRE2_CASELESS)) {\n-\t\tstruct strbuf buf;\n-\t\tint len;\n-\t\tint err;\n-\n-\t\tif ((len = pcre2_config(PCRE2_CONFIG_VERSION, NULL)) < 0)\n-\t\t\tBUG(\"pcre2_config(..., NULL) failed: %d\", len);\n-\t\tstrbuf_init(&buf, len + 1);\n-\t\tif ((err = pcre2_config(PCRE2_CONFIG_VERSION, buf.buf)) < 0)\n-\t\t\tBUG(\"pcre2_config(..., buf.buf) failed: %d\", err);\n-\t\tif (versioncmp(buf.buf, \"10.36\") < 0)\n-\t\t\toptions |= PCRE2_NO_START_OPTIMIZE;\n-\t\tstrbuf_release(&buf);\n-\t}\n+\tif (PCRE2_MATCH_INVALID_UTF && options & (PCRE2_UTF | PCRE2_CASELESS))\n+\t\toptions |= PCRE2_NO_START_OPTIMIZE;\n+#endif\n \n \tp->pcre2_pattern = pcre2_compile((PCRE2_SPTR)p->pattern,\n \t\t\t\t\t p->patternlen, options, &error, &erroffset,\ndiff --git a/grep.h b/grep.h\nindex ae89d6254b3..54e52042cb9 100644\n--- a/grep.h\n+++ b/grep.h\n@@ -4,6 +4,9 @@\n #ifdef USE_LIBPCRE2\n #define PCRE2_CODE_UNIT_WIDTH 8\n #include \n+#if (PCRE2_MAJOR >= 10 && PCRE2_MINOR >= 36) || PCRE2_MAJOR >= 11\n+#define GIT_PCRE2_VERSION_10_36_OR_HIGHER\n+#endif\n #else\n typedef int pcre2_code;\n typedef int pcre2_match_data;\n-- \n2.30.0.284.gd98b1dd5eaa7\n\n"},{"id":"417259","messageId":"20210218000728.13995-8-avarab@gmail.com","threadId":"52046","inReplyTo":"20210204210556.25242-1-avarab@gmail.com","subject":"[PATCH v2 07/10] grep/pcre2: use pcre2_maketables_free() function","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-02-18T00:07:25Z","receivedAt":"2021-02-18T00:09:03Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Make use of the pcre2_maketables_free() function to free the memory\nallocated by pcre2_maketables().\n\nAt first sight it's strange that 10da030ab75 (grep: avoid leak of\nchartables in PCRE2, 2019-10-16) which added the free() call here\ndoesn't make use of the pcre2_free() the author introduced in the\npreceding commit in 513f2b0bbd4 (grep: make PCRE2 aware of custom\nallocator, 2019-10-16).\n\nThe reason is that at the time the function didn't exist. It was first\nintroduced in PCREv2 version 10.34, released on 2019-11-21.\n\nLet's make use of it behind a macro. I don't think this matters for\nanything to do with custom allocators, but it makes our use of PCREv2\nmore discoverable.\n\nAt some distant point in the future we'll be able to drop the version\nguard, as nobody will be running a version older than 10.34.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n grep.c | 4 ++++\n grep.h | 3 +++\n 2 files changed, 7 insertions(+)\n\ndiff --git a/grep.c b/grep.c\nindex e58044474dc..c63dbff4b24 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -490,7 +490,11 @@ static void free_pcre2_pattern(struct grep_pat *p)\n \tpcre2_compile_context_free(p->pcre2_compile_context);\n \tpcre2_code_free(p->pcre2_pattern);\n \tpcre2_match_data_free(p->pcre2_match_data);\n+#ifdef GIT_PCRE2_VERSION_10_34_OR_HIGHER\n+\tpcre2_maketables_free(pcre2_global_context, p->pcre2_tables);\n+#else\n \tfree((void *)p->pcre2_tables);\n+#endif\n }\n #else /* !USE_LIBPCRE2 */\n static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt)\ndiff --git a/grep.h b/grep.h\nindex 54e52042cb9..64666e92049 100644\n--- a/grep.h\n+++ b/grep.h\n@@ -7,6 +7,9 @@\n #if (PCRE2_MAJOR >= 10 && PCRE2_MINOR >= 36) || PCRE2_MAJOR >= 11\n #define GIT_PCRE2_VERSION_10_36_OR_HIGHER\n #endif\n+#if (PCRE2_MAJOR >= 10 && PCRE2_MINOR >= 34) || PCRE2_MAJOR >= 11\n+#define GIT_PCRE2_VERSION_10_34_OR_HIGHER\n+#endif\n #else\n typedef int pcre2_code;\n typedef int pcre2_match_data;\n-- \n2.30.0.284.gd98b1dd5eaa7\n\n"},{"id":"417260","messageId":"20210218000728.13995-9-avarab@gmail.com","threadId":"52046","inReplyTo":"20210204210556.25242-1-avarab@gmail.com","subject":"[PATCH v2 08/10] grep/pcre2: actually make pcre2 use custom allocator","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-02-18T00:07:26Z","receivedAt":"2021-02-18T00:09:05Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Continue work started in 513f2b0bbd4 (grep: make PCRE2 aware of custom\nallocator, 2019-10-16) and make PCREv2 use our pcre2_{malloc,free}().\nfunctions for allocation. We'll now use it for all PCREv2 allocations.\n\nThe reason 513f2b0bbd4 worked as a bugfix for the USE_NED_ALLOCATOR\nissue is because it targeted the allocation freed via free(), as\nopposed to by a pcre2_*free() function. I.e. the pcre2_maketables()\nand pcre2_maketables_free() pair.\n\nFor most of the rest we continued allocating with stock malloc()\ninside PCREv2 itself, but didn't segfault because we'd use its\ncorresponding free().\n\nIn a preceding commit of mine I changed the free() to\npcre2_maketables_free() on versions of PCREv2 10.34 and newer. So as\nfar as fixing the segfault goes we could revert 513f2b0bbd4. But then\nwe wouldn't use the desired allocator, let's just use it instead.\n\nBefore this patch we'd on e.g.:\n\n grep --threads=1 -iP æ.*var.*xyz\n\nOnly use pcre2_{malloc,free}() for 2 malloc() calls and 2\ncorresponding free() calls. Now it's 12 calls to each. This can be\nobserved with the GREP_PCRE2_DEBUG_MALLOC debug mode.\n\nReading the history of how this bug got introduced it wasn't present\nin Johannes's original patch[1] to fix the issue.\n\nMy reading of that thread is that the approach the follow-up patches\nto Johannes's original pursued were based on misunderstanding of how\nthe PCREv2 API works. In particular this part of [2]:\n\n \"most of the time (like when using UTF-8) the chartable (and\n therefore the global context) is not needed (even when using\n alternate allocators)\"\n\nThat's simply not how PCREv2 memory allocation works. It's easy to see\nhow the misunderstanding came about. It's because (as noted above) the\nissue was noticed because of our use of free() in our own grep.c for\nfreeing the memory allocated by pcre2_maketables().\n\nThus the misunderstanding that PCREv2's compile context is something\nonly needed for pcre2_maketables(), and e.g. an aborted earlier\nattempt[3] to only set it up when we ourselves called\npcre2_maketables().\n\nThat's not what PCREv2's compile context is. To quote PCREv2's\ndocumentation:\n\n \"This context just contains pointers to (and data for) external\n memory management functions that are called from several places in\n the PCRE2 library.\"\n\nThus the failed attempts to go down the route of only creating the\ngeneral context in cases where we ourselves call pcre2_maketables(),\nbefore finally settling on the approach 513f2b0bbd4 took of always\ncreating it, but then mostly not using it.\n\nInstead we should always create it, and then pass the general context\nto those functions that accept it, so that they'll consistently use\nour preferred memory allocation functions.\n\n1. https://lore.kernel.org/git/3397e6797f872aedd18c6d795f4976e1c579514b.1565005867.git.gitgitgadget@gmail.com/\n2. https://lore.kernel.org/git/CAPUEsphMh_ZqcH3M7PXC9jHTfEdQN3mhTAK2JDkdvKBp53YBoA@mail.gmail.com/\n3. https://lore.kernel.org/git/20190806085014.47776-3-carenas@gmail.com/\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n grep.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/grep.c b/grep.c\nindex c63dbff4b24..0116ff5f09f 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -390,7 +390,7 @@ static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt\n \t\t\tif (!pcre2_global_context)\n \t\t\t\tBUG(\"pcre2_global_context uninitialized\");\n \t\t\tp->pcre2_tables = pcre2_maketables(pcre2_global_context);\n-\t\t\tp->pcre2_compile_context = pcre2_compile_context_create(NULL);\n+\t\t\tp->pcre2_compile_context = pcre2_compile_context_create(pcre2_global_context);\n \t\t\tpcre2_set_character_tables(p->pcre2_compile_context,\n \t\t\t\t\t\t\tp->pcre2_tables);\n \t\t}\n@@ -411,7 +411,7 @@ static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt\n \t\t\t\t\t p->pcre2_compile_context);\n \n \tif (p->pcre2_pattern) {\n-\t\tp->pcre2_match_data = pcre2_match_data_create_from_pattern(p->pcre2_pattern, NULL);\n+\t\tp->pcre2_match_data = pcre2_match_data_create_from_pattern(p->pcre2_pattern, pcre2_global_context);\n \t\tif (!p->pcre2_match_data)\n \t\t\tdie(\"Couldn't allocate PCRE2 match data\");\n \t} else {\n-- \n2.30.0.284.gd98b1dd5eaa7\n\n"},{"id":"417261","messageId":"20210218000728.13995-11-avarab@gmail.com","threadId":"52046","inReplyTo":"20210204210556.25242-1-avarab@gmail.com","subject":"[PATCH v2 10/10] grep/pcre2: move definitions of pcre2_{malloc,free}","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-02-18T00:07:28Z","receivedAt":"2021-02-18T00:09:17Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Move the definitions of the pcre2_{malloc,free} functions above the\ncompile_pcre2_pattern() function they're used in.\n\nBefore the preceding commit they used to be needed earlier, but now we\ncan move them to be adjacent to the other PCREv2 functions.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n grep.c | 46 ++++++++++++++++++++++------------------------\n 1 file changed, 22 insertions(+), 24 deletions(-)\n\ndiff --git a/grep.c b/grep.c\nindex 2599f329cd6..636ac48bf07 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -40,30 +40,6 @@ static struct grep_opt grep_defaults = {\n \t.output = std_output,\n };\n \n-#ifdef USE_LIBPCRE2\n-#define GREP_PCRE2_DEBUG_MALLOC 0\n-\n-static void *pcre2_malloc(PCRE2_SIZE size, MAYBE_UNUSED void *memory_data)\n-{\n-\tvoid *pointer = malloc(size);\n-#if GREP_PCRE2_DEBUG_MALLOC\n-\tstatic int count = 1;\n-\tfprintf(stderr, \"PCRE2:%p -> #%02d: alloc(%lu)\\n\", pointer, count++, size);\n-#endif\n-\treturn pointer;\n-}\n-\n-static void pcre2_free(void *pointer, MAYBE_UNUSED void *memory_data)\n-{\n-#if GREP_PCRE2_DEBUG_MALLOC\n-\tstatic int count = 1;\n-\tif (pointer)\n-\t\tfprintf(stderr, \"PCRE2:%p -> #%02d: free()\\n\", pointer, count++);\n-#endif\n-\tfree(pointer);\n-}\n-#endif\n-\n static const char *color_grep_slots[] = {\n \t[GREP_COLOR_CONTEXT]\t = \"context\",\n \t[GREP_COLOR_FILENAME]\t = \"filename\",\n@@ -355,6 +331,28 @@ static int is_fixed(const char *s, size_t len)\n }\n \n #ifdef USE_LIBPCRE2\n+#define GREP_PCRE2_DEBUG_MALLOC 0\n+\n+static void *pcre2_malloc(PCRE2_SIZE size, MAYBE_UNUSED void *memory_data)\n+{\n+\tvoid *pointer = malloc(size);\n+#if GREP_PCRE2_DEBUG_MALLOC\n+\tstatic int count = 1;\n+\tfprintf(stderr, \"PCRE2:%p -> #%02d: alloc(%lu)\\n\", pointer, count++, size);\n+#endif\n+\treturn pointer;\n+}\n+\n+static void pcre2_free(void *pointer, MAYBE_UNUSED void *memory_data)\n+{\n+#if GREP_PCRE2_DEBUG_MALLOC\n+\tstatic int count = 1;\n+\tif (pointer)\n+\t\tfprintf(stderr, \"PCRE2:%p -> #%02d: free()\\n\", pointer, count++);\n+#endif\n+\tfree(pointer);\n+}\n+\n static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt)\n {\n \tint error;\n-- \n2.30.0.284.gd98b1dd5eaa7\n\n"},{"id":"417262","messageId":"20210218000728.13995-10-avarab@gmail.com","threadId":"52046","inReplyTo":"20210204210556.25242-1-avarab@gmail.com","subject":"[PATCH v2 09/10] grep/pcre2: move back to thread-only PCREv2 structures","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-02-18T00:07:27Z","receivedAt":"2021-02-18T00:09:22Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Change the setup of the \"pcre2_general_context\" to happen per-thread\nin compile_pcre2_pattern() instead of in grep_init().\n\nThis change brings it in line with how the rest of the pcre2_* members\nin the grep_pat structure are set up.\n\nAs noted in the preceding commit the approach 513f2b0bbd4 (grep: make\nPCRE2 aware of custom allocator, 2019-10-16) took to allocate the\npcre2_general_context seems to have been initially based on a\nmisunderstanding of how PCREv2 memory allocation works.\n\nThe approach of creating a global context in grep_init() is just added\ncomplexity for almost zero gain. On my system it's 24 bytes saved\nper-thread. For comparison PCREv2 will then go on to allocate at least\na kilobyte for its own thread-local state.\n\nAs noted in 6d423dd542f (grep: don't redundantly compile throwaway\npatterns under threading, 2017-05-25) the grep code is intentionally\nnot trying to micro-optimize allocations by e.g. sharing some PCREv2\nstructures globally, while making others thread-local.\n\nSo let's remove this special case and make all of them thread-local\nagain for simplicity. With this change we could move the\npcre2_{malloc,free} functions around to live closer to their current\nuse. I'm not doing that here to keep this change small, that cleanup\nwill be done in a follow-up commit.\n\nSee also the discussion in 94da9193a6 (grep: add support for PCRE v2,\n2017-06-01) about thread safety, and Johannes's comments[1] to the\neffect that we should be doing what this patch is doing.\n\n1. https://lore.kernel.org/git/nycvar.QRO.7.76.6.1908052120302.46@tvgsbejvaqbjf.bet/\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n builtin/grep.c | 1 -\n grep.c | 41 +++++++++++++++--------------------------\n grep.h | 3 ++-\n 3 files changed, 17 insertions(+), 28 deletions(-)\n\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex 55d06c95130..c69fe993406 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -1175,6 +1175,5 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n \t\trun_pager(&opt, prefix);\n \tclear_pathspec(&pathspec);\n \tfree_grep_patterns(&opt);\n-\tgrep_destroy();\n \treturn !hit;\n }\ndiff --git a/grep.c b/grep.c\nindex 0116ff5f09f..2599f329cd6 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -41,7 +41,6 @@ static struct grep_opt grep_defaults = {\n };\n \n #ifdef USE_LIBPCRE2\n-static pcre2_general_context *pcre2_global_context;\n #define GREP_PCRE2_DEBUG_MALLOC 0\n \n static void *pcre2_malloc(PCRE2_SIZE size, MAYBE_UNUSED void *memory_data)\n@@ -163,20 +162,9 @@ int grep_config(const char *var, const char *value, void *cb)\n * Initialize one instance of grep_opt and copy the\n * default values from the template we read the configuration\n * information in an earlier call to git_config(grep_config).\n- *\n- * If using PCRE, make sure that the library is configured\n- * to use the same allocator as Git (e.g. nedmalloc on Windows).\n- *\n- * Any allocated memory needs to be released in grep_destroy().\n */\n void grep_init(struct grep_opt *opt, struct repository *repo, const char *prefix)\n {\n-#if defined(USE_LIBPCRE2)\n-\tif (!pcre2_global_context)\n-\t\tpcre2_global_context = pcre2_general_context_create(\n-\t\t\t\t\tpcre2_malloc, pcre2_free, NULL);\n-#endif\n-\n \t*opt = grep_defaults;\n \n \topt->repo = repo;\n@@ -186,13 +174,6 @@ void grep_init(struct grep_opt *opt, struct repository *repo, const char *prefix\n \topt->header_tail = &opt->header_list;\n }\n \n-void grep_destroy(void)\n-{\n-#ifdef USE_LIBPCRE2\n-\tpcre2_general_context_free(pcre2_global_context);\n-#endif\n-}\n-\n static void grep_set_pattern_type_option(enum grep_pattern_type pattern_type, struct grep_opt *opt)\n {\n \t/*\n@@ -384,13 +365,20 @@ static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt\n \tint patinforet;\n \tsize_t jitsizearg;\n \n-\t/* pcre2_global_context is initialized in grep_init */\n+\t/*\n+\t * Call pcre2_general_context_create() before calling any\n+\t * other pcre2_*(). It sets up our malloc()/free() functions\n+\t * with which everything else is allocated.\n+\t */\n+\tp->pcre2_general_context = pcre2_general_context_create(\n+\t\tpcre2_malloc, pcre2_free, NULL);\n+\tif (!p->pcre2_general_context)\n+\t\tdie(\"Couldn't allocate PCRE2 general context\");\n+\n \tif (opt->ignore_case) {\n \t\tif (!opt->ignore_locale && has_non_ascii(p->pattern)) {\n-\t\t\tif (!pcre2_global_context)\n-\t\t\t\tBUG(\"pcre2_global_context uninitialized\");\n-\t\t\tp->pcre2_tables = pcre2_maketables(pcre2_global_context);\n-\t\t\tp->pcre2_compile_context = pcre2_compile_context_create(pcre2_global_context);\n+\t\t\tp->pcre2_tables = pcre2_maketables(p->pcre2_general_context);\n+\t\t\tp->pcre2_compile_context = pcre2_compile_context_create(p->pcre2_general_context);\n \t\t\tpcre2_set_character_tables(p->pcre2_compile_context,\n \t\t\t\t\t\t\tp->pcre2_tables);\n \t\t}\n@@ -411,7 +399,7 @@ static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt\n \t\t\t\t\t p->pcre2_compile_context);\n \n \tif (p->pcre2_pattern) {\n-\t\tp->pcre2_match_data = pcre2_match_data_create_from_pattern(p->pcre2_pattern, pcre2_global_context);\n+\t\tp->pcre2_match_data = pcre2_match_data_create_from_pattern(p->pcre2_pattern, p->pcre2_general_context);\n \t\tif (!p->pcre2_match_data)\n \t\t\tdie(\"Couldn't allocate PCRE2 match data\");\n \t} else {\n@@ -491,10 +479,11 @@ static void free_pcre2_pattern(struct grep_pat *p)\n \tpcre2_code_free(p->pcre2_pattern);\n \tpcre2_match_data_free(p->pcre2_match_data);\n #ifdef GIT_PCRE2_VERSION_10_34_OR_HIGHER\n-\tpcre2_maketables_free(pcre2_global_context, p->pcre2_tables);\n+\tpcre2_maketables_free(p->pcre2_general_context, p->pcre2_tables);\n #else\n \tfree((void *)p->pcre2_tables);\n #endif\n+\tpcre2_general_context_free(p->pcre2_general_context);\n }\n #else /* !USE_LIBPCRE2 */\n static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt)\ndiff --git a/grep.h b/grep.h\nindex 64666e92049..72f82b1e302 100644\n--- a/grep.h\n+++ b/grep.h\n@@ -14,6 +14,7 @@\n typedef int pcre2_code;\n typedef int pcre2_match_data;\n typedef int pcre2_compile_context;\n+typedef int pcre2_general_context;\n #endif\n #ifndef PCRE2_MATCH_INVALID_UTF\n /* PCRE2_MATCH_* dummy also with !USE_LIBPCRE2, for test-pcre2-config.c */\n@@ -75,6 +76,7 @@ struct grep_pat {\n \tpcre2_code *pcre2_pattern;\n \tpcre2_match_data *pcre2_match_data;\n \tpcre2_compile_context *pcre2_compile_context;\n+\tpcre2_general_context *pcre2_general_context;\n \tconst uint8_t *pcre2_tables;\n \tuint32_t pcre2_jit_on;\n \tunsigned fixed:1;\n@@ -167,7 +169,6 @@ struct grep_opt {\n \n int grep_config(const char *var, const char *value, void *);\n void grep_init(struct grep_opt *, struct repository *repo, const char *prefix);\n-void grep_destroy(void);\n void grep_commit_pattern_type(enum grep_pattern_type, struct grep_opt *opt);\n \n void append_grep_pat(struct grep_opt *opt, const char *pat, size_t patlen, const char *origin, int no, enum grep_pat_token t);\n-- \n2.30.0.284.gd98b1dd5eaa7\n\n"},{"id":"418177","messageId":"xmqqtuprhj1s.fsf@gitster.c.googlers.com","threadId":"52046","inReplyTo":"20210218000728.13995-7-avarab@gmail.com","subject":"Re: [PATCH v2 06/10] grep/pcre2: use compile-time PCREv2 version test","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-03-04T00:14:07Z","receivedAt":"2021-03-04T00:24:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> +#if (PCRE2_MAJOR >= 10 && PCRE2_MINOR >= 36) || PCRE2_MAJOR >= 11\n> +#define GIT_PCRE2_VERSION_10_36_OR_HIGHER\n> +#endif\n\nThis is not wrong per-se but it does look misleading not to spell it\nas\n\n#if (PCRE2_MAJOR == 10 && PCRE2_MINOR >= 36) || PCRE2_MAJOR >= 11\n\nwhich would convey the intention much clearly.\n\nHint to notice the difference: imagine if it were \"9.37 and later in\n9.X series, 10.36 and later in 10.X series and anything after 11 are\nOK\".\n\nIn other words, the minor version is always tied to a particular\nmajor version and \"major >= X && minor >= Y\" is often a bug, even\nthough in this case it happens to be OK only because 10 and 11 are\nconsecutive.\n\n"},{"id":"418186","messageId":"xmqqlfb3hiku.fsf@gitster.c.googlers.com","threadId":"52046","inReplyTo":"20210218000728.13995-9-avarab@gmail.com","subject":"Re: [PATCH v2 08/10] grep/pcre2: actually make pcre2 use custom allocator","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-03-04T00:24:17Z","receivedAt":"2021-03-04T01:08:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> Continue work started in 513f2b0bbd4 (grep: make PCRE2 aware of custom\n> allocator, 2019-10-16) and make PCREv2 use our pcre2_{malloc,free}().\n> functions for allocation. We'll now use it for all PCREv2 allocations.\n>\n> The reason 513f2b0bbd4 worked as a bugfix for the USE_NED_ALLOCATOR\n> issue is because it targeted the allocation freed via free(), as\n> opposed to by a pcre2_*free() function. I.e. the pcre2_maketables()\n> and pcre2_maketables_free() pair.\n>\n> For most of the rest we continued allocating with stock malloc()\n> inside PCREv2 itself, but didn't segfault because we'd use its\n> corresponding free().\n>\n> In a preceding commit of mine I changed the free() to\n> pcre2_maketables_free() on versions of PCREv2 10.34 and newer. So as\n> far as fixing the segfault goes ...\n\nWait, wait. So, because of the previous step, we would start\nsegfaulting and we need to fix that breakage, which is the reason\nwhy this commit exists?\n\nIf so, ...\n\n> we could revert 513f2b0bbd4. But then\n> we wouldn't use the desired allocator, let's just use it instead.\n\n... I agree with the conclusion that both the previous step and this\nstep are needed and better than a reversion of 513f2b0b (grep: make\nPCRE2 aware of custom allocator, 2019-10-16) and the previou step.\n\nBut even then, it feels somewhat backwards. Shouldn't this step\ncome first, so that we would be using a matching alloc/free pair,\nand then do the previous step?\n\n> Instead we should always create it, and then pass the general context\n> to those functions that accept it, so that they'll consistently use\n> our preferred memory allocation functions.\n\nThanks.\n"},{"id":"418187","messageId":"xmqq8s73hi4b.fsf@gitster.c.googlers.com","threadId":"52046","inReplyTo":"20210218000728.13995-1-avarab@gmail.com","subject":"Re: [PATCH v2 00/10] grep/pcre2: memory allocation fixes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-03-04T00:34:12Z","receivedAt":"2021-03-04T01:08:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> Now based on \"master\", when I sent v1[1] it dependen on other\n> in-flight topics of mine.\n\nThis has been in \"Needs review\" state for too long, so I looked at\nthem myself. Aside from a few minor issues, they all made sense to\nme.\n\nThanks.\n\n\n"},{"id":"418188","messageId":"xmqqczwfhidz.fsf@gitster.c.googlers.com","threadId":"52046","inReplyTo":"20210218000728.13995-11-avarab@gmail.com","subject":"Re: [PATCH v2 10/10] grep/pcre2: move definitions of pcre2_{malloc,free}","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-03-04T00:28:24Z","receivedAt":"2021-03-04T01:08:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> Move the definitions of the pcre2_{malloc,free} functions above the\n> compile_pcre2_pattern() function they're used in.\n>\n> Before the preceding commit they used to be needed earlier, but now we\n> can move them to be adjacent to the other PCREv2 functions.\n\nYes, much nicer.\n\n"},{"id":"418194","messageId":"xmqqh7lrhies.fsf@gitster.c.googlers.com","threadId":"52046","inReplyTo":"20210218000728.13995-10-avarab@gmail.com","subject":"Re: [PATCH v2 09/10] grep/pcre2: move back to thread-only PCREv2 structures","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-03-04T00:27:55Z","receivedAt":"2021-03-04T01:09:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> So let's remove this special case and make all of them thread-local\n> again for simplicity. With this change we could move the\n> pcre2_{malloc,free} functions around to live closer to their current\n> use. I'm not doing that here to keep this change small, that cleanup\n> will be done in a follow-up commit.\n\nNice.\n\n"},{"id":"418195","messageId":"xmqqpn0fhiya.fsf@gitster.c.googlers.com","threadId":"52046","inReplyTo":"20210204210556.25242-8-avarab@gmail.com","subject":"Re: [PATCH 07/10] grep/pcre2: use pcre2_maketables_free() function","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-03-04T00:16:13Z","receivedAt":"2021-03-04T01:09:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> Make use of the pcre2_maketables_free() function to free the memory\n> allocated by pcre2_maketables(). At first sight it's strange that\n> 10da030ab75 (grep: avoid leak of chartables in PCRE2, 2019-10-16)\n> which added the free() call here doesn't make use of the pcre2_free()\n> the author introduced in the preceding commit in 513f2b0bbd4 (grep:\n> make PCRE2 aware of custom allocator, 2019-10-16).\n>\n> The reason is that at the time the function didn't exist. It was first\n> introduced in PCREv2 version 10.34, released on 2019-11-21.\n>\n> Let's make use of it behind a macro. I don't think this matters for\n> anything to do with custom allocators, but it makes our use of PCREv2\n> more discoverable. At some distant point in the future we'll be able\n> to drop the version guard, as nobody will be running a version older\n> than 10.34.\n\nOK. The same comment about the macro that happens to be OK only\nbecause 10 and 11 are consecutive applies here.\n\n>\n> Signed-off-by: Ævar Arnfjörð Bjarmason \n> ---\n> grep.c | 4 ++++\n> grep.h | 3 +++\n> 2 files changed, 7 insertions(+)\n>\n> diff --git a/grep.c b/grep.c\n> index e58044474d..c63dbff4b2 100644\n> --- a/grep.c\n> +++ b/grep.c\n> @@ -490,7 +490,11 @@ static void free_pcre2_pattern(struct grep_pat *p)\n> \tpcre2_compile_context_free(p->pcre2_compile_context);\n> \tpcre2_code_free(p->pcre2_pattern);\n> \tpcre2_match_data_free(p->pcre2_match_data);\n> +#ifdef GIT_PCRE2_VERSION_10_34_OR_HIGHER\n> +\tpcre2_maketables_free(pcre2_global_context, p->pcre2_tables);\n> +#else\n> \tfree((void *)p->pcre2_tables);\n> +#endif\n> }\n> #else /* !USE_LIBPCRE2 */\n> static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt)\n> diff --git a/grep.h b/grep.h\n> index 54e52042cb..64666e9204 100644\n> --- a/grep.h\n> +++ b/grep.h\n> @@ -7,6 +7,9 @@\n> #if (PCRE2_MAJOR >= 10 && PCRE2_MINOR >= 36) || PCRE2_MAJOR >= 11\n> #define GIT_PCRE2_VERSION_10_36_OR_HIGHER\n> #endif\n> +#if (PCRE2_MAJOR >= 10 && PCRE2_MINOR >= 34) || PCRE2_MAJOR >= 11\n> +#define GIT_PCRE2_VERSION_10_34_OR_HIGHER\n> +#endif\n> #else\n> typedef int pcre2_code;\n> typedef int pcre2_match_data;\n"}]}