Re: [PATCH 5/9] test-tool regex: call regfree(), fix memory leaks

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 1, 2022, 02:17 UTC
Message-ID: <xmqq8rpda850.fsf@gitster.g>
In-Reply-To: <patch-5.9-fe2a8d898f6-20220630T180129Z-avarab@gmail.com>

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

Show 18 quoted lines

> diff --git a/t/helper/test-regex.c b/t/helper/test-regex.c
> index d6f28ca8d14..a37d1f7a546 100644
> --- a/t/helper/test-regex.c
> +++ b/t/helper/test-regex.c
> @@ -24,27 +24,35 @@ static int test_regex_bug(void)
>  	char *str = "={}\nfred";
>  	regex_t r;
>  	regmatch_t m[1];
> +	int err = 0;
>  
>  	if (regcomp(&r, pat, REG_EXTENDED | REG_NEWLINE))
>  		die("failed regcomp() for pattern '%s'", pat);
> -	if (regexec(&r, str, 1, m, 0))
> -		die("no match of pattern '%s' to string '%s'", pat, str);
> +	if (regexec(&r, str, 1, m, 0)) {
> +		err = error("no match of pattern '%s' to string '%s'", pat, str);
> +		goto cleanup;
> +	}

Hmph. Does it count as leaking to die() while r is still on the stack? I do not mind cleaning everything up thoroughly, but I am curious if this is really necessary. Apparently it is, as can be seen in the patch to t/t7812-grep-icase-non-ascii.sh that allows us to say "PASSES" with this patch.

Show 17 quoted lines

> @@ -85,27 +93,29 @@ int cmd__regex(int argc, const char **argv)
>  	}
>  	git_setup_gettext();
>  
> -	ret = regcomp(&r, pat, flags);
> -	if (ret) {
> +	if (regcomp(&r, pat, flags)) {
>  		if (silent)
> -			return ret;
> +			return 1;
>  
>  		regerror(ret, &r, errbuf, sizeof(errbuf));
>  		die("failed regcomp() for pattern '%s' (%s)", pat, errbuf);
>  	}
>  	if (!str)
> -		return 0;
> +		goto cleanup;

Ahh, OK, this one does not bother with regfree() at all. The changes to the other one may be irrelevant but the changes to this function would be necessary to mark the test with "PASSES".

Show 33 quoted lines

> -	ret = regexec(&r, str, 1, m, 0);
> -	if (ret) {
> +	if (regexec(&r, str, 1, m, 0)) {
> +		ret = 1;
>  		if (silent || ret == REG_NOMATCH)
> -			return ret;
> +			goto cleanup;
>  
>  		regerror(ret, &r, errbuf, sizeof(errbuf));
> -		die("failed regexec() for subject '%s' (%s)", str, errbuf);
> +		error("failed regexec() for subject '%s' (%s)", str, errbuf);
> +		goto cleanup;
>  	}
>  
> -	return 0;
> +cleanup:
> +	regfree(&r);
> +	return ret;
>  usage:
>  	usage("\ttest-tool regex --bug\n"
>  	      "\ttest-tool regex [--silent] <pattern>\n"
> diff --git a/t/t7812-grep-icase-non-ascii.sh b/t/t7812-grep-icase-non-ascii.sh
> index ac7be547145..31c66b63c2c 100755
> --- a/t/t7812-grep-icase-non-ascii.sh
> +++ b/t/t7812-grep-icase-non-ascii.sh
> @@ -2,6 +2,7 @@
>  
>  test_description='grep icase on non-English locales'
>  
> +TEST_PASSES_SANITIZE_LEAK=true
>  . ./lib-gettext.sh
>  
>  doalarm () {