Re: [PATCH v2 02/22] git: fix leaking system paths

From: Taylor Blau <me@ttaylorr.com>
Date: Aug 12, 2024, 14:11 UTC
Message-ID: <ZroX+SSgTe7YnWHS@nand.local>
In-Reply-To: <324140e4fdee54fbf0054e76abb287b7212fd21e.1723121979.git.ps@pks.im>

On Thu, Aug 08, 2024 at 03:04:39PM +0200, Patrick Steinhardt wrote:

Show 43 quoted lines

> Git has some flags to make it output system paths as they have been
> compiled into Git. This is done by calling `system_path()`, which
> returns an allocated string. This string isn't ever free'd though,
> creating a memory leak.
>
> Plug those leaks. While they are surfaced by t0211, there are more
> memory leaks looming exposed by that test suite and it thus does not yet
> pass with the memory leak checker enabled.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  git.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/git.c b/git.c
> index e35af9b0e5..5eab88b472 100644
> --- a/git.c
> +++ b/git.c
> @@ -173,15 +173,21 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)
>  				exit(0);
>  			}
>  		} else if (!strcmp(cmd, "--html-path")) {
> -			puts(system_path(GIT_HTML_PATH));
> +			char *path = system_path(GIT_HTML_PATH);
> +			puts(path);
> +			free(path);
>  			trace2_cmd_name("_query_");
>  			exit(0);
>  		} else if (!strcmp(cmd, "--man-path")) {
> -			puts(system_path(GIT_MAN_PATH));
> +			char *path = system_path(GIT_MAN_PATH);
> +			puts(path);
> +			free(path);
>  			trace2_cmd_name("_query_");
>  			exit(0);
>  		} else if (!strcmp(cmd, "--info-path")) {
> -			puts(system_path(GIT_INFO_PATH));
> +			char *path = system_path(GIT_INFO_PATH);
> +			puts(path);
> +			free(path);
>  			trace2_cmd_name("_query_");
>  			exit(0);
>  		} else if (!strcmp(cmd, "-p") || !strcmp(cmd, "--paginate")) {

Makes sense, though I wonder if this would be slightly cleaner to write like so (applies on top of this patch):

--- 8< ---

diff --git a/git.c b/git.c
index 5eab88b472..9a618a2740 100644
--- a/git.c
+++ b/git.c
@@ -143,6 +143,13 @@ void setup_auto_pager(const char *cmd, int def)
 	commit_pager_choice();
 }

+static void print_system_path(const char *path)
+{
+	char *s_path = system_path(path);
+	puts(s_path);
+	free(s_path);
+}
+
 static int handle_options(const char ***argv, int *argc, int *envchanged)
 {
 	const char **orig_argv = *argv;
@@ -173,21 +180,15 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)
 				exit(0);
 			}
 		} else if (!strcmp(cmd, "--html-path")) {
-			char *path = system_path(GIT_HTML_PATH);
-			puts(path);
-			free(path);
+			print_system_path(GIT_HTML_PATH);
 			trace2_cmd_name("_query_");
 			exit(0);
 		} else if (!strcmp(cmd, "--man-path")) {
-			char *path = system_path(GIT_MAN_PATH);
-			puts(path);
-			free(path);
+			print_system_path(GIT_MAN_PATH);
 			trace2_cmd_name("_query_");
 			exit(0);
 		} else if (!strcmp(cmd, "--info-path")) {
-			char *path = system_path(GIT_INFO_PATH);
-			puts(path);
-			free(path);
+			print_system_path(GIT_INFO_PATH);
 			trace2_cmd_name("_query_");
 			exit(0);
 		} else if (!strcmp(cmd, "-p") || !strcmp(cmd, "--paginate")) {
--- >8 ---

Thanks,
Taylor