[RFC,v3,0/3] grep: no leaks or crashes (windows testing needed)

Message

Carlo Marcelo Arenas Belón Aug. 6, 2019, 4:36 p.m. UTC

This series is a candidate reroll for cb/pcre2-chartables-leakfix, that
hopefully addresses the root cause of the problem reported by Dscho in
Windows, where the PCRE2 library wasn't aware of the custom allocator and
was returning a pointer created with the system malloc but passing it to
NED's free, resulting in a segfault.

The reason why it was triggered by the original leak fix is the layering
violation reported by René and that is exclusive to PCRE2 (hence why it
hasn't been reported with PCRE1).  The first patch could be droped and
then used in a different series that will fully integrate the custom
allocator with the PCRE library and that is currently missing with PCRE2.

Eitherway, since I am unable to replicate the original bug or take
performance numbers in a representative environment without Windows
this is only published as an RFC, eventhough it has been tested and
considered mostly complete.

Junio, could you comment in my assumption that the use of grep in
revision.c doesn't require initializing a PCRE2 global context and
therefore not doing the cleanup?

Carlo Marcelo Arenas Belón (3):
  grep: make PCRE1 aware of custom allocator
  grep: make PCRE2 aware of custom allocator
  grep: avoid leak of chartables in PCRE2

 Makefile       |  2 +-
 builtin/grep.c |  1 +
 grep.c         | 51 +++++++++++++++++++++++++++++++++++++++++++++++---
 grep.h         |  2 ++
 4 files changed, 52 insertions(+), 4 deletions(-)

Comments

Junio C Hamano Aug. 6, 2019, 4:48 p.m. UTC | #1

Carlo Marcelo Arenas Belón  <[e-mail]> writes:

> Junio, could you comment in my assumption that the use of grep in
> revision.c doesn't require initializing a PCRE2 global context and
> therefore not doing the cleanup?

Many callers of setup_revisions() do so only to run two-thing diffs
(e.g. run_diff_files() that compares the index and the working tree),
but some do so to traverse the history with the grep_filter
(e.g. "git log --grep=<pattern>").  "git log -P" may affect how that
pattern is used to match the contents of the log messages, no?

Johannes Schindelin Aug. 8, 2019, 8:21 p.m. UTC | #2

Hi Carlo,

On Tue, 6 Aug 2019, Carlo Marcelo Arenas Belón wrote:

> Eitherway, since I am unable to replicate the original bug or take
> performance numbers in a representative environment without Windows
> this is only published as an RFC, eventhough it has been tested and
> considered mostly complete.

Well, this is disappointing.

I worked several weeks on getting Azure Pipelines support in shape, so
that you can now open PRs against:

- https://github.com/git/git

- https://github.com/gitgitgadget/git

- https://github.com/git-for-windows/git

to get Windows/macOS/Linux testing for free.

So I guess you'd like fries with that, extra large ones, with extra
pepperoni seasoning?

Ciao,
Dscho

Carlo Marcelo Arenas Belón Aug. 9, 2019, 6:52 a.m. UTC | #3

On Thu, Aug 8, 2019 at 1:21 PM Johannes Schindelin
<[e-mail]> wrote:
> On Tue, 6 Aug 2019, Carlo Marcelo Arenas Belón wrote:
> > Eitherway, since I am unable to replicate the original bug or take
> > performance numbers in a representative environment without Windows
> > this is only published as an RFC, eventhough it has been tested and
> > considered mostly complete.
>
> Well, this is disappointing.

Apologies

> I worked several weeks on getting Azure Pipelines support in shape, so
> that you can now open PRs against:

Thanks, I owe you a drink

  https://dev.azure.com/git/git/_build/results?buildId=862

Carlo

PS. I might had broken something here as I can't see the test results
that failed
https://dev.azure.com/git/git/_build/results?buildId=857&view=ms.vss-test-web.build-test-results-tab

Johannes Schindelin Aug. 9, 2019, 9:13 p.m. UTC | #4

Hi Carlo,

On Thu, 8 Aug 2019, Carlo Arenas wrote:

> PS. I might had broken something here as I can't see the test results
> that failed
> https://dev.azure.com/git/git/_build/results?buildId=857&view=ms.vss-test-web.build-test-results-tab

The test results load dynamically, so you'll probably just have to wait
for a couple moments. After that, just click on the lines indicating the
failed test cases to pop up a verbose trace of it.

Ciao,
Dscho