Hi Carlo,
On Mon, 5 Aug 2019, Carlo Arenas wrote:
> On Mon, Aug 5, 2019 at 4:51 AM Johannes Schindelin via GitGitGadget
> <gitgitgadget@xxxxxxxxx> 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
