{"thread":{"id":"51538","subject":"[PATCH 0/3] grep: memory leak in PCRE2","startedAt":"2019-07-27T20:28:04Z","lastAt":"2019-08-05T19:35:04Z","messageCount":15,"participants":["Carlo Marcelo Arenas Belón","Ævar Arnfjörð Bjarmason","Carlo Arenas","René Scharfe","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"379424","messageId":"20190727202759.22310-1-carenas@gmail.com","threadId":"51538","inReplyTo":null,"subject":"[PATCH 0/3] grep: memory leak in PCRE2","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2019-07-27T20:27:56Z","receivedAt":"2019-07-27T20:28:04Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"This series fixes a small memory leak that was introduced when PCRE2\nsupport was added, and that should be visible with t7813 and valgrind\n\nApplies cleanly to maint and master but will conflict slightly with\nab/no-kwset (currently in next) and most likely other changes introduced\nabout this same codebase (ex: ab/pcre-jit-fixes) but hopefully the\nspreading on short simple commits helps with reviewing.\n\nCarlo Marcelo Arenas Belón (3):\n  grep: make pcre1_tables version agnostic\n  grep: use pcre_tables also for PCRE2\n  grep: plug leak of pcre chartables in PCRE2\n\n grep.c | 12 ++++++------\n grep.h |  2 +-\n 2 files changed, 7 insertions(+), 7 deletions(-)\n\n-- \n2.22.0\n"},{"id":"379425","messageId":"20190727202759.22310-2-carenas@gmail.com","threadId":"51538","inReplyTo":"20190727202759.22310-1-carenas@gmail.com","subject":"[PATCH 1/3] grep: make pcre1_tables version agnostic","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2019-07-27T20:27:57Z","receivedAt":"2019-07-27T20:28:05Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"6d4b5747f0 (\"grep: change internal *pcre* variable & function names\nto be *pcre1*\", 2017-05-25), renamed most variables to be PCRE1\nspecific to give space to similarly named variables for PCRE2, but\nin this case the change wasn't needed as the types were compatible\nenough (const unsigned char* vs const uint8_t*) to be shared.\n\nRevert that change, as 94da9193a6 (\"grep: add support for PCRE v2\",\n2017-06-01) failed to create an equivalent PCRE2 version.\n\nSigned-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>\n---\n grep.c | 6 +++---\n grep.h | 2 +-\n 2 files changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/grep.c b/grep.c\nindex f7c3a5803e..cc65f7a987 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -389,14 +389,14 @@ static void compile_pcre1_regexp(struct grep_pat *p, const struct grep_opt *opt)\n \n \tif (opt->ignore_case) {\n \t\tif (has_non_ascii(p->pattern))\n-\t\t\tp->pcre1_tables = pcre_maketables();\n+\t\t\tp->pcre_tables = pcre_maketables();\n \t\toptions |= PCRE_CASELESS;\n \t}\n \tif (is_utf8_locale() && has_non_ascii(p->pattern))\n \t\toptions |= PCRE_UTF8;\n \n \tp->pcre1_regexp = pcre_compile(p->pattern, options, &error, &erroffset,\n-\t\t\t\t      p->pcre1_tables);\n+\t\t\t\t      p->pcre_tables);\n \tif (!p->pcre1_regexp)\n \t\tcompile_regexp_failed(p, error);\n \n@@ -462,7 +462,7 @@ static void free_pcre1_regexp(struct grep_pat *p)\n \t{\n \t\tpcre_free(p->pcre1_extra_info);\n \t}\n-\tpcre_free((void *)p->pcre1_tables);\n+\tpcre_free((void *)p->pcre_tables);\n }\n #else /* !USE_LIBPCRE1 */\n static void compile_pcre1_regexp(struct grep_pat *p, const struct grep_opt *opt)\ndiff --git a/grep.h b/grep.h\nindex 1875880f37..d34f66b384 100644\n--- a/grep.h\n+++ b/grep.h\n@@ -89,7 +89,7 @@ struct grep_pat {\n \tpcre *pcre1_regexp;\n \tpcre_extra *pcre1_extra_info;\n \tpcre_jit_stack *pcre1_jit_stack;\n-\tconst unsigned char *pcre1_tables;\n+\tconst unsigned char *pcre_tables;\n \tint pcre1_jit_on;\n \tpcre2_code *pcre2_pattern;\n \tpcre2_match_data *pcre2_match_data;\n-- \n2.22.0\n"},{"id":"379426","messageId":"20190727202759.22310-3-carenas@gmail.com","threadId":"51538","inReplyTo":"20190727202759.22310-1-carenas@gmail.com","subject":"[PATCH 2/3] grep: use pcre_tables also for PCRE2","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2019-07-27T20:27:58Z","receivedAt":"2019-07-27T20:28:08Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"94da9193a6 (\"grep: add support for PCRE v2\", 2017-06-01) added a\nlocal variable to keep track of the chartables (used when locale\nis not UTF-8 but non-ASCII characters are needed)\n\nRemove that local variable in favor of the shared one within the\ngrep structure and that can be shared by all functions.\n\nSigned-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>\n---\n grep.c | 5 ++---\n 1 file changed, 2 insertions(+), 3 deletions(-)\n\ndiff --git a/grep.c b/grep.c\nindex cc65f7a987..d04635fad4 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -488,7 +488,6 @@ static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt\n \tPCRE2_UCHAR errbuf[256];\n \tPCRE2_SIZE erroffset;\n \tint options = PCRE2_MULTILINE;\n-\tconst uint8_t *character_tables = NULL;\n \tint jitret;\n \tint patinforet;\n \tsize_t jitsizearg;\n@@ -499,9 +498,9 @@ static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt\n \n \tif (opt->ignore_case) {\n \t\tif (has_non_ascii(p->pattern)) {\n-\t\t\tcharacter_tables = pcre2_maketables(NULL);\n+\t\t\tp->pcre_tables = pcre2_maketables(NULL);\n \t\t\tp->pcre2_compile_context = pcre2_compile_context_create(NULL);\n-\t\t\tpcre2_set_character_tables(p->pcre2_compile_context, character_tables);\n+\t\t\tpcre2_set_character_tables(p->pcre2_compile_context, p->pcre_tables);\n \t\t}\n \t\toptions |= PCRE2_CASELESS;\n \t}\n-- \n2.22.0\n"},{"id":"379427","messageId":"20190727202759.22310-4-carenas@gmail.com","threadId":"51538","inReplyTo":"20190727202759.22310-1-carenas@gmail.com","subject":"[PATCH 3/3] grep: plug leak of pcre chartables in PCRE2","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2019-07-27T20:27:59Z","receivedAt":"2019-07-27T20:28:09Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"Just as it is done with PCRE1, make sure that the allocated chartables\nget free at cleanup time.\n\nThis assumes no global context is used (NULL passed when created the\ntables), but will likely be updated in tandem if that ever changes.\n\nSigned-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>\n---\n grep.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/grep.c b/grep.c\nindex d04635fad4..d9768c5f05 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -604,6 +604,7 @@ static void free_pcre2_pattern(struct grep_pat *p)\n \tpcre2_match_data_free(p->pcre2_match_data);\n \tpcre2_jit_stack_free(p->pcre2_jit_stack);\n \tpcre2_match_context_free(p->pcre2_match_context);\n+\tfree((void *)p->pcre_tables);\n }\n #else /* !USE_LIBPCRE2 */\n static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt)\n-- \n2.22.0\n"},{"id":"379435","messageId":"875znn6nvt.fsf@evledraar.gmail.com","threadId":"51538","inReplyTo":"20190727202759.22310-2-carenas@gmail.com","subject":"Re: [PATCH 1/3] grep: make pcre1_tables version agnostic","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-07-27T23:47:34Z","receivedAt":"2019-07-27T23:47:39Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Sat, Jul 27 2019, Carlo Marcelo Arenas Belón wrote:\n\n> 6d4b5747f0 (\"grep: change internal *pcre* variable & function names\n> to be *pcre1*\", 2017-05-25), renamed most variables to be PCRE1\n> specific to give space to similarly named variables for PCRE2, but\n> in this case the change wasn't needed as the types were compatible\n> enough (const unsigned char* vs const uint8_t*) to be shared.\n\nBoth the v1 and v2 functions return const unsigned char *. I don't know\nwhere I got the uint8_t from. This makes more sense.\n\nThis series looks good to me. Thanks for the fix. Just one caveat:\n\nThe point of 6d4b5747f0 was not to only split out those variables we\ncouldn't get away with re-using. Then I would have later re-used\ne.g. pcre1_jit_on & pcre2_jit_on as just pcre_jit_on. We could also do\nthat now.\n\nI think doing that & this part of the your changes makes things less\nreadable. The two code branches we compile with ifdefs are mutually\nexclusive, so having the variables be unique helps with eyeballing /\nreasoning when changing the code.\n\n> Revert that change, as 94da9193a6 (\"grep: add support for PCRE v2\",\n> 2017-06-01) failed to create an equivalent PCRE2 version.\n>\n> Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>\n> ---\n>  grep.c | 6 +++---\n>  grep.h | 2 +-\n>  2 files changed, 4 insertions(+), 4 deletions(-)\n>\n> diff --git a/grep.c b/grep.c\n> index f7c3a5803e..cc65f7a987 100644\n> --- a/grep.c\n> +++ b/grep.c\n> @@ -389,14 +389,14 @@ static void compile_pcre1_regexp(struct grep_pat *p, const struct grep_opt *opt)\n>\n>  \tif (opt->ignore_case) {\n>  \t\tif (has_non_ascii(p->pattern))\n> -\t\t\tp->pcre1_tables = pcre_maketables();\n> +\t\t\tp->pcre_tables = pcre_maketables();\n>  \t\toptions |= PCRE_CASELESS;\n>  \t}\n>  \tif (is_utf8_locale() && has_non_ascii(p->pattern))\n>  \t\toptions |= PCRE_UTF8;\n>\n>  \tp->pcre1_regexp = pcre_compile(p->pattern, options, &error, &erroffset,\n> -\t\t\t\t      p->pcre1_tables);\n> +\t\t\t\t      p->pcre_tables);\n>  \tif (!p->pcre1_regexp)\n>  \t\tcompile_regexp_failed(p, error);\n>\n> @@ -462,7 +462,7 @@ static void free_pcre1_regexp(struct grep_pat *p)\n>  \t{\n>  \t\tpcre_free(p->pcre1_extra_info);\n>  \t}\n> -\tpcre_free((void *)p->pcre1_tables);\n> +\tpcre_free((void *)p->pcre_tables);\n>  }\n>  #else /* !USE_LIBPCRE1 */\n>  static void compile_pcre1_regexp(struct grep_pat *p, const struct grep_opt *opt)\n> diff --git a/grep.h b/grep.h\n> index 1875880f37..d34f66b384 100644\n> --- a/grep.h\n> +++ b/grep.h\n> @@ -89,7 +89,7 @@ struct grep_pat {\n>  \tpcre *pcre1_regexp;\n>  \tpcre_extra *pcre1_extra_info;\n>  \tpcre_jit_stack *pcre1_jit_stack;\n> -\tconst unsigned char *pcre1_tables;\n> +\tconst unsigned char *pcre_tables;\n>  \tint pcre1_jit_on;\n>  \tpcre2_code *pcre2_pattern;\n>  \tpcre2_match_data *pcre2_match_data;\n"},{"id":"379436","messageId":"874l376nud.fsf@evledraar.gmail.com","threadId":"51538","inReplyTo":"20190727202759.22310-4-carenas@gmail.com","subject":"Re: [PATCH 3/3] grep: plug leak of pcre chartables in PCRE2","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-07-27T23:48:26Z","receivedAt":"2019-07-27T23:48:30Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Sat, Jul 27 2019, Carlo Marcelo Arenas Belón wrote:\n\n> Just as it is done with PCRE1, make sure that the allocated chartables\n> get free at cleanup time.\n>\n> This assumes no global context is used (NULL passed when created the\n> tables), but will likely be updated in tandem if that ever changes.\n>\n> Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>\n> ---\n>  grep.c | 1 +\n>  1 file changed, 1 insertion(+)\n>\n> diff --git a/grep.c b/grep.c\n> index d04635fad4..d9768c5f05 100644\n> --- a/grep.c\n> +++ b/grep.c\n> @@ -604,6 +604,7 @@ static void free_pcre2_pattern(struct grep_pat *p)\n>  \tpcre2_match_data_free(p->pcre2_match_data);\n>  \tpcre2_jit_stack_free(p->pcre2_jit_stack);\n>  \tpcre2_match_context_free(p->pcre2_match_context);\n> +\tfree((void *)p->pcre_tables);\n\nIs the cast really needed? I'm rusty on the rules, removing it from the\npcre_free() you might have copied this from produces a warning for me,\nbut not for free() itself. This is on GCC 8.3.0. How about for you &\nwhat compiler(s)?\n"},{"id":"379437","messageId":"CAPUEspgowrhj56SzW8cPKjZL28nPTkX-RScKrSAai8i0YkwFaA@mail.gmail.com","threadId":"51538","inReplyTo":"874l376nud.fsf@evledraar.gmail.com","subject":"Re: [PATCH 3/3] grep: plug leak of pcre chartables in PCRE2","fromName":"Carlo Arenas","fromEmail":"carenas@gmail.com","sentAt":"2019-07-28T01:41:08Z","receivedAt":"2019-07-28T01:41:21Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"On Sat, Jul 27, 2019 at 4:48 PM Ævar Arnfjörð Bjarmason\n<avarab@gmail.com> wrote:\n> > +     free((void *)p->pcre_tables);\n>\n> Is the cast really needed? I'm rusty on the rules, removing it from the\n> pcre_free() you might have copied this from produces a warning for me,\n> but not for free() itself. This is on GCC 8.3.0. How about for you &\n> what compiler(s)?\n\nboth will trigger warnings for the same reason\n(-Wincompatible-pointer-types-discards-qualifiers)\nwith Apple LLVM version 10.0.1 (clang-1001.0.46.4)\n\ngcc-9 in macOS triggers 2 \"warnings\"; one for discarding the const\nqualifier (-Wdiscarded-qualifiers)\nand another for mismatching parameter to free():\n\nnote: expected 'void *' but argument is of type 'const uint8_t *' {aka\n'const unsigned char *'}\n\nCarlo\n"},{"id":"379438","messageId":"CAPUEspg255TAE-1ozc8CuC=k9oeP_WLD4TrFur5p=XY3s8PX3Q@mail.gmail.com","threadId":"51538","inReplyTo":"875znn6nvt.fsf@evledraar.gmail.com","subject":"Re: [PATCH 1/3] grep: make pcre1_tables version agnostic","fromName":"Carlo Arenas","fromEmail":"carenas@gmail.com","sentAt":"2019-07-28T02:50:07Z","receivedAt":"2019-07-28T02:50:21Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"On Sat, Jul 27, 2019 at 4:47 PM Ævar Arnfjörð Bjarmason\n<avarab@gmail.com> wrote:\n>\n> On Sat, Jul 27 2019, Carlo Marcelo Arenas Belón wrote:\n>\n> > 6d4b5747f0 (\"grep: change internal *pcre* variable & function names\n> > to be *pcre1*\", 2017-05-25), renamed most variables to be PCRE1\n> > specific to give space to similarly named variables for PCRE2, but\n> > in this case the change wasn't needed as the types were compatible\n> > enough (const unsigned char* vs const uint8_t*) to be shared.\n\nreading this again, had to admit there is a fair amount of guessing on\nintent, but reading commits and the email discussion couldn't come\nup with a better explanation.\n\nis the root cause for the bug different?, could it be that the pcre2 API\nwas misunderstood? and you expected this pointer will be free together\nwith the context? (as it is done when a cloned context with tables is\nused?)\n\n> Both the v1 and v2 functions return const unsigned char *. I don't know\n> where I got the uint8_t from. This makes more sense.\n\nthe type in PCRE2 is uint8_t, the documentation has a bug[1]\nalmost every platform git cares for would have unsigned char = uint8_t\nthough.\n\n> The point of 6d4b5747f0 was not to only split out those variables we\n> couldn't get away with re-using. Then I would have later re-used\n> e.g. pcre1_jit_on & pcre2_jit_on as just pcre_jit_on. We could also do\n> that now.\n\nIMHO pcre_jit_on makes more sense as a bit, with local variables being\nused for the pcre*_config() call with the right type.(uint32_t != int)\n\n> I think doing that & this part of the your changes makes things less\n> readable. The two code branches we compile with ifdefs are mutually\n> exclusive, so having the variables be unique helps with eyeballing /\n> reasoning when changing the code.\n\nI thought that too until the typo[2] in the pcre?_jit_on variable (now\nin next) kind of\nproved us wrong.  Maybe the names are too similar?\n\neither way, would you rather drop this patch and make a replica variable?\nshould I rebase to next with Reviewed-By tags so that all other changes\nin flight that would conflict could be corrected?\n\nCarlo\n\n[1] https://bugs.exim.org/show_bug.cgi?id=2420\n[2] https://public-inbox.org/git/20190722181923.21572-1-dev+git@drbeat.li/\n"},{"id":"379550","messageId":"7ec60d57-9940-35f2-f7b5-c87d4dc7cdd5@web.de","threadId":"51538","inReplyTo":"CAPUEspgowrhj56SzW8cPKjZL28nPTkX-RScKrSAai8i0YkwFaA@mail.gmail.com","subject":"Re: [PATCH 3/3] grep: plug leak of pcre chartables in PCRE2","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2019-07-29T20:34:52Z","receivedAt":"2019-07-29T20:35:07Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 28.07.19 um 03:41 schrieb Carlo Arenas:\n> On Sat, Jul 27, 2019 at 4:48 PM Ævar Arnfjörð Bjarmason\n> <avarab@gmail.com> wrote:\n>>> +     free((void *)p->pcre_tables);\n>>\n>> Is the cast really needed? I'm rusty on the rules, removing it from the\n>> pcre_free() you might have copied this from produces a warning for me,\n>> but not for free() itself. This is on GCC 8.3.0. How about for you &\n>> what compiler(s)?\n>\n> both will trigger warnings for the same reason\n> (-Wincompatible-pointer-types-discards-qualifiers)\n> with Apple LLVM version 10.0.1 (clang-1001.0.46.4)\n>\n> gcc-9 in macOS triggers 2 \"warnings\"; one for discarding the const\n> qualifier (-Wdiscarded-qualifiers)\n> and another for mismatching parameter to free():\n>\n> note: expected 'void *' but argument is of type 'const uint8_t *' {aka\n> 'const unsigned char *'}\n\nRight: pcre_maketables() returns a const pointer, and you have to cast\naway this const'ness at some point if you want to free(3) that memory.\nReturning a non-const pointer would be more fitting, but I guess the\nidea is that users of the library are not supposed to change the\ncontents of the table.\n\nBut wouldn't it be more correct to use pcre_free()?  As long as we keep\npcre_malloc() and pcre_free() at their default values it doesn't matter\nin practice, but using free(3) directly is a layering violation, no?\n\nPerhaps just UNLEAK that thing?  There is only a single way to build it\nand we can reuse it throughout the lifetime of the program, so there is\nno real need to clean it up before the OS does.\n\nRené\n"},{"id":"379565","messageId":"CAPUEspgRUymY0Tm0zvfoSsYm0dKh8fy=fW-jN4Roid6oZVY86Q@mail.gmail.com","threadId":"51538","inReplyTo":"7ec60d57-9940-35f2-f7b5-c87d4dc7cdd5@web.de","subject":"Re: [PATCH 3/3] grep: plug leak of pcre chartables in PCRE2","fromName":"Carlo Arenas","fromEmail":"carenas@gmail.com","sentAt":"2019-07-30T00:08:55Z","receivedAt":"2019-07-30T00:09:09Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"On Mon, Jul 29, 2019 at 1:35 PM René Scharfe <l.s.r@web.de> wrote:\n>\n> Am 28.07.19 um 03:41 schrieb Carlo Arenas:\n> > On Sat, Jul 27, 2019 at 4:48 PM Ævar Arnfjörð Bjarmason\n> > <avarab@gmail.com> wrote:\n> >>> +     free((void *)p->pcre_tables);\n> >>\n> >> Is the cast really needed? I'm rusty on the rules, removing it from the\n> >> pcre_free() you might have copied this from produces a warning for me,\n> >> but not for free() itself. This is on GCC 8.3.0. How about for you &\n> >> what compiler(s)?\n> >\n> > both will trigger warnings for the same reason\n> > (-Wincompatible-pointer-types-discards-qualifiers)\n> > with Apple LLVM version 10.0.1 (clang-1001.0.46.4)\n> >\n> > gcc-9 in macOS triggers 2 \"warnings\"; one for discarding the const\n> > qualifier (-Wdiscarded-qualifiers)\n> > and another for mismatching parameter to free():\n> >\n> > note: expected 'void *' but argument is of type 'const uint8_t *' {aka\n> > 'const unsigned char *'}\n>\n> Right: pcre_maketables() returns a const pointer, and you have to cast\n> away this const'ness at some point if you want to free(3) that memory.\n> Returning a non-const pointer would be more fitting, but I guess the\n> idea is that users of the library are not supposed to change the\n> contents of the table.\n\nnote that this pointer was generated by pcre2_maketables() instead\nwhich is actually \"const uint8_t *\", but yes these tables are meant to\nbe inmutable and that is why they are \"cost\".\n\nthere is even a compile time generated table for the C locale that is\nused internally and obviously shouldn't be free().\n\n> But wouldn't it be more correct to use pcre_free()?  As long as we keep\n> pcre_malloc() and pcre_free() at their default values it doesn't matter\n> in practice, but using free(3) directly is a layering violation, no?\n\nyes, but that is the only option PCRE2 gives when not using a global\ncontext which is what the comment in the commit refers to.\n\nFWIW pcre_free() doesn't exist anymore in PCRE2.\n\n> Perhaps just UNLEAK that thing?  There is only a single way to build it\n> and we can reuse it throughout the lifetime of the program, so there is\n> no real need to clean it up before the OS does.\n\nThat would be a better fit if it would be created once in cmd_grep and\nthen shared with all worker threads (which I thought would be nice to\ndo in the future anyway), but this change was trying to be conservative\nand just to the minimum to close the leak.\n\nCarlo\n"},{"id":"379582","messageId":"dff4a832-e8a8-789f-04cd-b11fd1671c52@web.de","threadId":"51538","inReplyTo":"CAPUEspgRUymY0Tm0zvfoSsYm0dKh8fy=fW-jN4Roid6oZVY86Q@mail.gmail.com","subject":"Re: [PATCH 3/3] grep: plug leak of pcre chartables in PCRE2","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2019-07-30T16:52:41Z","receivedAt":"2019-07-30T16:52:50Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 30.07.19 um 02:08 schrieb Carlo Arenas:\n> On Mon, Jul 29, 2019 at 1:35 PM René Scharfe <l.s.r@web.de> wrote:\n>>\n>> Am 28.07.19 um 03:41 schrieb Carlo Arenas:\n>>> On Sat, Jul 27, 2019 at 4:48 PM Ævar Arnfjörð Bjarmason\n>>> <avarab@gmail.com> wrote:\n>>>>> +     free((void *)p->pcre_tables);\n>>>>\n>>>> Is the cast really needed? I'm rusty on the rules, removing it from the\n>>>> pcre_free() you might have copied this from produces a warning for me,\n>>>> but not for free() itself. This is on GCC 8.3.0. How about for you &\n>>>> what compiler(s)?\n>>>\n>>> both will trigger warnings for the same reason\n>>> (-Wincompatible-pointer-types-discards-qualifiers)\n>>> with Apple LLVM version 10.0.1 (clang-1001.0.46.4)\n>>>\n>>> gcc-9 in macOS triggers 2 \"warnings\"; one for discarding the const\n>>> qualifier (-Wdiscarded-qualifiers)\n>>> and another for mismatching parameter to free():\n>>>\n>>> note: expected 'void *' but argument is of type 'const uint8_t *' {aka\n>>> 'const unsigned char *'}\n>>\n>> Right: pcre_maketables() returns a const pointer, and you have to cast\n>> away this const'ness at some point if you want to free(3) that memory.\n>> Returning a non-const pointer would be more fitting, but I guess the\n>> idea is that users of the library are not supposed to change the\n>> contents of the table.\n>\n> note that this pointer was generated by pcre2_maketables() instead\n> which is actually \"const uint8_t *\", but yes these tables are meant to\n> be inmutable and that is why they are \"cost\".\n\nOnly the const'ness matters in regards to the need for casting a pointer\nto feed to free(3).\n\nDoing the cast in a library function or using an opaque pointer type\ninstead of a fake const pointer would be nicer ways on the part of PCRE2\nto keep callers from messing with the table.  Forcing users to cast\nconst away or leak is not very nice..  Nothing we can do about it,\nexcept perhaps adding a wish list item for PCRE3, I guess.\n\nhttps://pcre.org/current/doc/html/pcre2_maketables.html says that\npcre2_maketables() returns const unsigned char *, by the way.  I don't\nget the PCRE2_SUFFIX business in pcre2.h (\"Define macros that generate\nwidth-specific names from generic versions.\"), though.\n\n>> But wouldn't it be more correct to use pcre_free()?  As long as we keep\n>> pcre_malloc() and pcre_free() at their default values it doesn't matter\n>> in practice, but using free(3) directly is a layering violation, no?\n>\n> yes, but that is the only option PCRE2 gives when not using a global\n> context which is what the comment in the commit refers to.\n>\n> FWIW pcre_free() doesn't exist anymore in PCRE2.\n\nOK, and while https://pcre.org/original/doc/html/pcreapi.html#TOC1\nsays that pcre_malloc() is used by pcre_maketables() (and thus the\nresult should be passed to pcre_free() after use),\nhttps://pcre.org/current/doc/html/pcre2_maketables.html says\npcre2_maketables() uses malloc(3), so that pointer needs to go to\nfree(3) at the end.  Missed the second part in my earlier reply.\n\n>> Perhaps just UNLEAK that thing?  There is only a single way to build it\n>> and we can reuse it throughout the lifetime of the program, so there is\n>> no real need to clean it up before the OS does.\n>\n> That would be a better fit if it would be created once in cmd_grep and\n> then shared with all worker threads (which I thought would be nice to\n> do in the future anyway), but this change was trying to be conservative\n> and just to the minimum to close the leak.\n\nSure.  I wonder how sharing between threads would influence performance..\n\nRené\n"},{"id":"379759","messageId":"20190801170946.81221-1-carenas@gmail.com","threadId":"51538","inReplyTo":"20190727202759.22310-1-carenas@gmail.com","subject":"[PATCH v2] grep: avoid leak of chartables in PCRE2","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2019-08-01T17:09:46Z","receivedAt":"2019-08-01T17:09:58Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"94da9193a6 (\"grep: add support for PCRE v2\", 2017-06-01) introduced\na small memory leak visible with valgrind in t7813.\n\nComplete the creation of a PCRE2 specific variable that was missing from\nthe original change and free the generated table just like it is done\nfor PCRE1.\n\nThe table cleanup use free as there is no global context defined when\nit was created (pcre2_maketables is passed a NULL pointer) but if that\ngets ever changed will need to be updated in tandem.\n\nSigned-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>\n---\nV2:\n* better document why free is used as suggested by René\n* avoid reusing PCRE1 variable for easy of maintenance (per Ævar)\n\n grep.c | 7 ++++---\n grep.h | 1 +\n 2 files changed, 5 insertions(+), 3 deletions(-)\n\ndiff --git a/grep.c b/grep.c\nindex f7c3a5803e..fbd3f3757c 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -488,7 +488,6 @@ static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt\n \tPCRE2_UCHAR errbuf[256];\n \tPCRE2_SIZE erroffset;\n \tint options = PCRE2_MULTILINE;\n-\tconst uint8_t *character_tables = NULL;\n \tint jitret;\n \tint patinforet;\n \tsize_t jitsizearg;\n@@ -499,9 +498,10 @@ static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt\n \n \tif (opt->ignore_case) {\n \t\tif (has_non_ascii(p->pattern)) {\n-\t\t\tcharacter_tables = pcre2_maketables(NULL);\n+\t\t\tp->pcre2_tables = pcre2_maketables(NULL);\n \t\t\tp->pcre2_compile_context = pcre2_compile_context_create(NULL);\n-\t\t\tpcre2_set_character_tables(p->pcre2_compile_context, character_tables);\n+\t\t\tpcre2_set_character_tables(p->pcre2_compile_context,\n+\t\t\t\t\t\t\tp->pcre2_tables);\n \t\t}\n \t\toptions |= PCRE2_CASELESS;\n \t}\n@@ -605,6 +605,7 @@ static void free_pcre2_pattern(struct grep_pat *p)\n \tpcre2_match_data_free(p->pcre2_match_data);\n \tpcre2_jit_stack_free(p->pcre2_jit_stack);\n \tpcre2_match_context_free(p->pcre2_match_context);\n+\tfree((void *)p->pcre2_tables);\n }\n #else /* !USE_LIBPCRE2 */\n static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt)\ndiff --git a/grep.h b/grep.h\nindex 1875880f37..26d21a3433 100644\n--- a/grep.h\n+++ b/grep.h\n@@ -96,6 +96,7 @@ struct grep_pat {\n \tpcre2_compile_context *pcre2_compile_context;\n \tpcre2_match_context *pcre2_match_context;\n \tpcre2_jit_stack *pcre2_jit_stack;\n+\tconst uint8_t *pcre2_tables;\n \tuint32_t pcre2_jit_on;\n \tkwset_t kws;\n \tunsigned fixed:1;\n-- \n2.22.0\n\n"},{"id":"379834","messageId":"xmqqwofvleuv.fsf@gitster-ct.c.googlers.com","threadId":"51538","inReplyTo":"20190801170946.81221-1-carenas@gmail.com","subject":"Re: [PATCH v2] grep: avoid leak of chartables in PCRE2","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-08-02T16:19:20Z","receivedAt":"2019-08-02T16:19:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Carlo Marcelo Arenas Belón  <carenas@gmail.com> writes:\n\n> 94da9193a6 (\"grep: add support for PCRE v2\", 2017-06-01) introduced\n> a small memory leak visible with valgrind in t7813.\n>\n> Complete the creation of a PCRE2 specific variable that was missing from\n> the original change and free the generated table just like it is done\n> for PCRE1.\n>\n> The table cleanup use free as there is no global context defined when\n> it was created (pcre2_maketables is passed a NULL pointer) but if that\n> gets ever changed will need to be updated in tandem.\n>\n> Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>\n> ---\n> V2:\n> * better document why free is used as suggested by René\n> * avoid reusing PCRE1 variable for easy of maintenance (per Ævar)\n\nThanks; will queue.\n\n>\n>  grep.c | 7 ++++---\n>  grep.h | 1 +\n>  2 files changed, 5 insertions(+), 3 deletions(-)\n>\n> diff --git a/grep.c b/grep.c\n> index f7c3a5803e..fbd3f3757c 100644\n> --- a/grep.c\n> +++ b/grep.c\n> @@ -488,7 +488,6 @@ static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt\n>  \tPCRE2_UCHAR errbuf[256];\n>  \tPCRE2_SIZE erroffset;\n>  \tint options = PCRE2_MULTILINE;\n> -\tconst uint8_t *character_tables = NULL;\n>  \tint jitret;\n>  \tint patinforet;\n>  \tsize_t jitsizearg;\n> @@ -499,9 +498,10 @@ static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt\n>  \n>  \tif (opt->ignore_case) {\n>  \t\tif (has_non_ascii(p->pattern)) {\n> -\t\t\tcharacter_tables = pcre2_maketables(NULL);\n> +\t\t\tp->pcre2_tables = pcre2_maketables(NULL);\n>  \t\t\tp->pcre2_compile_context = pcre2_compile_context_create(NULL);\n> -\t\t\tpcre2_set_character_tables(p->pcre2_compile_context, character_tables);\n> +\t\t\tpcre2_set_character_tables(p->pcre2_compile_context,\n> +\t\t\t\t\t\t\tp->pcre2_tables);\n>  \t\t}\n>  \t\toptions |= PCRE2_CASELESS;\n>  \t}\n> @@ -605,6 +605,7 @@ static void free_pcre2_pattern(struct grep_pat *p)\n>  \tpcre2_match_data_free(p->pcre2_match_data);\n>  \tpcre2_jit_stack_free(p->pcre2_jit_stack);\n>  \tpcre2_match_context_free(p->pcre2_match_context);\n> +\tfree((void *)p->pcre2_tables);\n>  }\n>  #else /* !USE_LIBPCRE2 */\n>  static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt)\n> diff --git a/grep.h b/grep.h\n> index 1875880f37..26d21a3433 100644\n> --- a/grep.h\n> +++ b/grep.h\n> @@ -96,6 +96,7 @@ struct grep_pat {\n>  \tpcre2_compile_context *pcre2_compile_context;\n>  \tpcre2_match_context *pcre2_match_context;\n>  \tpcre2_jit_stack *pcre2_jit_stack;\n> +\tconst uint8_t *pcre2_tables;\n>  \tuint32_t pcre2_jit_on;\n>  \tkwset_t kws;\n>  \tunsigned fixed:1;\n"},{"id":"379883","messageId":"CAPUEsph2S1+PpET6R-=YnTyvm8+5t5ytJL6VJR0nbmyMQWcYuw@mail.gmail.com","threadId":"51538","inReplyTo":"xmqqwofvleuv.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2] grep: avoid leak of chartables in PCRE2","fromName":"Carlo Arenas","fromEmail":"carenas@gmail.com","sentAt":"2019-08-03T18:50:51Z","receivedAt":"2019-08-03T18:56:56Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"Junio\n\nThanks for fixing the conflicts in pu as well; apologize if I could\nhad made it easier by doings things differently\n\nCarlo\n"},{"id":"379951","messageId":"xmqqr25zmmn3.fsf@gitster-ct.c.googlers.com","threadId":"51538","inReplyTo":"CAPUEsph2S1+PpET6R-=YnTyvm8+5t5ytJL6VJR0nbmyMQWcYuw@mail.gmail.com","subject":"Re: [PATCH v2] grep: avoid leak of chartables in PCRE2","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-08-05T19:34:56Z","receivedAt":"2019-08-05T19:35:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Carlo Arenas <carenas@gmail.com> writes:\n\n> Thanks for fixing the conflicts in pu as well; apologize if I could\n> had made it easier by doings things differently\n\nWith many topics in flight, conflicts between topics happen all the\ntime.  Thanks for checking to catch a mismerge.\n\n"}]}