# Fix another file leak 5 messages from 2011-10-07 to 2011-10-07. Participants: Chris Wilson, René Scharfe, Tay Ray Chuan. Thread: https://gitlist.dev/t/28619 ## Chris Wilson, 2011-10-07 01:41 Subject: Fix another file leak Message-ID: <20111007014136.GB10839@localhost> URL: https://gitlist.dev/e/20111007014136.GB10839%40localhost ``` Hi, Vigilant Sentry (our C/C++ static analysis tool) found that commit 6d4bb383, added a file leak to builtin/fetch.c. static int store_updated_refs(... { FILE *fp; ... fp = fopen(filename, "a"); if (!fp) return error(_("cannot open %s: %s\n"), filename, strerror(errno)); .... if (check_everything_connected(iterate_ref_map, 0, &rm)) return error(_("%s did not send all necessary objects\n"), url); Please close the file handle before returning from the function. Thanks, Chris -- Chris Wilson http://vigilantsw.com/ Vigilant Software, LLC ``` ## René Scharfe, 2011-10-07 06:13 Subject: [PATCH] fetch: plug two leaks on error exit in store_updated_refs Message-ID: <4E8E98A7.8010008@lsrfire.ath.cx> URL: https://gitlist.dev/e/4E8E98A7.8010008%40lsrfire.ath.cx In-Reply-To: <20111007014136.GB10839@localhost> ``` Close FETCH_HEAD and release the string url even if we have to leave the function store_updated_refs() early. Reported-by: Chris Wilson Signed-off-by: Rene Scharfe --- builtin/fetch.c | 8 ++++++-- 1 files changed, 6 insertions(+), 2 deletions(-) diff --git a/builtin/fetch.c b/builtin/fetch.c index 7a4e41c..79db796 100644 --- a/builtin/fetch.c +++ b/builtin/fetch.c @@ -379,8 +379,12 @@ static int store_updated_refs(const char *raw_url, const char *remote_name, url = xstrdup("foreign"); rm = ref_map; - if (check_everything_connected(iterate_ref_map, 0, &rm)) - return error(_("%s did not send all necessary objects\n"), url); + if (check_everything_connected(iterate_ref_map, 0, &rm)) { + error(_("%s did not send all necessary objects\n"), url); + free(url); + fclose(fp); + return -1; + } for (rm = ref_map; rm; rm = rm->next) { struct ref *ref = NULL; -- 1.7.7 ``` ## Tay Ray Chuan, 2011-10-07 06:49 Subject: Re: [PATCH] fetch: plug two leaks on error exit in store_updated_refs Message-ID: URL: https://gitlist.dev/e/CALUzUxp4Eo7j%3DkM7YPJbj70-rwuyFK5V1mZZMY7vBwwPYWS6gQ%40mail.gmail.com In-Reply-To: <4E8E98A7.8010008@lsrfire.ath.cx> ``` On Fri, Oct 7, 2011 at 2:13 PM, René Scharfe wrote: > diff --git a/builtin/fetch.c b/builtin/fetch.c > index 7a4e41c..79db796 100644 > --- a/builtin/fetch.c > +++ b/builtin/fetch.c > @@ -379,8 +379,12 @@ static int store_updated_refs(const char *raw_url, const char *remote_name, >                url = xstrdup("foreign"); > >        rm = ref_map; > -       if (check_everything_connected(iterate_ref_map, 0, &rm)) > -               return error(_("%s did not send all necessary objects\n"), url); > +       if (check_everything_connected(iterate_ref_map, 0, &rm)) { > +               error(_("%s did not send all necessary objects\n"), url); > +               free(url); > +               fclose(fp); > +               return -1; > +       } > >        for (rm = ref_map; rm; rm = rm->next) { >                struct ref *ref = NULL; > -- > 1.7.7 How about reusing the function's cleanup calls, like this? -- >8 -- diff --git a/builtin/fetch.c b/builtin/fetch.c index fc254b6..56267c4 100644 --- a/builtin/fetch.c +++ b/builtin/fetch.c @@ -423,8 +423,10 @@ static int store_updated_refs(const char *raw_url, const char *remote_name, else url = xstrdup("foreign"); - if (check_everything_connected(ref_map, 0)) - return error(_("%s did not send all necessary objects\n"), url); + if (check_everything_connected(ref_map, 0)) { + rc = error(_("%s did not send all necessary objects\n"), url); + goto abort; + } for (rm = ref_map; rm; rm = rm->next) { struct ref *ref = NULL; @@ -506,12 +508,15 @@ static int store_updated_refs(const char *raw_url, const char *remote_name, fprintf(stderr, " %s\n", note); } } - free(url); - fclose(fp); + if (rc & STORE_REF_ERROR_DF_CONFLICT) error(_("some local refs could not be updated; try running\n" " 'git remote prune %s' to remove any old, conflicting " "branches"), remote_name); + +abort: + free(url); + fclose(fp); return rc; } -- -- Cheers, Ray Chuan ``` ## René Scharfe, 2011-10-07 06:59 Subject: Re: [PATCH] fetch: plug two leaks on error exit in store_updated_refs Message-ID: <4E8EA33E.5020009@lsrfire.ath.cx> URL: https://gitlist.dev/e/4E8EA33E.5020009%40lsrfire.ath.cx In-Reply-To: ``` Am 07.10.2011 08:49, schrieb Tay Ray Chuan: > How about reusing the function's cleanup calls, like this? Yes, that's better. > -- >8 -- > diff --git a/builtin/fetch.c b/builtin/fetch.c > index fc254b6..56267c4 100644 > --- a/builtin/fetch.c > +++ b/builtin/fetch.c > @@ -423,8 +423,10 @@ static int store_updated_refs(const char > *raw_url, const char *remote_name, > else > url = xstrdup("foreign"); > > - if (check_everything_connected(ref_map, 0)) > - return error(_("%s did not send all necessary objects\n"), url); > + if (check_everything_connected(ref_map, 0)) { > + rc = error(_("%s did not send all necessary objects\n"), url); > + goto abort; > + } > > for (rm = ref_map; rm; rm = rm->next) { > struct ref *ref = NULL; > @@ -506,12 +508,15 @@ static int store_updated_refs(const char > *raw_url, const char *remote_name, > fprintf(stderr, " %s\n", note); > } > } > - free(url); > - fclose(fp); > + > if (rc & STORE_REF_ERROR_DF_CONFLICT) > error(_("some local refs could not be updated; try running\n" > " 'git remote prune %s' to remove any old, conflicting " > "branches"), remote_name); > + > +abort: > + free(url); > + fclose(fp); > return rc; > } > Micro-nit: If you start the label with a space ("+ abort:") then the code continues to play nice with git grep -W. René ``` ## Tay Ray Chuan, 2011-10-07 07:40 Subject: [PATCH v2] fetch: plug two leaks on error exit in store_updated_refs Message-ID: <1317973222-7340-1-git-send-email-rctay89@gmail.com> URL: https://gitlist.dev/e/1317973222-7340-1-git-send-email-rctay89%40gmail.com In-Reply-To: <4E8EA33E.5020009@lsrfire.ath.cx> ``` Close FETCH_HEAD and release the string url even if we have to leave the function store_updated_refs() early. Reported-by: Chris Wilson Helped-by: Rene Scharfe Signed-off-by: Tay Ray Chuan --- builtin/fetch.c | 13 +++++++++---- 1 files changed, 9 insertions(+), 4 deletions(-) diff --git a/builtin/fetch.c b/builtin/fetch.c index fc254b6..9b7ce10 100644 --- a/builtin/fetch.c +++ b/builtin/fetch.c @@ -423,8 +423,10 @@ static int store_updated_refs(const char *raw_url, const char *remote_name, else url = xstrdup("foreign"); - if (check_everything_connected(ref_map, 0)) - return error(_("%s did not send all necessary objects\n"), url); + if (check_everything_connected(ref_map, 0)) { + rc = error(_("%s did not send all necessary objects\n"), url); + goto abort; + } for (rm = ref_map; rm; rm = rm->next) { struct ref *ref = NULL; @@ -506,12 +508,15 @@ static int store_updated_refs(const char *raw_url, const char *remote_name, fprintf(stderr, " %s\n", note); } } - free(url); - fclose(fp); + if (rc & STORE_REF_ERROR_DF_CONFLICT) error(_("some local refs could not be updated; try running\n" " 'git remote prune %s' to remove any old, conflicting " "branches"), remote_name); + + abort: + free(url); + fclose(fp); return rc; } -- 1.7.7.584.g16d0ea ```