[1/1] pcre2: allow overriding the system allocator

Commit Message

Linus Arver via GitGitGadget Aug. 5, 2019, 11:51 a.m. UTC

From: Johannes Schindelin <[e-mail]>

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

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

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

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

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

Signed-off-by: Johannes Schindelin <[e-mail]>
---
 grep.c | 31 +++++++++++++++++++++++++++----
 1 file changed, 27 insertions(+), 4 deletions(-)

Comments

Carlo Marcelo Arenas Belón Aug. 5, 2019, 4:19 p.m. UTC | #1

On Mon, Aug 5, 2019 at 4:51 AM Johannes Schindelin via GitGitGadget
<[e-mail]> wrote:
>
> Since 7d3bf769994 (grep: avoid leak of chartables in PCRE2, 2019-08-01),
> we try to release the UTF-8 tables obtained via `pcre2_maketables()`. To
> do that, we use the function `free()`. That is all fine and dandy as
> long as that refers to the system allocator.

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

Presume git in windows only builds against PCRE2?

LGTM except from the suggestion below that might make the code more "standard"
and probably be a good base for a similar PCRE1 fix
>
> +static pcre2_general_context *get_pcre2_context(void)
> +{
> +       static pcre2_general_context *context;
> +
> +       if (!context)
> +               context = pcre2_general_context_create(pcre2_malloc,
> +                                                      pcre2_free, NULL);
> +
> +       return context;
> +}

instead of using a static variable inside this helper function it
might be better to use
one extra field inside the (struct grep_pat *p), where all other
variables are kept

Additionally to being more consistent will avoid creating the global
context for the
most common case (when the locale is either C/POSIX or UTF-8) and therefore
have a smaller impact on performance.

Carlo

Carlo Marcelo Arenas Belón Aug. 5, 2019, 4:27 p.m. UTC | #2

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

Carlo

Johannes Schindelin Aug. 5, 2019, 7:26 p.m. UTC | #3

Hi Carlo,

On Mon, 5 Aug 2019, Carlo Arenas wrote:

> On Mon, Aug 5, 2019 at 4:51 AM Johannes Schindelin via GitGitGadget
> <[e-mail]> wrote:
> >
> > Since 7d3bf769994 (grep: avoid leak of chartables in PCRE2, 2019-08-01),
> > we try to release the UTF-8 tables obtained via `pcre2_maketables()`. To
> > do that, we use the function `free()`. That is all fine and dandy as
> > long as that refers to the system allocator.
>
> Sorry; I should have thought of this, but assumed was safe since it
> would be broken
> the same way with PCRE1.
>
> Presume git in windows only builds against PCRE2?

That's right, we only build against PCRE2.

> LGTM except from the suggestion below that might make the code more "standard"
> and probably be a good base for a similar PCRE1 fix
> >
> > +static pcre2_general_context *get_pcre2_context(void)
> > +{
> > +       static pcre2_general_context *context;
> > +
> > +       if (!context)
> > +               context = pcre2_general_context_create(pcre2_malloc,
> > +                                                      pcre2_free, NULL);
> > +
> > +       return context;
> > +}
>
> instead of using a static variable inside this helper function it
> might be better to use one extra field inside the (struct grep_pat
> *p), where all other variables are kept

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

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

> Additionally to being more consistent will avoid creating the global
> context for the most common case (when the locale is either C/POSIX or
> UTF-8) and therefore have a smaller impact on performance.

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

Ciao,
Dscho

Johannes Schindelin Aug. 5, 2019, 7:32 p.m. UTC | #4

Hi Carlo,

On Mon, 5 Aug 2019, Carlo Arenas wrote:

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

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

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

Ciao,
Dscho

Junio C Hamano Aug. 5, 2019, 9:53 p.m. UTC | #5

Carlo Arenas <[e-mail]> writes:

> LGTM except from the suggestion below that might make the code more "standard"
> and probably be a good base for a similar PCRE1 fix
>>
>> +static pcre2_general_context *get_pcre2_context(void)
>> +{
>> +       static pcre2_general_context *context;
>> +
>> +       if (!context)
>> +               context = pcre2_general_context_create(pcre2_malloc,
>> +                                                      pcre2_free, NULL);
>> +
>> +       return context;
>> +}
>
> instead of using a static variable inside this helper function it
> might be better to use
> one extra field inside the (struct grep_pat *p), where all other
> variables are kept
>
> Additionally to being more consistent will avoid creating the global

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

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

