git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / Atom feed
From: Carlo Arenas <carenas@gmail.com>
To: Johannes Schindelin <Johannes.Schindelin@gmx.de>
Cc: gitster@pobox.com, git@vger.kernel.org
Subject: Re: [PATCH 1/3] grep: make PCRE1 aware of custom allocator
Date: Thu, 8 Aug 2019 08:19:52 -0700
Message-ID: <CAPUEsphLZ+x1hL+=8P+bh34mR6pUcXB5Qd2htj4AzUjjrKC=nQ@mail.gmail.com> (raw)
In-Reply-To: <nycvar.QRO.7.76.6.1908081552440.46@tvgsbejvaqbjf.bet>

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

  reply	other threads:[~2019-08-08 15:20 UTC|newest]

Thread overview: 68+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-08-05 11:51 [PATCH 0/1] Fix a problem with PCRE2 and nedmalloc, found via Azure Pipelines Johannes Schindelin via GitGitGadget
2019-08-05 11:51 ` [PATCH 1/1] pcre2: allow overriding the system allocator Johannes Schindelin via GitGitGadget
2019-08-05 16:19   ` Carlo Arenas
2019-08-05 16:27     ` Carlo Arenas
2019-08-05 19:32       ` Johannes Schindelin
2019-08-05 19:26     ` Johannes Schindelin
2019-08-05 21:53     ` Junio C Hamano
2019-08-06  6:24       ` Carlo Arenas
2019-08-06  8:50       ` [PATCH 0/3] grep: no leaks (WIP) Carlo Marcelo Arenas Belón
2019-08-06  8:50         ` [PATCH 1/3] grep: make PCRE1 aware of custom allocator Carlo Marcelo Arenas Belón
2019-08-08 13:54           ` Johannes Schindelin
2019-08-08 15:19             ` Carlo Arenas [this message]
2019-08-06  8:50         ` [PATCH 2/3] grep: make PCRE2 " Carlo Marcelo Arenas Belón
2019-08-08 13:56           ` Johannes Schindelin
2019-08-08 14:32             ` Carlo Arenas
2019-08-06  8:50         ` [PATCH 3/3] grep: avoid leak of chartables in PCRE2 Carlo Marcelo Arenas Belón
2019-08-06 16:36         ` [RFC PATCH v3 0/3] grep: no leaks or crashes (windows testing needed) Carlo Marcelo Arenas Belón
2019-08-06 16:36           ` [RFC PATCH v3 1/3] grep: make PCRE1 aware of custom allocator Carlo Marcelo Arenas Belón
2019-08-06 16:36           ` [RFC PATCH v3 2/3] grep: make PCRE2 " Carlo Marcelo Arenas Belón
2019-08-07  5:38             ` René Scharfe
2019-08-07  9:49               ` Carlo Arenas
2019-08-07 13:02                 ` René Scharfe
2019-08-07 13:08                   ` [PATCH 1/2] nedmalloc: do assignments only after the declaration section René Scharfe
2019-08-07 13:09                   ` [PATCH 2/2] nedmalloc: avoid compiler warning about unused value René Scharfe
2019-08-08  2:35                   ` [RFC PATCH v3 2/3] grep: make PCRE2 aware of custom allocator Carlo Arenas
2019-08-08  7:07                     ` René Scharfe
2019-08-08 12:38                       ` Carlo Arenas
2019-08-08 14:29                         ` René Scharfe
2019-08-08 20:18                           ` Johannes Schindelin
2019-08-07 18:15                 ` Junio C Hamano
2019-08-06 16:36           ` [RFC PATCH v3 3/3] grep: avoid leak of chartables in PCRE2 Carlo Marcelo Arenas Belón
2019-08-06 16:48           ` [RFC PATCH v3 0/3] grep: no leaks or crashes (windows testing needed) Junio C Hamano
2019-08-07 21:39           ` [RFC PATCH v4 " Carlo Marcelo Arenas Belón
2019-08-07 21:39             ` [RFC PATCH v4 1/3] grep: make PCRE1 aware of custom allocator Carlo Marcelo Arenas Belón
2019-08-07 21:39             ` [RFC PATCH v4 2/3] grep: make PCRE2 " Carlo Marcelo Arenas Belón
2019-08-07 22:28               ` Junio C Hamano
2019-08-07 21:39             ` [RFC PATCH v4 3/3] grep: avoid leak of chartables in PCRE2 Carlo Marcelo Arenas Belón
2019-08-09  3:02             ` [RFC PATCH v5 0/3] grep: almost no more leaks, hopefully no crashes Carlo Marcelo Arenas Belón
2019-08-09  3:02               ` [RFC PATCH v5 1/3] grep: make PCRE1 aware of custom allocator Carlo Marcelo Arenas Belón
2019-08-09  3:02               ` [RFC PATCH v5 2/3] grep: make PCRE2 " Carlo Marcelo Arenas Belón
2019-08-27  9:07                 ` Johannes Schindelin
2019-08-27 11:51                   ` Carlo Arenas
2019-10-03  5:02                     ` Junio C Hamano
2019-10-03  8:08                       ` Johannes Schindelin
2019-10-03 11:17                         ` Carlo Arenas
2019-10-03 18:23                           ` Johannes Schindelin
2019-10-03 22:57                           ` Junio C Hamano
2019-08-09  3:02               ` [RFC PATCH v5 3/3] grep: avoid leak of chartables in PCRE2 Carlo Marcelo Arenas Belón
2019-08-09 11:24               ` [RFC PATCH v5 0/3] grep: almost no more leaks, hopefully no crashes Carlo Arenas
2019-08-09 17:01                 ` René Scharfe
2019-08-09 17:46                   ` Junio C Hamano
2019-08-09 21:26                   ` Johannes Schindelin
2019-08-10  3:03                     ` [PATCH] SQUASH Carlo Marcelo Arenas Belón
2019-08-10  7:57                       ` René Scharfe
2019-08-10  8:42                         ` Carlo Arenas
2019-08-10 19:47                           ` René Scharfe
2019-08-12  7:35                             ` Carlo Arenas
2019-08-12 12:14                               ` René Scharfe
2019-08-12 12:28                                 ` Carlo Arenas
2019-08-10 13:57                       ` Johannes Schindelin
2019-08-10  3:05                     ` [RFC PATCH v5 0/3] grep: almost no more leaks, hopefully no crashes Carlo Arenas
2019-08-10  7:56                       ` René Scharfe
2019-08-10 12:40                         ` Carlo Arenas
2019-08-10 21:16                           ` René Scharfe
2019-08-08 20:21           ` [RFC PATCH v3 0/3] grep: no leaks or crashes (windows testing needed) Johannes Schindelin
2019-08-09  6:52             ` Carlo Arenas
2019-08-09 21:13               ` Johannes Schindelin
2019-10-16 12:06 [PATCH 0/3] Revive 'pcre2-chartables-leakfix' Johannes Schindelin via GitGitGadget
2019-10-16 12:06 ` [PATCH 1/3] grep: make PCRE1 aware of custom allocator Carlo Marcelo Arenas Belón via GitGitGadget

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAPUEsphLZ+x1hL+=8P+bh34mR6pUcXB5Qd2htj4AzUjjrKC=nQ@mail.gmail.com' \
    --to=carenas@gmail.com \
    --cc=Johannes.Schindelin@gmx.de \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

git@vger.kernel.org list mirror (unofficial, one of many)

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V1 git git/ https://public-inbox.org/git \
		git@vger.kernel.org
	public-inbox-index git

Example config snippet for mirrors.
Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.io/gmane.comp.version-control.git
 note: .onion URLs require Tor: https://www.torproject.org/

code repositories for the project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

AGPL code for this site: git clone https://public-inbox.org/public-inbox.git