Re: [PATCH 1/2] remote.c: don't leak the base branch name in format_tracking_info
On 10.08.2014 15:57, Stefan Beller wrote: Found by scan.coverity.com (Id: 1127809) Signed-off-by: Stefan Beller stefanbel...@gmail.com --- remote.c | 1 + 1 file changed, 1 insertion(+) diff --git a/remote.c b/remote.c index 3d6c86a..2c1458f 100644 --- a/remote.c +++ b/remote.c @@ -1983,6 +1983,7 @@ int format_tracking_info(struct branch *branch, struct strbuf *sb) strbuf_addf(sb, _( (use \git pull\ to merge the remote branch into yours)\n)); } + free(base); return 1; } Upon testing this one again, I get a warning remote.c: In function ‘format_tracking_info’: remote.c:1986:2: warning: passing argument 1 of ‘free’ discards ‘const’ qualifier from pointer target type [enabled by default] free(base); ^ In file included from git-compat-util.h:103:0, from cache.h:4, from remote.c:1: /usr/include/stdlib.h:483:13: note: expected ‘void *’ but argument is of type ‘const char *’ extern void free (void *__ptr) __THROW; ^ Please ignore this patch. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] remote.c: don't leak the base branch name in format_tracking_info
On Sun, Aug 10, 2014 at 05:14:33PM +0200, Stefan Beller wrote: On 10.08.2014 15:57, Stefan Beller wrote: Found by scan.coverity.com (Id: 1127809) Signed-off-by: Stefan Beller stefanbel...@gmail.com --- remote.c | 1 + 1 file changed, 1 insertion(+) diff --git a/remote.c b/remote.c index 3d6c86a..2c1458f 100644 --- a/remote.c +++ b/remote.c @@ -1983,6 +1983,7 @@ int format_tracking_info(struct branch *branch, struct strbuf *sb) strbuf_addf(sb, _( (use \git pull\ to merge the remote branch into yours)\n)); } + free(base); return 1; } Upon testing this one again, I get a warning remote.c: In function ‘format_tracking_info’: remote.c:1986:2: warning: passing argument 1 of ‘free’ discards ‘const’ qualifier from pointer target type [enabled by default] free(base); ^ In file included from git-compat-util.h:103:0, from cache.h:4, from remote.c:1: /usr/include/stdlib.h:483:13: note: expected ‘void *’ but argument is of type ‘const char *’ extern void free (void *__ptr) __THROW; ^ Please ignore this patch. I think your patch is definitely fixing a leak; it's just that the existing code is a little sloppy. It does: const char *base; ... base = branch-merge[0]-dst; base = shorten_unambiguous_ref(base, 0); In the first assignment, base should be const, as we are pointing to somebody else's memory. But in the second, we use the same pointer to store newly allocated memory from shorten_unambiguous_ref. In the general case, you need two pointers to do this right. However, we don't actually look at base between the two assignments, so I think you could just do it as: char *base; ... base = shorten_unambiguous_ref(branch-merge[0]-dst, 0); -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] remote.c: don't leak the base branch name in format_tracking_info
Stefan Beller stefanbel...@gmail.com writes: On 10.08.2014 15:57, Stefan Beller wrote: Found by scan.coverity.com (Id: 1127809) Signed-off-by: Stefan Beller stefanbel...@gmail.com --- remote.c | 1 + 1 file changed, 1 insertion(+) diff --git a/remote.c b/remote.c index 3d6c86a..2c1458f 100644 --- a/remote.c +++ b/remote.c @@ -1983,6 +1983,7 @@ int format_tracking_info(struct branch *branch, struct strbuf *sb) strbuf_addf(sb, _( (use \git pull\ to merge the remote branch into yours)\n)); } +free(base); return 1; } Upon testing this one again, I get a warning remote.c: In function ‘format_tracking_info’: remote.c:1986:2: warning: passing argument 1 of ‘free’ discards ‘const’ qualifier from pointer target type [enabled by default] free(base); ^ ... Please ignore this patch. It is perfectly fine to cast it to (char *) in this case, I think. The real culprit is that the functionà reuses the same base (which is a pointer into a constant region of memory) to receive the new copy allocated by shorten_unambiguous_ref(); the piece of memory returned by the callee belongs to this function, and it is perfectly fine if this function modifies the contents of it (which it doesn't happen to do). Storing the returned value to a variable of type const char * does not absolve it from the responsibility to free it (hence your patch). -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
