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

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Aug 5, 2019, 19:26 UTC
Message-ID: <nycvar.QRO.7.76.6.1908052120302.46@tvgsbejvaqbjf.bet>
In-Reply-To: <CAPUEspgH1v1zo7smzQWCV4rX9pKVKLV84gDSfCPdT7LffQxUWw@mail.gmail.com>

Hi Carlo,

On Mon, 5 Aug 2019, Carlo Arenas wrote:

Show 13 quoted lines

> On Mon, Aug 5, 2019 at 4:51 AM Johannes Schindelin via GitGitGadget
> <gitgitgadget@gmail.com> 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.

Show 17 quoted lines

> 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