Fix a problem with PCRE2 and nedmalloc, found via Azure Pipelines #306
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
94da919 ("grep: add support for PCRE v2", 2017-06-01) introduced a small memory leak visible with valgrind in t7813. Complete the creation of a PCRE2 specific variable that was missing from the original change and free the generated table just like it is done for PCRE1. The table cleanup use free as there is no global context defined when it was created (pcre2_maketables is passed a NULL pointer) but if that gets ever changed will need to be updated in tandem. Signed-off-by: Carlo Marcelo Arenas Belón <[email protected]> Signed-off-by: Junio C Hamano <[email protected]>
Since 7d3bf76 (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. However, when we compile Git with `USE_NED_ALLOCATOR` (notably on Windows), then `free()` actually calls `nedfree()`. But `pcre2_maketables()` allocated the tables using the system allocator because we did not tell it to use nedmalloc instead. This leads to segmentation faults when the UTF-8 tables are released, most notably in the `t7816-grep-binary-pattern.sh` test script. PCRE2 does have an option to override the allocator it should use, and this patch calls upon it. As there are other ways to override the system allocator than `USE_NED_ALLOCATOR`, we choose to specify the allocator we want to use specifically, even if we still use the system allocator. Signed-off-by: Johannes Schindelin <[email protected]>
|
/submit |
|
Submitted as [email protected] |
![gitgitgadget[bot]](https://avatars.githubusercontent.com/in/12836?s=60&v=4){kind=small}
dscho
force-pushed
the
cb/pcre2-chartables-leakfix
branch
from
7d3bf76 to
54aef14
Compare
This was referenced Aug 11, 2019
dscho
added
the
needs-work
dscho
force-pushed
the
cb/pcre2-chartables-leakfix
branch
from
54aef14 to
10da030
Compare
|
Closing in favor of #402 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
For a couple of days, maybe even a week,
pufails consistently, in the Windows job where it tests t7816. The symptom is a segmentation fault.I finally got to diagnose this, and it looked at first as if there was yet another buffer overrun that was so small that valgrind failed to detect it (see #178). The problem is another one, though: we ask PCRE2 to allocate a table (and it uses the system allocator for that), and then try to release it using nedmalloc's
free()replacement.This is squarely a problem with
cb/pcre2-chartables-leakfixin conjunction with an overridden allocator.Junio, for your convenience, I rebased this patch directly on top of
ab/pcre-v2and pushed out thelet-pcre2-respect-nedmallocbranch at https://github.com/dscho/git ready to be pulled. The rebased version is not technically a bug fix, as I do not see any way thatab/pcre-v2uses mismatched allocators formalloc()/free(), but just in case that you wanted to have it in v2.23.0 and not on topcb/pcre2-chartables-leakfix...Cc: Carlo Marcelo Arenas Belón [email protected]