Re: [PATCH 1/3] grep: make PCRE1 aware of custom allocator

[Carlo Arenas <carenas@gmail.com>]

On Thu, Aug 8, 2019 at 6:55 AM Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:

> On Tue, 6 Aug 2019, Carlo Marcelo Arenas Belón wrote:
>
> > 63e7e9d8b6 ("git-grep: Learn PCRE", 2011-05-09) didn't include a way
> > to override the system allocator, and so it is incompatible with
> > USE_NED_ALLOCATOR as reported by Dscho[1] (in similar code from PCRE2)
> >
> > Make the minimum change possible to ensure this combination is supported
> >
> > [1] https://public-inbox.org/git/pull.306.git.gitgitgadget@gmail.com
> >
> > Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
> > ---
> >  Makefile |  2 +-
> >  grep.c   | 10 ++++++++++
> >  2 files changed, 11 insertions(+), 1 deletion(-)
> >
> > diff --git a/Makefile b/Makefile
> > index bd246f2989..4b384f3759 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -1764,7 +1764,7 @@ ifdef NATIVE_CRLF
> >  endif
> >
> >  ifdef USE_NED_ALLOCATOR
> > -     COMPAT_CFLAGS += -Icompat/nedmalloc
> > +     COMPAT_CFLAGS += -DUSE_NED_ALLOCATOR -Icompat/nedmalloc
>
> This pretends that all custom allocators are selected at build time,
> something I tried to stress in my commit message as not true. You can
> pre-load not only nedmalloc, but also jemalloc and unless I am mistaken
> also tcmalloc. And mi-malloc.

the patch only fixes the case when NED was chosen at build time, which
before this patch meant that the system allocator was used by PCRE1
and NED by git.

That combination happened to work unless any pointer crossed over but
neglected any benefit of using NED instead of the system allocator
with PCRE1.

the PCRE1 API was therefore safer to use, PCRE2 is not and that will
be addressed[1] in a future version by making sure this "layering
violation" is covered

runtime custom allocator changes are not affected and will keep
working as usual, which (at least in Linux when using LD_PRELOAD which
is probably what you are refer to by "preload") means that the dynamic
linker will replace all calls to malloc/free with the ones you
provided at exec time and before it starts the binary (including
whichever libraries it loaded) and therefore will make both git and
PCRE2 to use the same allocator.

nedmalloc seems (at least in Linux) to be smart enough to cope with
pointers that were not allocated by itself and doesn't crash so
presume there is something else in Windows (maybe the dynamic linker,
if there is one) that might be also part of the reason for this
segfault.

of course, the trigger of this is my patch, so I'll take the blame for
this bug regardless and I am looking forward to a fix

> So the premise of this patch, that you can tell at compile time that a
> different allocator than the system one will be in use is simply
> incorrect.

in the context that the only available (and integrated with our build
system) allocator is NED at compile time, I think it is correct.
you are correct though that if another allocator will be added as a
compile time option, we will need to do something similar as well.

Carlo

[1] https://bugs.exim.org/show_bug.cgi?id=2429