Re: [PATCH] regex: do not call `regfree()` if compilation fails

[Stefan Beller <sbeller@google.com>]

On Sun, May 20, 2018 at 3:50 AM, Martin Ågren <martin.agren@gmail.com> wrote:
> It is apparently undefined behavior to call `regfree()` on a regex where
> `regcomp()` failed. The language in [1] is a bit muddy, at least to me,
> but the clearest hint is this (`preg` is the `regex_t *`):
>
>     Upon successful completion, the regcomp() function shall return 0.
>     Otherwise, it shall return an integer value indicating an error as
>     described in <regex.h>, and the content of preg is undefined.
>
> Funnily enough, there is also the `regerror()` function which should be
> given a pointer to such a "failed" `regex_t` -- the content of which
> would supposedly be undefined -- and which may investigate it to come up
> with a detailed error message.
>
> In any case, the example in that document shows how `regfree()` is not
> called after `regcomp()` fails.
>
> We have quite a few users of this API and most get this right. These
> three users do not.
>
> Several implementations can handle this just fine [2] and these code paths
> supposedly have not wreaked havoc or we'd have heard about it. (These
> are all in code paths where git got bad input and is just about to die
> anyway.) But let's just avoid the issue altogether.
>
> [1] http://pubs.opengroup.org/onlinepubs/9699919799/functions/regcomp.html
>
> [2] https://www.redhat.com/archives/libvir-list/2013-September/msg00262.html
>
> Researched-by: Eric Sunshine <sunshine@sunshineco.com>
> Signed-off-byi Martin Ågren <martin.agren@gmail.com>
> ---
>
> On 14 May 2018 at 05:05, Eric Sunshine <sunshine@sunshineco.com> wrote:
>> My research (for instance [1,2]) seems to indicate that it would be
>> better to avoid regfree() upon failed regcomp(), even though such a
>> situation is handled sanely in some implementations.
>>
>> [1]: https://www.redhat.com/archives/libvir-list/2013-September/msg00276.html
>> [2]: https://www.redhat.com/archives/libvir-list/2013-September/msg00273.html
>
> Thank you for researching this. I think it would make sense to get rid
> of the few places we have where we get this wrong (if our understanding
> of this being undefined is right).
>
>  diffcore-pickaxe.c | 1 -
>  grep.c             | 2 --
>  2 files changed, 3 deletions(-)
>
> diff --git a/diffcore-pickaxe.c b/diffcore-pickaxe.c
> index 239ce5122b..800a899c86 100644
> --- a/diffcore-pickaxe.c
> +++ b/diffcore-pickaxe.c
> @@ -215,7 +215,6 @@ static void regcomp_or_die(regex_t *regex, const char *needle, int cflags)
>                 /* The POSIX.2 people are surely sick */
>                 char errbuf[1024];
>                 regerror(err, regex, errbuf, 1024);
> -               regfree(regex);

While the commit message is very clear why we supposedly introduce a leak here,
it is hard to be found from the source code (as we only delete code
there, so digging
for history is not obvious), so maybe

     /* regfree(regex) is invalid here */

instead?

>                 die("invalid regex: %s", errbuf);
>         }
>  }
> diff --git a/grep.c b/grep.c
> index 65b90c10a3..5e4f3f9a9d 100644
> --- a/grep.c
> +++ b/grep.c
> @@ -636,7 +636,6 @@ static void compile_fixed_regexp(struct grep_pat *p, struct grep_opt *opt)
>         if (err) {
>                 char errbuf[1024];
>                 regerror(err, &p->regexp, errbuf, sizeof(errbuf));
> -               regfree(&p->regexp);
>                 compile_regexp_failed(p, errbuf);
>         }
>  }
> @@ -701,7 +700,6 @@ static void compile_regexp(struct grep_pat *p, struct grep_opt *opt)
>         if (err) {
>                 char errbuf[1024];
>                 regerror(err, &p->regexp, errbuf, 1024);
> -               regfree(&p->regexp);
>                 compile_regexp_failed(p, errbuf);
>         }
>  }
> --
> 2.17.0.840.g5d83f92caf
>