git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH 1/3] grep: make pcre1_tables version agnostic

From
Carlo Arenas <carenas@gmail.com>
Date
Jul 28, 2019, 02:50 UTC
Message-ID
<CAPUEspg255TAE-1ozc8CuC=k9oeP_WLD4TrFur5p=XY3s8PX3Q@mail.gmail.com>
In-Reply-To
<875znn6nvt.fsf@evledraar.gmail.com>

On Sat, Jul 27, 2019 at 4:47 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:

Show 8 quoted lines
>
> On Sat, Jul 27 2019, Carlo Marcelo Arenas Belón wrote:
>
> > 6d4b5747f0 ("grep: change internal *pcre* variable & function names
> > to be *pcre1*", 2017-05-25), renamed most variables to be PCRE1
> > specific to give space to similarly named variables for PCRE2, but
> > in this case the change wasn't needed as the types were compatible
> > enough (const unsigned char* vs const uint8_t*) to be shared.

reading this again, had to admit there is a fair amount of guessing on intent, but reading commits and the email discussion couldn't come up with a better explanation.

is the root cause for the bug different?, could it be that the pcre2 API was misunderstood? and you expected this pointer will be free together with the context? (as it is done when a cloned context with tables is used?)

> Both the v1 and v2 functions return const unsigned char *. I don't know
> where I got the uint8_t from. This makes more sense.

the type in PCRE2 is uint8_t, the documentation has a bug[1] almost every platform git cares for would have unsigned char = uint8_t though.

> The point of 6d4b5747f0 was not to only split out those variables we
> couldn't get away with re-using. Then I would have later re-used
> e.g. pcre1_jit_on & pcre2_jit_on as just pcre_jit_on. We could also do
> that now.

IMHO pcre_jit_on makes more sense as a bit, with local variables being used for the pcre*_config() call with the right type.(uint32_t != int)

> I think doing that & this part of the your changes makes things less
> readable. The two code branches we compile with ifdefs are mutually
> exclusive, so having the variables be unique helps with eyeballing /
> reasoning when changing the code.

I thought that too until the typo[2] in the pcre?_jit_on variable (now in next) kind of proved us wrong. Maybe the names are too similar?

either way, would you rather drop this patch and make a replica variable? should I rebase to next with Reviewed-By tags so that all other changes in flight that would conflict could be corrected?

Carlo

[1] https://bugs.exim.org/show_bug.cgi?id=2420 [2] https://public-inbox.org/git/20190722181923.21572-1-dev+git@drbeat.li/

Previous: Ævar Arnfjörð BjarmasonNext: Carlo Marcelo Arenas Belón
Message 4 of 15 in “grep: memory leak in PCRE2”
  1. 0/3 grep: memory leak in PCRE2Carlo Marcelo Arenas Belón, Jul 27, 2019
  2. 1/3 grep: make pcre1_tables version agnosticCarlo Marcelo Arenas Belón, Jul 27, 2019
  3. Ævar Arnfjörð BjarmasonJul 27, 2019
  4. Carlo ArenasJul 28, 2019
  5. 2/3 grep: use pcre_tables also for PCRE2Carlo Marcelo Arenas Belón, Jul 27, 2019
  6. 3/3 grep: plug leak of pcre chartables in PCRE2Carlo Marcelo Arenas Belón, Jul 27, 2019
  7. Ævar Arnfjörð BjarmasonJul 27, 2019
  8. Carlo ArenasJul 28, 2019
  9. René ScharfeJul 29, 2019
  10. Carlo ArenasJul 30, 2019
  11. René ScharfeJul 30, 2019
  12. grep: avoid leak of chartables in PCRE2Carlo Marcelo Arenas Belón, Aug 1, 2019
  13. Junio C HamanoAug 2, 2019
  14. Carlo ArenasAug 3, 2019
  15. Junio C HamanoAug 5, 2019

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.