[2/3] grep: make PCRE2 aware of custom allocator
diff mbox series

Message ID 20190806085014.47776-3-carenas@gmail.com
State New
Headers show
Series
  • grep: no leaks (WIP)
Related show

Commit Message

Carlo Marcelo Arenas Belón Aug. 6, 2019, 8:50 a.m. UTC
Most of the code stolen from[1] to easy on comparison and including
the deficiency of setting the global context even for patterns that
won't need it.

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

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

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

[1] https://public-inbox.org/git/pull.306.git.gitgitgadget@gmail.com/

Suggested-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin/grep.c |  1 +
 grep.c         | 31 +++++++++++++++++++++++++++++--
 grep.h         |  1 +
 3 files changed, 31 insertions(+), 2 deletions(-)

Comments

Johannes Schindelin Aug. 8, 2019, 1:56 p.m. UTC | #1
Hi Carlo,

On Tue, 6 Aug 2019, Carlo Marcelo Arenas Belón wrote:

> Most of the code stolen from[1] to easy on comparison and including
> the deficiency of setting the global context even for patterns that
> won't need it.
>
> Ideally, the call from grep_init could be moved to a place where it
> could be set without needing a lock and at least with this approach
> we have a place to clear it (which is obviously missing more callers,
> but at least shows how it will look for the grep subcommand)
>
> I had also dropped most other users of the global context in a failed
> attempt to make the change smaller, but also to keep the current
> behaviour so that we could see the effect of enabling NED for PCRE2
> more clearly.
>
> Sadly, that will likely require a Windows box, as NED (at least our
> version) is horribly broken in macOS (maybe it wasn't 64 bit clean)
> and in Linux builds, but I can't reproduce your crasher and it is
> most likely slower than the system malloc.
>
> [1] https://public-inbox.org/git/pull.306.git.gitgitgadget@gmail.com/
>
> Suggested-by: Johannes Schindelin <johannes.schindelin@gmx.de>

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

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

Ciao,
Dscho

> ---
>  builtin/grep.c |  1 +
>  grep.c         | 31 +++++++++++++++++++++++++++++--
>  grep.h         |  1 +
>  3 files changed, 31 insertions(+), 2 deletions(-)
>
> diff --git a/builtin/grep.c b/builtin/grep.c
> index 560051784e..e49c20df60 100644
> --- a/builtin/grep.c
> +++ b/builtin/grep.c
> @@ -1145,5 +1145,6 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
>  		run_pager(&opt, prefix);
>  	clear_pathspec(&pathspec);
>  	free_grep_patterns(&opt);
> +	grep_destroy();
>  	return !hit;
>  }
> diff --git a/grep.c b/grep.c
> index 0154998695..e748a6d68c 100644
> --- a/grep.c
> +++ b/grep.c
> @@ -16,6 +16,20 @@ static int grep_source_is_binary(struct grep_source *gs,
>
>  static struct grep_opt grep_defaults;
>
> +#ifdef USE_LIBPCRE2
> +static pcre2_general_context *pcre2_global_context;
> +
> +static void *pcre2_malloc(PCRE2_SIZE size, void *memory_data)
> +{
> +	return malloc(size);
> +}
> +
> +static void pcre2_free(void *pointer, void *memory_data)
> +{
> +	return free(pointer);
> +}
> +#endif
> +
>  static const char *color_grep_slots[] = {
>  	[GREP_COLOR_CONTEXT]	    = "context",
>  	[GREP_COLOR_FILENAME]	    = "filename",
> @@ -153,6 +167,7 @@ int grep_config(const char *var, const char *value, void *cb)
>   *
>   * If using PCRE make sure that the library is configured
>   * to use the right allocator (ex: NED)
> + * if any object is created it should be cleaned up in grep_destroy()
>   */
>  void grep_init(struct grep_opt *opt, struct repository *repo, const char *prefix)
>  {
> @@ -164,6 +179,10 @@ void grep_init(struct grep_opt *opt, struct repository *repo, const char *prefix
>  	pcre_malloc = malloc;
>  	pcre_free = free;
>  #endif
> +#ifdef USE_LIBPCRE2
> +	pcre2_global_context = pcre2_general_context_create(pcre2_malloc,
> +							pcre2_free, NULL);
> +#endif
>  #endif
>
>  	memset(opt, 0, sizeof(*opt));
> @@ -188,6 +207,13 @@ void grep_init(struct grep_opt *opt, struct repository *repo, const char *prefix
>  		color_set(opt->colors[i], def->colors[i]);
>  }
>
> +void grep_destroy(void)
> +{
> +#ifdef USE_LIBPCRE2
> +	pcre2_general_context_free(pcre2_global_context);
> +#endif
> +}
> +
>  static void grep_set_pattern_type_option(enum grep_pattern_type pattern_type, struct grep_opt *opt)
>  {
>  	/*
> @@ -509,7 +535,7 @@ static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt
>
>  	if (opt->ignore_case) {
>  		if (has_non_ascii(p->pattern)) {
> -			character_tables = pcre2_maketables(NULL);
> +			character_tables = pcre2_maketables(pcre2_global_context);
>  			p->pcre2_compile_context = pcre2_compile_context_create(NULL);
>  			pcre2_set_character_tables(p->pcre2_compile_context, character_tables);
>  		}
> @@ -560,7 +586,8 @@ static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt
>  			return;
>  		}
>
> -		p->pcre2_jit_stack = pcre2_jit_stack_create(1, 1024 * 1024, NULL);
> +		p->pcre2_jit_stack = pcre2_jit_stack_create(1, 1024 * 1024,
> +					pcre2_global_context);
>  		if (!p->pcre2_jit_stack)
>  			die("Couldn't allocate PCRE2 JIT stack");
>  		p->pcre2_match_context = pcre2_match_context_create(NULL);
> diff --git a/grep.h b/grep.h
> index 1875880f37..526c2db9ef 100644
> --- a/grep.h
> +++ b/grep.h
> @@ -189,6 +189,7 @@ struct grep_opt {
>  void init_grep_defaults(struct repository *);
>  int grep_config(const char *var, const char *value, void *);
>  void grep_init(struct grep_opt *, struct repository *repo, const char *prefix);
> +void grep_destroy(void);
>  void grep_commit_pattern_type(enum grep_pattern_type, struct grep_opt *opt);
>
>  void append_grep_pat(struct grep_opt *opt, const char *pat, size_t patlen, const char *origin, int no, enum grep_pat_token t);
> --
> 2.23.0.rc1
>
>
Carlo Marcelo Arenas Belón Aug. 8, 2019, 2:32 p.m. UTC | #2
On Thu, Aug 8, 2019 at 6:57 AM Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> On Tue, 6 Aug 2019, Carlo Marcelo Arenas Belón wrote:
> > Suggested-by: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> Actually not so much suggested by me, as your patch still causes
> crashes (mine didn't):

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

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

Carlo

[1] https://public-inbox.org/git/20190807213945.10464-3-carenas@gmail.com/
[2] https://public-inbox.org/git/20190807095322.8988-1-carenas@gmail.com/

Patch
diff mbox series

diff --git a/builtin/grep.c b/builtin/grep.c
index 560051784e..e49c20df60 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -1145,5 +1145,6 @@  int cmd_grep(int argc, const char **argv, const char *prefix)
 		run_pager(&opt, prefix);
 	clear_pathspec(&pathspec);
 	free_grep_patterns(&opt);
+	grep_destroy();
 	return !hit;
 }
diff --git a/grep.c b/grep.c
index 0154998695..e748a6d68c 100644
--- a/grep.c
+++ b/grep.c
@@ -16,6 +16,20 @@  static int grep_source_is_binary(struct grep_source *gs,
 
 static struct grep_opt grep_defaults;
 
+#ifdef USE_LIBPCRE2
+static pcre2_general_context *pcre2_global_context;
+
+static void *pcre2_malloc(PCRE2_SIZE size, void *memory_data)
+{
+	return malloc(size);
+}
+
+static void pcre2_free(void *pointer, void *memory_data)
+{
+	return free(pointer);
+}
+#endif
+
 static const char *color_grep_slots[] = {
 	[GREP_COLOR_CONTEXT]	    = "context",
 	[GREP_COLOR_FILENAME]	    = "filename",
@@ -153,6 +167,7 @@  int grep_config(const char *var, const char *value, void *cb)
  *
  * If using PCRE make sure that the library is configured
  * to use the right allocator (ex: NED)
+ * if any object is created it should be cleaned up in grep_destroy()
  */
 void grep_init(struct grep_opt *opt, struct repository *repo, const char *prefix)
 {
@@ -164,6 +179,10 @@  void grep_init(struct grep_opt *opt, struct repository *repo, const char *prefix
 	pcre_malloc = malloc;
 	pcre_free = free;
 #endif
+#ifdef USE_LIBPCRE2
+	pcre2_global_context = pcre2_general_context_create(pcre2_malloc,
+							pcre2_free, NULL);
+#endif
 #endif
 
 	memset(opt, 0, sizeof(*opt));
@@ -188,6 +207,13 @@  void grep_init(struct grep_opt *opt, struct repository *repo, const char *prefix
 		color_set(opt->colors[i], def->colors[i]);
 }
 
+void grep_destroy(void)
+{
+#ifdef USE_LIBPCRE2
+	pcre2_general_context_free(pcre2_global_context);
+#endif
+}
+
 static void grep_set_pattern_type_option(enum grep_pattern_type pattern_type, struct grep_opt *opt)
 {
 	/*
@@ -509,7 +535,7 @@  static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt
 
 	if (opt->ignore_case) {
 		if (has_non_ascii(p->pattern)) {
-			character_tables = pcre2_maketables(NULL);
+			character_tables = pcre2_maketables(pcre2_global_context);
 			p->pcre2_compile_context = pcre2_compile_context_create(NULL);
 			pcre2_set_character_tables(p->pcre2_compile_context, character_tables);
 		}
@@ -560,7 +586,8 @@  static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt
 			return;
 		}
 
-		p->pcre2_jit_stack = pcre2_jit_stack_create(1, 1024 * 1024, NULL);
+		p->pcre2_jit_stack = pcre2_jit_stack_create(1, 1024 * 1024,
+					pcre2_global_context);
 		if (!p->pcre2_jit_stack)
 			die("Couldn't allocate PCRE2 JIT stack");
 		p->pcre2_match_context = pcre2_match_context_create(NULL);
diff --git a/grep.h b/grep.h
index 1875880f37..526c2db9ef 100644
--- a/grep.h
+++ b/grep.h
@@ -189,6 +189,7 @@  struct grep_opt {
 void init_grep_defaults(struct repository *);
 int grep_config(const char *var, const char *value, void *);
 void grep_init(struct grep_opt *, struct repository *repo, const char *prefix);
+void grep_destroy(void);
 void grep_commit_pattern_type(enum grep_pattern_type, struct grep_opt *opt);
 
 void append_grep_pat(struct grep_opt *opt, const char *pat, size_t patlen, const char *origin, int no, enum grep_pat_token t);