Re: [PATCH v2 00/21] Various memory leak fixes

[Junio C Hamano <gitster@pobox.com>]

Junio C Hamano <gitster@pobox.com> writes:

> Patrick Steinhardt <ps@pks.im> writes:
>
>> Indeed. The following diff fixes the leak:
>>
>>     diff --git a/builtin/update-ref.c b/builtin/update-ref.c
>>     index 7d2a419230..e54be9c429 100644
>>     --- a/builtin/update-ref.c
>>     +++ b/builtin/update-ref.c
>>     @@ -130,6 +130,8 @@ static char *parse_next_arg(const char **next)
>>      
>>         if (arg.len)
>>             return strbuf_detach(&arg, NULL);
>>     +
>>     +	strbuf_release(&arg);
>>         return NULL;
>>      }
>>      
>>
>> Karthik is out of office this week, so you may want to add this as a
>> "SQUASH???" commit on top of his topic branch to make "seen" pass.
>
> Alright.  Thanks.

But I am curious.  How would this leak even be possible?  

arg.len==0 and arg.buf != strbuf_slopbuf would mean that somebody
who touched arg (either parse_arg() call or strbuf_addstr() call)
allocated arg.buf but ended up not adding any byte (or added some
bytes but at the end rewound with "arg.len = 0").

Ahh, strbuf_addstr() does an unconditional strbuf_grow().  I wonder
if the following patch is a good idea in general, then (not as a
replacement for your fix, but as a general code hygiene---unless
you know you need more memory, don't allocate).

I have a suspicion that this is optimizing for a wrong case, though,
so I'd probably refrain from committing it.  If the caller often has
an empty string, and if the caller feels like it is worth checking
to omit such an extra "opportunistic" allocation, the caller can do
so.

 strbuf.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git c/strbuf.c w/strbuf.c
index 0d929e4e19..c0e58f5b95 100644
--- c/strbuf.c
+++ w/strbuf.c
@@ -308,6 +308,8 @@ void strbuf_remove(struct strbuf *sb, size_t pos, size_t len)
 
 void strbuf_add(struct strbuf *sb, const void *data, size_t len)
 {
+	if (!len)
+		return;
 	strbuf_grow(sb, len);
 	memcpy(sb->buf + sb->len, data, len);
 	strbuf_setlen(sb, sb->len + len);
@@ -315,6 +317,8 @@ void strbuf_add(struct strbuf *sb, const void *data, size_t len)
 
 void strbuf_addbuf(struct strbuf *sb, const struct strbuf *sb2)
 {
+	if (!sb2->len)
+		return;
 	strbuf_grow(sb, sb2->len);
 	memcpy(sb->buf + sb->len, sb2->buf, sb2->len);
 	strbuf_setlen(sb, sb->len + sb2->len);
@@ -337,6 +341,8 @@ const char *strbuf_join_argv(struct strbuf *buf,
 
 void strbuf_addchars(struct strbuf *sb, int c, size_t n)
 {
+	if (!n)
+		return;
 	strbuf_grow(sb, n);
 	memset(sb->buf + sb->len, c, n);
 	strbuf_setlen(sb, sb->len + n);
@@ -805,6 +811,8 @@ void strbuf_addstr_xml_quoted(struct strbuf *buf, const char *s)
 static void strbuf_add_urlencode(struct strbuf *sb, const char *s, size_t len,
 				 char_predicate allow_unencoded_fn)
 {
+	if (!len)
+		return;
 	strbuf_grow(sb, len);
 	while (len--) {
 		char ch = *s++;