| Message ID | 324140e4fdee54fbf0054e76abb287b7212fd21e.1723121979.git.ps@pks.im (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Memory leak fixes (pt.4) | expand |
On Thu, Aug 08, 2024 at 03:04:39PM +0200, Patrick Steinhardt wrote: > 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
On Mon, Aug 12, 2024 at 10:11:05AM -0400, Taylor Blau wrote: > On Thu, Aug 08, 2024 at 03:04:39PM +0200, Patrick Steinhardt wrote: > > 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): It is cleaner indeed, thanks for the proposal! Patrick
Patrick Steinhardt <ps@pks.im> writes: >> Makes sense, though I wonder if this would be slightly cleaner to write >> like so (applies on top of this patch): > > It is cleaner indeed, thanks for the proposal! Yup, it is only 3 repetitions but they are repetitions nevertheless. A small helper function that gets rid of them is worth it. Thanks.
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")) {
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(-)