> context for the
> most common case (when the locale is either C/POSIX or UTF-8) and therefore
> have a smaller impact on performance.

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

Carlo Marcelo Arenas Belón Aug. 6, 2019, 6:24 a.m. UTC | #6

On Mon, Aug 5, 2019 at 2:53 PM Junio C Hamano <[e-mail]> wrote:
>
> Carlo Arenas <[e-mail]> writes:
>
> > LGTM except from the suggestion below that might make the code more "standard"
> > and probably be a good base for a similar PCRE1 fix
> >>
> >> +static pcre2_general_context *get_pcre2_context(void)
> >> +{
> >> +       static pcre2_general_context *context;
> >> +
> >> +       if (!context)
> >> +               context = pcre2_general_context_create(pcre2_malloc,
> >> +                                                      pcre2_free, NULL);
> >> +
> >> +       return context;
> >> +}
> >
> > instead of using a static variable inside this helper function it
> > might be better to use
> > one extra field inside the (struct grep_pat *p), where all other
> > variables are kept
> >
> > Additionally to being more consistent will avoid creating the global
>
> "standard" and "more consistent" are good things, but I am not sure
> I should agree with the argument without knowing what you are
> comparing your suggested improvement with.  Whose standard practice
> are we trying to be consistent with?  Keeping dynamic resources hooked
> to "struct grep_pat" so that (1) different patterns could use different
> settings when they desire and (2) the resources are not hidden behind
> a function-scope static and can be discarded when we are done with
> the pattern, which is the standard in our "grep" subsystem?

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

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

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

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

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

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

> > context for the
> > most common case (when the locale is either C/POSIX or UTF-8) and therefore
> > have a smaller impact on performance.
>
> I am not sure about the impact on performance, but if it helps us
> keep the subsystem reusable by avoiding function-scope static that
> we cannot clear, that would be a good thing.  But "struct grep_pat"
> may be a bit too fine-grained to control general context.

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

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

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

Carlo

Patch

diff --git a/grep.c b/grep.c
index d4c5d464ad..d6d29fc724 100644
--- a/grep.c
+++ b/grep.c
@@ -482,6 +482,27 @@  static void free_pcre1_regexp(struct grep_pat *p)
 #endif /* !USE_LIBPCRE1 */
 
 #ifdef USE_LIBPCRE2
+static void *pcre2_malloc(PCRE2_SIZE size, void *memory_data)
+{
+	return malloc(size);
+}
+
+static void pcre2_free(void *pointer, void *memory_data)
+{
+	return free(pointer);
+}
+
+static pcre2_general_context *get_pcre2_context(void)
+{
+	static pcre2_general_context *context;
+
+	if (!context)
+		context = pcre2_general_context_create(pcre2_malloc,
+						       pcre2_free, NULL);
+
+	return context;
+}
+
 static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt)
 {
 	int error;
@@ -498,8 +519,9 @@  static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt
 
 	if (opt->ignore_case) {
 		if (has_non_ascii(p->pattern)) {
-			p->pcre2_tables = pcre2_maketables(NULL);
-			p->pcre2_compile_context = pcre2_compile_context_create(NULL);
+			p->pcre2_tables = pcre2_maketables(get_pcre2_context());
+			p->pcre2_compile_context =
+				pcre2_compile_context_create(get_pcre2_context());
 			pcre2_set_character_tables(p->pcre2_compile_context,
 							p->pcre2_tables);
 		}
@@ -513,7 +535,8 @@  static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt
 					 p->pcre2_compile_context);
 
 	if (p->pcre2_pattern) {
-		p->pcre2_match_data = pcre2_match_data_create_from_pattern(p->pcre2_pattern, NULL);
+		p->pcre2_match_data =
+			pcre2_match_data_create_from_pattern(p->pcre2_pattern, get_pcre2_context());
 		if (!p->pcre2_match_data)
 			die("Couldn't allocate PCRE2 match data");
 	} else {
@@ -550,7 +573,7 @@  static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt
 			return;
 		}
 
-		p->pcre2_jit_stack = pcre2_jit_stack_create(1, 1024 * 1024, NULL);
+		p->pcre2_jit_stack = pcre2_jit_stack_create(1, 1024 * 1024, get_pcre2_context());
 		if (!p->pcre2_jit_stack)
 			die("Couldn't allocate PCRE2 JIT stack");
 		p->pcre2_match_context = pcre2_match_context_create(NULL);