* Fix another file leak
@ 2011-10-07 1:41 Chris Wilson
2011-10-07 6:13 ` [PATCH] fetch: plug two leaks on error exit in store_updated_refs René Scharfe
0 siblings, 1 reply; 5+ messages in thread
From: Chris Wilson @ 2011-10-07 1:41 UTC (permalink / raw)
To: git
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
^ permalink raw reply [flat|nested] 5+ messages in thread* [PATCH] fetch: plug two leaks on error exit in store_updated_refs 2011-10-07 1:41 Fix another file leak Chris Wilson @ 2011-10-07 6:13 ` René Scharfe 2011-10-07 6:49 ` Tay Ray Chuan 0 siblings, 1 reply; 5+ messages in thread From: René Scharfe @ 2011-10-07 6:13 UTC (permalink / raw) To: Chris Wilson; +Cc: git, Junio C Hamano Close FETCH_HEAD and release the string url even if we have to leave the function store_updated_refs() early. Reported-by: Chris Wilson <cwilson@vigilantsw.com> Signed-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx> --- 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 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] fetch: plug two leaks on error exit in store_updated_refs 2011-10-07 6:13 ` [PATCH] fetch: plug two leaks on error exit in store_updated_refs René Scharfe @ 2011-10-07 6:49 ` Tay Ray Chuan 2011-10-07 6:59 ` René Scharfe 0 siblings, 1 reply; 5+ messages in thread From: Tay Ray Chuan @ 2011-10-07 6:49 UTC (permalink / raw) To: René Scharfe; +Cc: Chris Wilson, git, Junio C Hamano On Fri, Oct 7, 2011 at 2:13 PM, René Scharfe <rene.scharfe@lsrfire.ath.cx> 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 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] fetch: plug two leaks on error exit in store_updated_refs 2011-10-07 6:49 ` Tay Ray Chuan @ 2011-10-07 6:59 ` René Scharfe 2011-10-07 7:40 ` [PATCH v2] " Tay Ray Chuan 0 siblings, 1 reply; 5+ messages in thread From: René Scharfe @ 2011-10-07 6:59 UTC (permalink / raw) To: Tay Ray Chuan; +Cc: Chris Wilson, git, Junio C Hamano 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é ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH v2] fetch: plug two leaks on error exit in store_updated_refs 2011-10-07 6:59 ` René Scharfe @ 2011-10-07 7:40 ` Tay Ray Chuan 0 siblings, 0 replies; 5+ messages in thread From: Tay Ray Chuan @ 2011-10-07 7:40 UTC (permalink / raw) To: git; +Cc: Junio C Hamano, Chris Wilson, René Scharfe Close FETCH_HEAD and release the string url even if we have to leave the function store_updated_refs() early. Reported-by: Chris Wilson <cwilson@vigilantsw.com> Helped-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx> Signed-off-by: Tay Ray Chuan <rctay89@gmail.com> --- 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 ^ permalink raw reply related [flat|nested] 5+ messages in thread
end of thread, other threads:[~2011-10-07 7:40 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2011-10-07 1:41 Fix another file leak Chris Wilson 2011-10-07 6:13 ` [PATCH] fetch: plug two leaks on error exit in store_updated_refs René Scharfe 2011-10-07 6:49 ` Tay Ray Chuan 2011-10-07 6:59 ` René Scharfe 2011-10-07 7:40 ` [PATCH v2] " Tay Ray Chuan
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).