Re: [PATCH 3/3] grep: plug leak of pcre chartables in PCRE2

From: René Scharfe <l.s.r@web.de>
Date: Jul 29, 2019, 20:34 UTC
Message-ID: <7ec60d57-9940-35f2-f7b5-c87d4dc7cdd5@web.de>
In-Reply-To: <CAPUEspgowrhj56SzW8cPKjZL28nPTkX-RScKrSAai8i0YkwFaA@mail.gmail.com>

Am 28.07.19 um 03:41 schrieb Carlo Arenas:

Show 19 quoted lines

> On Sat, Jul 27, 2019 at 4:48 PM Ævar Arnfjörð Bjarmason
> <avarab@gmail.com> wrote:
>>> +     free((void *)p->pcre_tables);
>>
>> Is the cast really needed? I'm rusty on the rules, removing it from the
>> pcre_free() you might have copied this from produces a warning for me,
>> but not for free() itself. This is on GCC 8.3.0. How about for you &
>> what compiler(s)?
>
> both will trigger warnings for the same reason
> (-Wincompatible-pointer-types-discards-qualifiers)
> with Apple LLVM version 10.0.1 (clang-1001.0.46.4)
>
> gcc-9 in macOS triggers 2 "warnings"; one for discarding the const
> qualifier (-Wdiscarded-qualifiers)
> and another for mismatching parameter to free():
>
> note: expected 'void *' but argument is of type 'const uint8_t *' {aka
> 'const unsigned char *'}

Right: pcre_maketables() returns a const pointer, and you have to cast
away this const'ness at some point if you want to free(3) that memory.
Returning a non-const pointer would be more fitting, but I guess the
idea is that users of the library are not supposed to change the
contents of the table.

But wouldn't it be more correct to use pcre_free()? As long as we keep pcre_malloc() and pcre_free() at their default values it doesn't matter in practice, but using free(3) directly is a layering violation, no?

Perhaps just UNLEAK that thing? There is only a single way to build it and we can reuse it throughout the lifetime of the program, so there is no real need to clean it up before the OS does.

René