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

From: Eric Sunshine <sunshine@sunshineco.com>
Date: May 22, 2018, 02:20 UTC
Message-ID: <CAPig+cQevUoYJOisFt9BDhLP1_SjPqX+2hmFKDPcBvAh2D7ksA@mail.gmail.com>
In-Reply-To: <CAGZ79kZotwAFauTkCJ6YZ_C-MuaQpNaaS8LCniL_Or=_ccfC4w@mail.gmail.com>

On Mon, May 21, 2018 at 2:43 PM, Stefan Beller <sbeller@google.com> wrote:

Show 20 quoted lines

> 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. [...]
>>
>> diff --git 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);
>>                 die("invalid regex: %s", errbuf);
>
> 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?

The commit message doesn't say that we are supposedly introducing a leak (and, indeed, no resources should have been allocated to the 'regex' upon failed compile). It's saying that removing this call potentially avoids a crash under some implementations.

Given that the very next line is die(), and that the function name has "_or_die" in it, I'm not sure that an in-code comment about regfree() being invalid upon failed compile would be all that helpful; indeed, it could be confusing, causing the reader to wonder why that is significant if we're just dying anyhow. I find that the patch, as is, clarifies rather than muddles the situation.