Re: [PATCH 2/2] remote: fix trivial memory leak

2013-10-15 Thread Junio C Hamano
Jeff King p...@peff.net writes:

 I wondered if we might also leak when seeing duplicate config options
 (i.e., leaking the old one when replacing it with the new). But we don't
 actually strdup() the configured remote names, but instead just point
 into the struct branch, which owns the data.

In addition, we do not copy this string to remote-name in make_remote(),
so even if we start allowing destruction of existing remote[], the
resulting code will stay safe.

 So I think an even better fix would be:

 -- 8 --
 Subject: remote: do not copy origin string literal

 Our default_remote_name starts at origin, but may be
 overridden by the config file. In the former case, we
 allocate a new string, but in the latter case, we point to
 the remote name in an existing struct branch.

 This gives the variable inconsistent free() semantics (we
 are sometimes responsible for freeing the string and
 sometimes pointing to somebody else's storage), and causes a
 small leak when the allocated string is overridden by
 config.

 We can fix both by simply dropping the extra copy and
 pointing to the string literal.

 Noticed-by: Felipe Contreras felipe.contre...@gmail.com
 Signed-off-by: Jeff King p...@peff.net

Sounds sensible. Thanks.

 ---
  remote.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

 diff --git a/remote.c b/remote.c
 index e9fedfa..9f1a8aa 100644
 --- a/remote.c
 +++ b/remote.c
 @@ -483,7 +483,7 @@ static void read_config(void)
   int flag;
   if (default_remote_name) /* did this already */
   return;
 - default_remote_name = xstrdup(origin);
 + default_remote_name = origin;
   current_branch = NULL;
   head_ref = resolve_ref_unsafe(HEAD, sha1, 0, flag);
   if (head_ref  (flag  REF_ISSYMREF) 
--
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 2/2] remote: fix trivial memory leak

2013-09-23 Thread Jeff King
On Sat, Sep 21, 2013 at 09:09:23AM -0500, Felipe Contreras wrote:

 diff --git a/remote.c b/remote.c
 index efcba93..654e7f5 100644
 --- a/remote.c
 +++ b/remote.c
 @@ -480,7 +480,6 @@ static void read_config(void)
   int flag;
   if (default_remote_name) /* did this already */
   return;
 - default_remote_name = xstrdup(origin);
   current_branch = NULL;
   head_ref = resolve_ref_unsafe(HEAD, sha1, 0, flag);
   if (head_ref  (flag  REF_ISSYMREF) 
 @@ -489,6 +488,8 @@ static void read_config(void)
   make_branch(head_ref + strlen(refs/heads/), 0);
   }
   git_config(handle_config, NULL);
 + if (!default_remote_name)
 + default_remote_name = xstrdup(origin);
   alias_all_urls();

I wondered if we might also leak when seeing duplicate config options
(i.e., leaking the old one when replacing it with the new). But we don't
actually strdup() the configured remote names, but instead just point
into the struct branch, which owns the data.

So I think an even better fix would be:

-- 8 --
Subject: remote: do not copy origin string literal

Our default_remote_name starts at origin, but may be
overridden by the config file. In the former case, we
allocate a new string, but in the latter case, we point to
the remote name in an existing struct branch.

This gives the variable inconsistent free() semantics (we
are sometimes responsible for freeing the string and
sometimes pointing to somebody else's storage), and causes a
small leak when the allocated string is overridden by
config.

We can fix both by simply dropping the extra copy and
pointing to the string literal.

Noticed-by: Felipe Contreras felipe.contre...@gmail.com
Signed-off-by: Jeff King p...@peff.net
---
 remote.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/remote.c b/remote.c
index e9fedfa..9f1a8aa 100644
--- a/remote.c
+++ b/remote.c
@@ -483,7 +483,7 @@ static void read_config(void)
int flag;
if (default_remote_name) /* did this already */
return;
-   default_remote_name = xstrdup(origin);
+   default_remote_name = origin;
current_branch = NULL;
head_ref = resolve_ref_unsafe(HEAD, sha1, 0, flag);
if (head_ref  (flag  REF_ISSYMREF) 
-- 
1.8.4.rc3.19.g9da5bf6

--
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