| Message ID | 20190806085014.47776-2-carenas@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | grep: no leaks (WIP) | expand |
Hi Carlo, 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 alocator, 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. 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. Ciao, Dscho > COMPAT_OBJS += compat/nedmalloc/nedmalloc.o > OVERRIDE_STRDUP = YesPlease > endif > diff --git a/grep.c b/grep.c > index cd952ef5d3..0154998695 100644 > --- a/grep.c > +++ b/grep.c > @@ -150,12 +150,22 @@ int grep_config(const char *var, const char *value, void *cb) > * Initialize one instance of grep_opt and copy the > * default values from the template we read the configuration > * information in an earlier call to git_config(grep_config). > + * > + * If using PCRE make sure that the library is configured > + * to use the right allocator (ex: NED) > */ > void grep_init(struct grep_opt *opt, struct repository *repo, const char *prefix) > { > struct grep_opt *def = &grep_defaults; > int i; > > +#ifdef USE_NED_ALLOCATOR > +#ifdef USE_LIBPCRE1 > + pcre_malloc = malloc; > + pcre_free = free; > +#endif > +#endif > + > memset(opt, 0, sizeof(*opt)); > opt->repo = repo; > opt->prefix = prefix; > -- > 2.23.0.rc1 > >
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
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 COMPAT_OBJS += compat/nedmalloc/nedmalloc.o OVERRIDE_STRDUP = YesPlease endif diff --git a/grep.c b/grep.c index cd952ef5d3..0154998695 100644 --- a/grep.c +++ b/grep.c @@ -150,12 +150,22 @@ int grep_config(const char *var, const char *value, void *cb) * Initialize one instance of grep_opt and copy the * default values from the template we read the configuration * information in an earlier call to git_config(grep_config). + * + * If using PCRE make sure that the library is configured + * to use the right allocator (ex: NED) */ void grep_init(struct grep_opt *opt, struct repository *repo, const char *prefix) { struct grep_opt *def = &grep_defaults; int i; +#ifdef USE_NED_ALLOCATOR +#ifdef USE_LIBPCRE1 + pcre_malloc = malloc; + pcre_free = free; +#endif +#endif + memset(opt, 0, sizeof(*opt)); opt->repo = repo; opt->prefix = prefix;
63e7e9d8b6 ("git-grep: Learn PCRE", 2011-05-09) didn't include a way to override the system alocator, 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(-)