[PATCH 0/3] ref-filter: reduce memory leaks

[PATCH 0/3] ref-filter: reduce memory leaks

[Оля Тележная]

Reduce memory leaks in ref-filter.c.
We still have leaks, but at least not so much.

I use command valgrind --leak-check=full -v ./git for-each-ref to check results.

Before:
==24727== LEAK SUMMARY:
==24727==    definitely lost: 69,424 bytes in 724 blocks
==24727==    indirectly lost: 29,643 bytes in 723 blocks
==24727==      possibly lost: 120 bytes in 3 blocks
==24727==    still reachable: 16,535 bytes in 185 blocks
==24727==         suppressed: 0 bytes in 0 blocks
==24727== Reachable blocks (those to which a pointer was found) are not shown.
==24727== To see them, rerun with: --leak-check=full --show-leak-kinds=all
==24727==
==24727== ERROR SUMMARY: 107 errors from 107 contexts (suppressed: 0 from 0)
==24727== ERROR SUMMARY: 107 errors from 107 contexts (suppressed: 0 from 0)

After:
==16419== LEAK SUMMARY:
==16419==    definitely lost: 17,144 bytes in 1,447 blocks
==16419==    indirectly lost: 0 bytes in 0 blocks
==16419==      possibly lost: 120 bytes in 3 blocks
==16419==    still reachable: 16,217 bytes in 181 blocks
==16419==         suppressed: 0 bytes in 0 blocks
==16419== Reachable blocks (those to which a pointer was found) are not shown.
==16419== To see them, rerun with: --leak-check=full --show-leak-kinds=all
==16419==
==16419== ERROR SUMMARY: 82 errors from 82 contexts (suppressed: 0 from 0)
==16419== ERROR SUMMARY: 82 errors from 82 contexts (suppressed: 0 from 0)

Travis build failed, but it also failed in master, so I guess it's not my fault.
https://github.com/git/git/pull/538

Thank you!
Olga

[PATCH 2/3] ls-remote: release memory instead of UNLEAK

[Olga Telezhnaya]

Use ref_array_clear() to release memory instead of UNLEAK macros.

Signed-off-by: Olga Telezhnaia <olyatelezhnaya@gmail.com>
---
 builtin/ls-remote.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/ls-remote.c b/builtin/ls-remote.c
index 1a25df7ee15b4..6a0cdec30d2d7 100644
--- a/builtin/ls-remote.c
+++ b/builtin/ls-remote.c
@@ -151,6 +151,6 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
 	}
 
 	UNLEAK(sorting);
-	UNLEAK(ref_array);
+	ref_array_clear(&ref_array);
 	return status;
 }

--
https://github.com/git/git/pull/538

[PATCH 1/3] ref-filter: free memory from used_atom

[Olga Telezhnaya]

Release memory from used_atom variable.

Signed-off-by: Olga Telezhnaia <olyatelezhnaya@gmail.com>
---
 ref-filter.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/ref-filter.c b/ref-filter.c
index e1bcb4ca8a197..1b71d08a43a84 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1996,6 +1996,9 @@ void ref_array_clear(struct ref_array *array)
 {
 	int i;
 
+	for (i = 0; i < used_atom_cnt; i++)
+		free((char *)used_atom[i].name);
+	free(used_atom);
 	for (i = 0; i < array->nr; i++)
 		free_array_item(array->items[i]);
 	FREE_AND_NULL(array->items);

--
https://github.com/git/git/pull/538

[PATCH 3/3] ref-filter: free item->value and item->value->s

[Olga Telezhnaya]

Release item->value.
Initialize item->value->s dynamically and then release its resources.
Release some local variables.

Signed-off-by: Olga Telezhnaia <olyatelezhnaya@gmail.com>
---
 ref-filter.c | 95 +++++++++++++++++++++++++++++-----------------------
 1 file changed, 53 insertions(+), 42 deletions(-)

diff --git a/ref-filter.c b/ref-filter.c
index 1b71d08a43a84..4d42c777a9b50 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -875,7 +875,7 @@ static void grab_common_values(struct atom_value *val, int deref, struct expand_
 		if (deref)
 			name++;
 		if (!strcmp(name, "objecttype"))
-			v->s = type_name(oi->type);
+			v->s = xstrdup(type_name(oi->type));
 		else if (!strcmp(name, "objectsize")) {
 			v->value = oi->size;
 			v->s = xstrfmt("%lu", oi->size);
@@ -899,9 +899,9 @@ static void grab_tag_values(struct atom_value *val, int deref, struct object *ob
 		if (deref)
 			name++;
 		if (!strcmp(name, "tag"))
-			v->s = tag->tag;
+			v->s = xstrdup(tag->tag);
 		else if (!strcmp(name, "type") && tag->tagged)
-			v->s = type_name(tag->tagged->type);
+			v->s = xstrdup(type_name(tag->tagged->type));
 		else if (!strcmp(name, "object") && tag->tagged)
 			v->s = xstrdup(oid_to_hex(&tag->tagged->oid));
 	}
@@ -1032,7 +1032,7 @@ static void grab_date(const char *buf, struct atom_value *v, const char *atomnam
 	v->value = timestamp;
 	return;
  bad:
-	v->s = "";
+	v->s = xstrdup("");
 	v->value = 0;
 }
 
@@ -1227,7 +1227,7 @@ static void fill_missing_values(struct atom_value *val)
 	for (i = 0; i < used_atom_cnt; i++) {
 		struct atom_value *v = &val[i];
 		if (v->s == NULL)
-			v->s = "";
+			v->s = xstrdup("");
 	}
 }
 
@@ -1273,7 +1273,8 @@ static inline char *copy_advance(char *dst, const char *src)
 static const char *lstrip_ref_components(const char *refname, int len)
 {
 	long remaining = len;
-	const char *start = refname;
+	const char *start = xstrdup(refname);
+	const char *to_free = start;
 
 	if (len < 0) {
 		int i;
@@ -1294,20 +1295,24 @@ static const char *lstrip_ref_components(const char *refname, int len)
 	while (remaining > 0) {
 		switch (*start++) {
 		case '\0':
-			return "";
+			free((char *)to_free);
+			return xstrdup("");
 		case '/':
 			remaining--;
 			break;
 		}
 	}
 
+	start = xstrdup(start);
+	free((char *)to_free);
 	return start;
 }
 
 static const char *rstrip_ref_components(const char *refname, int len)
 {
 	long remaining = len;
-	char *start = xstrdup(refname);
+	const char *start = xstrdup(refname);
+	const char *to_free = start;
 
 	if (len < 0) {
 		int i;
@@ -1327,9 +1332,10 @@ static const char *rstrip_ref_components(const char *refname, int len)
 
 	while (remaining-- > 0) {
 		char *p = strrchr(start, '/');
-		if (p == NULL)
-			return "";
-		else
+		if (p == NULL) {
+			free((char *)to_free);
+			return xstrdup("");
+		} else
 			p[0] = '\0';
 	}
 	return start;
@@ -1344,7 +1350,7 @@ static const char *show_ref(struct refname_atom *atom, const char *refname)
 	else if (atom->option == R_RSTRIP)
 		return rstrip_ref_components(refname, atom->rstrip);
 	else
-		return refname;
+		return xstrdup(refname);
 }
 
 static void fill_remote_ref_details(struct used_atom *atom, const char *refname,
@@ -1358,7 +1364,7 @@ static void fill_remote_ref_details(struct used_atom *atom, const char *refname,
 				       NULL, AHEAD_BEHIND_FULL) < 0) {
 			*s = xstrdup(msgs.gone);
 		} else if (!num_ours && !num_theirs)
-			*s = "";
+			*s = xstrdup("");
 		else if (!num_ours)
 			*s = xstrfmt(msgs.behind, num_theirs);
 		else if (!num_theirs)
@@ -1373,36 +1379,31 @@ static void fill_remote_ref_details(struct used_atom *atom, const char *refname,
 		}
 	} else if (atom->u.remote_ref.option == RR_TRACKSHORT) {
 		if (stat_tracking_info(branch, &num_ours, &num_theirs,
-				       NULL, AHEAD_BEHIND_FULL) < 0)
+				       NULL, AHEAD_BEHIND_FULL) < 0) {
+			*s = xstrdup("");
 			return;
-
+		}
 		if (!num_ours && !num_theirs)
-			*s = "=";
+			*s = xstrdup("=");
 		else if (!num_ours)
-			*s = "<";
+			*s = xstrdup("<");
 		else if (!num_theirs)
-			*s = ">";
+			*s = xstrdup(">");
 		else
-			*s = "<>";
+			*s = xstrdup("<>");
 	} else if (atom->u.remote_ref.option == RR_REMOTE_NAME) {
 		int explicit;
 		const char *remote = atom->u.remote_ref.push ?
 			pushremote_for_branch(branch, &explicit) :
 			remote_for_branch(branch, &explicit);
-		if (explicit)
-			*s = xstrdup(remote);
-		else
-			*s = "";
+		*s = explicit ? xstrdup(remote) : xstrdup("");
 	} else if (atom->u.remote_ref.option == RR_REMOTE_REF) {
 		int explicit;
 		const char *merge;
 
 		merge = remote_ref_for_branch(branch, atom->u.remote_ref.push,
 					      &explicit);
-		if (explicit)
-			*s = xstrdup(merge);
-		else
-			*s = "";
+		*s = explicit ? xstrdup(merge) : xstrdup("");
 	} else
 		BUG("unhandled RR_* enum");
 }
@@ -1451,7 +1452,7 @@ char *get_head_description(void)
 static const char *get_symref(struct used_atom *atom, struct ref_array_item *ref)
 {
 	if (!ref->symref)
-		return "";
+		return xstrdup("");
 	else
 		return show_ref(&atom->u.refname, ref->symref);
 }
@@ -1510,7 +1511,7 @@ static int populate_value(struct ref_array_item *ref, struct strbuf *err)
 		ref->symref = resolve_refdup(ref->refname, RESOLVE_REF_READING,
 					     NULL, NULL);
 		if (!ref->symref)
-			ref->symref = "";
+			ref->symref = xstrdup("");
 	}
 
 	/* Fill in specials first */
@@ -1536,20 +1537,23 @@ static int populate_value(struct ref_array_item *ref, struct strbuf *err)
 			refname = get_symref(atom, ref);
 		else if (starts_with(name, "upstream")) {
 			const char *branch_name;
-			v->s = "";
 			/* only local branches may have an upstream */
 			if (!skip_prefix(ref->refname, "refs/heads/",
-					 &branch_name))
+					 &branch_name)) {
+				v->s = xstrdup("");
 				continue;
+			}
 			branch = branch_get(branch_name);
 
 			refname = branch_get_upstream(branch, NULL);
 			if (refname)
 				fill_remote_ref_details(atom, refname, branch, &v->s);
+			else
+				v->s = xstrdup("");
 			continue;
 		} else if (atom->u.remote_ref.push) {
 			const char *branch_name;
-			v->s = "";
+			v->s = xstrdup("");
 			if (!skip_prefix(ref->refname, "refs/heads/",
 					 &branch_name))
 				continue;
@@ -1562,10 +1566,11 @@ static int populate_value(struct ref_array_item *ref, struct strbuf *err)
 				if (!refname)
 					continue;
 			}
+			free((char *)v->s); // we will definitely re-init it on the next line
 			fill_remote_ref_details(atom, refname, branch, &v->s);
 			continue;
 		} else if (starts_with(name, "color:")) {
-			v->s = atom->u.color;
+			v->s = xstrdup(atom->u.color);
 			continue;
 		} else if (!strcmp(name, "flag")) {
 			char buf[256], *cp = buf;
@@ -1574,7 +1579,7 @@ static int populate_value(struct ref_array_item *ref, struct strbuf *err)
 			if (ref->flag & REF_ISPACKED)
 				cp = copy_advance(cp, ",packed");
 			if (cp == buf)
-				v->s = "";
+				v->s = xstrdup("");
 			else {
 				*cp = '\0';
 				v->s = xstrdup(buf + 1);
@@ -1584,40 +1589,42 @@ static int populate_value(struct ref_array_item *ref, struct strbuf *err)
 			continue;
 		} else if (!strcmp(name, "HEAD")) {
 			if (atom->u.head && !strcmp(ref->refname, atom->u.head))
-				v->s = "*";
+				v->s = xstrdup("*");
 			else
-				v->s = " ";
+				v->s = xstrdup(" ");
 			continue;
 		} else if (starts_with(name, "align")) {
 			v->handler = align_atom_handler;
-			v->s = "";
+			v->s = xstrdup("");
 			continue;
 		} else if (!strcmp(name, "end")) {
 			v->handler = end_atom_handler;
-			v->s = "";
+			v->s = xstrdup("");
 			continue;
 		} else if (starts_with(name, "if")) {
 			const char *s;
-			v->s = "";
 			if (skip_prefix(name, "if:", &s))
 				v->s = xstrdup(s);
+			else
+				v->s = xstrdup("");
 			v->handler = if_atom_handler;
 			continue;
 		} else if (!strcmp(name, "then")) {
 			v->handler = then_atom_handler;
-			v->s = "";
+			v->s = xstrdup("");
 			continue;
 		} else if (!strcmp(name, "else")) {
 			v->handler = else_atom_handler;
-			v->s = "";
+			v->s = xstrdup("");
 			continue;
 		} else
 			continue;
 
 		if (!deref)
-			v->s = refname;
+			v->s = xstrdup(refname);
 		else
 			v->s = xstrfmt("%s^{}", refname);
+		free((char *)refname);
 	}
 
 	for (i = 0; i < used_atom_cnt; i++) {
@@ -1988,6 +1995,10 @@ static int ref_filter_handler(const char *refname, const struct object_id *oid,
 static void free_array_item(struct ref_array_item *item)
 {
 	free((char *)item->symref);
+	if (item->value) {
+		free((char *)item->value->s);
+		free(item->value);
+	}
 	free(item);
 }
 

--
https://github.com/git/git/pull/538

Re: [PATCH 1/3] ref-filter: free memory from used_atom

[Junio C Hamano]

Olga Telezhnaya <olyatelezhnaya@gmail.com> writes:

> Release memory from used_atom variable.

That much readers would know from a quick look of the patch text.

Without knowing what you are aiming at, it is impossible to judge if
the patch is a good change.

Seeing FREE_AND_NULL(array->items) in the same function makes me
think that the designer of ref_array_clear() function would want
this sequence of events to work correctly in an ideal future:

 * Do a ref-filter operation by calling filter_refs(), receiving the
   result into an array..

 * Do another ref-filter by calling filter_refs(), using different
   criteria, receiving the result into a different array.

 * Iterate over the resulting refs in the first array, and call
   format_ref_array_item().

 * ref_array_clear() the first array, as the caller is done with it.

 * Iterate over the resulting refs in the second array, and call
   format_ref_array_item().

 * ref_array_clear() the second array, as the caller is done with
   it.

However, I think it would make it impossible for the second call to
work correctly if this code freed used_atom without clearing, and
not re-initializing the used_atom_cnt etc.

If on the other hand, the only thing you are interested in is to
just discard pieces of memory we no longer use, and you are not
interested in helping to move us closer to the world in which we can
call filter_refs() twice, then the change this patch makes is
sufficient.

And the place to answer the "what are you aiming at?" question is in
the proposed commit log message.

In an ideal future, I _think_ the file-scope static variables in
ref-filter.c like used_atom and used_atom_cnt should become fields
of a new structure (say "struct ref_filter"), with initializer and
uninitializer ref_filter_new() and ref_filter_destroy().  When that
happens, I think FREE_AND_NULL(used_atom) + used_atom_cnt=0 should
become part of ref_filter_destroy(), not part of ref_array_clear().

But we are not there yet, and a clean-up patch like this does not
have to be a step towards that goal.

> Signed-off-by: Olga Telezhnaia <olyatelezhnaya@gmail.com>
> ---
>  ref-filter.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/ref-filter.c b/ref-filter.c
> index e1bcb4ca8a197..1b71d08a43a84 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -1996,6 +1996,9 @@ void ref_array_clear(struct ref_array *array)
>  {
>  	int i;
>  
> +	for (i = 0; i < used_atom_cnt; i++)
> +		free((char *)used_atom[i].name);
> +	free(used_atom);
>  	for (i = 0; i < array->nr; i++)
>  		free_array_item(array->items[i]);
>  	FREE_AND_NULL(array->items);
>
> --
> https://github.com/git/git/pull/538

Re: [PATCH 3/3] ref-filter: free item->value and item->value->s

[Junio C Hamano]

Olga Telezhnaya <olyatelezhnaya@gmail.com> writes:

> Release item->value.
> Initialize item->value->s dynamically and then release its resources.
> Release some local variables.

Again, "why" is lacking.

> @@ -1373,36 +1379,31 @@ static void fill_remote_ref_details(struct used_atom *atom, const char *refname,
>  		}
>  	} else if (atom->u.remote_ref.option == RR_TRACKSHORT) {
>  		if (stat_tracking_info(branch, &num_ours, &num_theirs,
> -				       NULL, AHEAD_BEHIND_FULL) < 0)
> +				       NULL, AHEAD_BEHIND_FULL) < 0) {
> +			*s = xstrdup("");
>  			return;

It is a bit sad that we need to sprinkle xstrdup() all over the
place, but I do not offhand think of a better alternative to ensure
that it is safe to blindly free the .s field.

> -		if (explicit)
> -			*s = xstrdup(remote);
> -		else
> -			*s = "";
> +		*s = explicit ? xstrdup(remote) : xstrdup("");

Next time, please avoid mixing this kind of unrelated changes with
the main theme (i.e. "the original had allocated and static pieces
of memory pointed by the same variable, which made it impossible to
blindly free it, so make sure everything is allocated").  It makes
it harder to let reviewers' eyes coast over the patch.

I say "Next time" because the change is already written this time,
and I already spent time to see it was an OK change.  By the way,

	*s = xstrdup(explicit ? remote : "");

is probably shorter.

> @@ -1562,10 +1566,11 @@ static int populate_value(struct ref_array_item *ref, struct strbuf *err)
>  				if (!refname)
>  					continue;
>  			}
> +			free((char *)v->s); // we will definitely re-init it on the next line

No // comment, please.

>  static void free_array_item(struct ref_array_item *item)
>  {
>  	free((char *)item->symref);
> +	if (item->value) {
> +		free((char *)item->value->s);
> +		free(item->value);
> +	}
>  	free(item);
>  }

OK.

Re: [PATCH 2/3] ls-remote: release memory instead of UNLEAK

[Junio C Hamano]

Olga Telezhnaya <olyatelezhnaya@gmail.com> writes:

> Use ref_array_clear() to release memory instead of UNLEAK macros.
>
> Signed-off-by: Olga Telezhnaia <olyatelezhnaya@gmail.com>
> ---
>  builtin/ls-remote.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

OK, this is immediately before the command exits, and we have a way
to clear and release the resource, so it is obvious we should use
it.

Good.


>
> diff --git a/builtin/ls-remote.c b/builtin/ls-remote.c
> index 1a25df7ee15b4..6a0cdec30d2d7 100644
> --- a/builtin/ls-remote.c
> +++ b/builtin/ls-remote.c
> @@ -151,6 +151,6 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
>  	}
>  
>  	UNLEAK(sorting);
> -	UNLEAK(ref_array);
> +	ref_array_clear(&ref_array);
>  	return status;
>  }
>
> --
> https://github.com/git/git/pull/538

Re: [PATCH 1/3] ref-filter: free memory from used_atom

[Junio C Hamano]

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

> Olga Telezhnaya <olyatelezhnaya@gmail.com> writes:

These three patches seem to cause t6300 to fail with an attempt to
free an invalid pointer in "git for-each-ref --format='%(push)'"
(6300.25)


*** Error in `/home/gitster/w/git.git/git': free(): invalid pointer: 0x000055cca3a9f920 ***
======= Backtrace: =========
/lib/x86_64-linux-gnu/libc.so.6(+0x70bcb)[0x7f052fdacbcb]
/lib/x86_64-linux-gnu/libc.so.6(+0x76f96)[0x7f052fdb2f96]
/home/gitster/w/git.git/git(+0x15a866)[0x55cca35ca866]
/home/gitster/w/git.git/git(+0x15ab48)[0x55cca35cab48]
/home/gitster/w/git.git/git(+0x15b6d3)[0x55cca35cb6d3]
/home/gitster/w/git.git/git(+0x15b7dd)[0x55cca35cb7dd]
/home/gitster/w/git.git/git(+0x49e18)[0x55cca34b9e18]
/home/gitster/w/git.git/git(+0x19b20)[0x55cca3489b20]
/home/gitster/w/git.git/git(+0x1aab5)[0x55cca348aab5]
/home/gitster/w/git.git/git(+0x19809)[0x55cca3489809]
/lib/x86_64-linux-gnu/libc.so.6(__libc_start_main+0xf1)[0x7f052fd5c2b1]
/home/gitster/w/git.git/git(+0x1984a)[0x55cca348984a]
======= Memory map: ========
55cca3470000-55cca36cc000 r-xp 00000000 fe:00 2760695                    /home/gitster/w/git.git/git
55cca38cc000-55cca38cf000 r--p 0025c000 fe:00 2760695                    /home/gitster/w/git.git/git
55cca38cf000-55cca38de000 rw-p 0025f000 fe:00 2760695                    /home/gitster/w/git.git/git
55cca38de000-55cca3921000 rw-p 00000000 00:00 0 
55cca3a9e000-55cca3abf000 rw-p 00000000 00:00 0                          [heap]
7f052fb24000-7f052fb3b000 r-xp 00000000 fe:00 393287                     /lib/x86_64-linux-gnu/libgcc_s.so.1
7f052fb3b000-7f052fd3a000 ---p 00017000 fe:00 393287                     /lib/x86_64-linux-gnu/libgcc_s.so.1
7f052fd3a000-7f052fd3b000 r--p 00016000 fe:00 393287                     /lib/x86_64-linux-gnu/libgcc_s.so.1
7f052fd3b000-7f052fd3c000 rw-p 00017000 fe:00 393287                     /lib/x86_64-linux-gnu/libgcc_s.so.1
7f052fd3c000-7f052fed1000 r-xp 00000000 fe:00 392469                     /lib/x86_64-linux-gnu/libc-2.24.so
7f052fed1000-7f05300d1000 ---p 00195000 fe:00 392469                     /lib/x86_64-linux-gnu/libc-2.24.so
7f05300d1000-7f05300d5000 r--p 00195000 fe:00 392469                     /lib/x86_64-linux-gnu/libc-2.24.so
7f05300d5000-7f05300d7000 rw-p 00199000 fe:00 392469                     /lib/x86_64-linux-gnu/libc-2.24.so
7f05300d7000-7f05300db000 rw-p 00000000 00:00 0 
7f05300db000-7f05300e2000 r-xp 00000000 fe:00 392487                     /lib/x86_64-linux-gnu/librt-2.24.so
7f05300e2000-7f05302e1000 ---p 00007000 fe:00 392487                     /lib/x86_64-linux-gnu/librt-2.24.so
7f05302e1000-7f05302e2000 r--p 00006000 fe:00 392487                     /lib/x86_64-linux-gnu/librt-2.24.so
7f05302e2000-7f05302e3000 rw-p 00007000 fe:00 392487                     /lib/x86_64-linux-gnu/librt-2.24.so
7f05302e3000-7f05302fb000 r-xp 00000000 fe:00 392485                     /lib/x86_64-linux-gnu/libpthread-2.24.so
7f05302fb000-7f05304fa000 ---p 00018000 fe:00 392485                     /lib/x86_64-linux-gnu/libpthread-2.24.so
7f05304fa000-7f05304fb000 r--p 00017000 fe:00 392485                     /lib/x86_64-linux-gnu/libpthread-2.24.so
7f05304fb000-7f05304fc000 rw-p 00018000 fe:00 392485                     /lib/x86_64-linux-gnu/libpthread-2.24.so
7f05304fc000-7f0530500000 rw-p 00000000 00:00 0 
7f0530500000-7f0530519000 r-xp 00000000 fe:00 392698                     /lib/x86_64-linux-gnu/libz.so.1.2.8
7f0530519000-7f0530718000 ---p 00019000 fe:00 392698                     /lib/x86_64-linux-gnu/libz.so.1.2.8
7f0530718000-7f0530719000 r--p 00018000 fe:00 392698                     /lib/x86_64-linux-gnu/libz.so.1.2.8
7f0530719000-7f053071a000 rw-p 00019000 fe:00 392698                     /lib/x86_64-linux-gnu/libz.so.1.2.8
7f053071a000-7f053073d000 r-xp 00000000 fe:00 392461                     /lib/x86_64-linux-gnu/ld-2.24.so
7f0530916000-7f0530918000 rw-p 00000000 00:00 0 
7f0530939000-7f053093d000 rw-p 00000000 00:00 0 
7f053093d000-7f053093e000 r--p 00023000 fe:00 392461                     /lib/x86_64-linux-gnu/ld-2.24.so
7f053093e000-7f053093f000 rw-p 00024000 fe:00 392461                     /lib/x86_64-linux-gnu/ld-2.24.so
7f053093f000-7f0530940000 rw-p 00000000 00:00 0 
7ffe894ee000-7ffe89510000 rw-p 00000000 00:00 0                          [stack]
7ffe8959e000-7ffe895a1000 r--p 00000000 00:00 0                          [vvar]
7ffe895a1000-7ffe895a3000 r-xp 00000000 00:00 0                          [vdso]
./test-lib.sh: line 631: 262132 Aborted                 git for-each-ref --format='%(push)' refs/heads/master > actual
not ok 25 - basic atom: head push
#	
#			git for-each-ref --format='%(push)' refs/heads/master >actual &&
#			sanitize_pgp <actual >actual.clean &&
#			test_cmp expected actual.clean
#		

Re: [PATCH 3/3] ref-filter: free item->value and item->value->s

[Jeff King]

On Thu, Oct 11, 2018 at 10:19:22AM +0900, Junio C Hamano wrote:

> > @@ -1373,36 +1379,31 @@ static void fill_remote_ref_details(struct used_atom *atom, const char *refname,
> >  		}
> >  	} else if (atom->u.remote_ref.option == RR_TRACKSHORT) {
> >  		if (stat_tracking_info(branch, &num_ours, &num_theirs,
> > -				       NULL, AHEAD_BEHIND_FULL) < 0)
> > +				       NULL, AHEAD_BEHIND_FULL) < 0) {
> > +			*s = xstrdup("");
> >  			return;
> 
> It is a bit sad that we need to sprinkle xstrdup() all over the
> place, but I do not offhand think of a better alternative to ensure
> that it is safe to blindly free the .s field.

I think the root of the problem is that the current code needs
_something_ in the "s" field to know that the value has already been
filled in.

If there were a separate flag for "filled", then this could just be
NULL (and the later code that builds the output would have to realize
to replace that with an empty string).

I think in the long run, we'd ideally have one of two code structures:

  - a single pass, without iterating over the used atoms list
    repeatedly. E.g., the way oid_object_info() takes a struct
    representing the set of items that the caller is interested in, and
    then fills it in as it digs for information.

  - individual atoms could write directly to an output strbuf, avoiding
    the extra allocation and copy altogether (that would help these
    cases that are just copying an empty string, but also the ones that
    really are allocating a piece of data and end up copying it.

I'm OK with this approach in the meantime, though. There's a fair bit of
refactoring to get to either of those end-states, and this clears up the
confusing memory ownership issues. And I don't think for-each-ref is
_too_ sensitive to a few extra mallocs (as compared to say, cat-file's
formatter, which is often run once per object).

I didn't dig into the valgrind errors you saw, but I'd guess they are
the result of this patch missing one of these cases (if not a string
literal like "", perhaps a const pointer into another heap string).

-Peff

Re: [PATCH 1/3] ref-filter: free memory from used_atom

[Jeff King]

On Fri, Oct 12, 2018 at 11:35:01AM +0900, Junio C Hamano wrote:

> Junio C Hamano <gitster@pobox.com> writes:
> 
> > Olga Telezhnaya <olyatelezhnaya@gmail.com> writes:
> 
> These three patches seem to cause t6300 to fail with an attempt to
> free an invalid pointer in "git for-each-ref --format='%(push)'"
> (6300.25)

I dug into this a bit. I think it's actually a misapplication of the
patches on your side. Applying them locally works, but your
ot/ref-filter-plug-leaks branch does not.

The patch below on top of your branch helps. :)

diff --git a/ref-filter.c b/ref-filter.c
index f4ff80eca0..4255de1d75 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1567,7 +1567,6 @@ static int populate_value(struct ref_array_item *ref, struct strbuf *err)
 					continue;
 			}
 			free((char *)v->s); /* we will definitely re-init it on the next line */
-			free((char *)v->s);
 			fill_remote_ref_details(atom, refname, branch, &v->s);
 			continue;
 		} else if (starts_with(name, "color:")) {

Presumably it came from the manual comment-style fixup.

With that fix, the tests run fine for me under ASan/UBSan (with the
exception of t5310, but that's fixed already in a parallel topic).

-Peff

Re: [PATCH 1/3] ref-filter: free memory from used_atom

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Presumably it came from the manual comment-style fixup.

Wow, that was embarrassing.  Thanks for catching it.

>
> With that fix, the tests run fine for me under ASan/UBSan (with the
> exception of t5310, but that's fixed already in a parallel topic).
>
> -Peff

Re: [PATCH 1/3] ref-filter: free memory from used_atom

[Оля Тележная]

чт, 18 окт. 2018 г. в 9:51, Junio C Hamano <gitster@pobox.com>:
>
> Jeff King <peff@peff.net> writes:
>
> > Presumably it came from the manual comment-style fixup.
>
> Wow, that was embarrassing.  Thanks for catching it.

Jeff, thanks a lot!
I just sent new version where I fixed all known issues including that comment.
>
> >
> > With that fix, the tests run fine for me under ASan/UBSan (with the
> > exception of t5310, but that's fixed already in a parallel topic).
> >
> > -Peff

[PATCH v2 2/3] ls-remote: release memory instead of UNLEAK

[Olga Telezhnaya]

Use ref_array_clear() to release memory instead of UNLEAK macros.

Signed-off-by: Olga Telezhnaia <olyatelezhnaya@gmail.com>
---
 builtin/ls-remote.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/ls-remote.c b/builtin/ls-remote.c
index 1a25df7ee15b4..6a0cdec30d2d7 100644
--- a/builtin/ls-remote.c
+++ b/builtin/ls-remote.c
@@ -151,6 +151,6 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
 	}
 
 	UNLEAK(sorting);
-	UNLEAK(ref_array);
+	ref_array_clear(&ref_array);
 	return status;
 }

--
https://github.com/git/git/pull/538

[PATCH v2 1/3] ref-filter: free memory from used_atom

[Olga Telezhnaya]

Release memory from used_atom variable for reducing number of memory
leaks.

Signed-off-by: Olga Telezhnaia <olyatelezhnaya@gmail.com>
---
 ref-filter.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/ref-filter.c b/ref-filter.c
index e1bcb4ca8a197..70f1d13ab3beb 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1996,6 +1996,10 @@ void ref_array_clear(struct ref_array *array)
 {
 	int i;
 
+	for (i = 0; i < used_atom_cnt; i++)
+		free((char *)used_atom[i].name);
+	FREE_AND_NULL(used_atom);
+	used_atom_cnt = 0;
 	for (i = 0; i < array->nr; i++)
 		free_array_item(array->items[i]);
 	FREE_AND_NULL(array->items);

--
https://github.com/git/git/pull/538

[PATCH v2 3/3] ref-filter: free item->value and item->value->s

[Olga Telezhnaya]

Release item->value.
Initialize item->value->s dynamically and then release its resources.
Release some local variables.

Final goal of this patch is to reduce number of memory leaks.

Signed-off-by: Olga Telezhnaia <olyatelezhnaya@gmail.com>
---
 ref-filter.c | 96 +++++++++++++++++++++++++++++-----------------------
 1 file changed, 54 insertions(+), 42 deletions(-)

diff --git a/ref-filter.c b/ref-filter.c
index 70f1d13ab3beb..ca52ee4608c2a 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -875,7 +875,7 @@ static void grab_common_values(struct atom_value *val, int deref, struct expand_
 		if (deref)
 			name++;
 		if (!strcmp(name, "objecttype"))
-			v->s = type_name(oi->type);
+			v->s = xstrdup(type_name(oi->type));
 		else if (!strcmp(name, "objectsize")) {
 			v->value = oi->size;
 			v->s = xstrfmt("%lu", oi->size);
@@ -899,9 +899,9 @@ static void grab_tag_values(struct atom_value *val, int deref, struct object *ob
 		if (deref)
 			name++;
 		if (!strcmp(name, "tag"))
-			v->s = tag->tag;
+			v->s = xstrdup(tag->tag);
 		else if (!strcmp(name, "type") && tag->tagged)
-			v->s = type_name(tag->tagged->type);
+			v->s = xstrdup(type_name(tag->tagged->type));
 		else if (!strcmp(name, "object") && tag->tagged)
 			v->s = xstrdup(oid_to_hex(&tag->tagged->oid));
 	}
@@ -1032,7 +1032,7 @@ static void grab_date(const char *buf, struct atom_value *v, const char *atomnam
 	v->value = timestamp;
 	return;
  bad:
-	v->s = "";
+	v->s = xstrdup("");
 	v->value = 0;
 }
 
@@ -1227,7 +1227,7 @@ static void fill_missing_values(struct atom_value *val)
 	for (i = 0; i < used_atom_cnt; i++) {
 		struct atom_value *v = &val[i];
 		if (v->s == NULL)
-			v->s = "";
+			v->s = xstrdup("");
 	}
 }
 
@@ -1273,7 +1273,8 @@ static inline char *copy_advance(char *dst, const char *src)
 static const char *lstrip_ref_components(const char *refname, int len)
 {
 	long remaining = len;
-	const char *start = refname;
+	const char *start = xstrdup(refname);
+	const char *to_free = start;
 
 	if (len < 0) {
 		int i;
@@ -1294,20 +1295,24 @@ static const char *lstrip_ref_components(const char *refname, int len)
 	while (remaining > 0) {
 		switch (*start++) {
 		case '\0':
-			return "";
+			free((char *)to_free);
+			return xstrdup("");
 		case '/':
 			remaining--;
 			break;
 		}
 	}
 
+	start = xstrdup(start);
+	free((char *)to_free);
 	return start;
 }
 
 static const char *rstrip_ref_components(const char *refname, int len)
 {
 	long remaining = len;
-	char *start = xstrdup(refname);
+	const char *start = xstrdup(refname);
+	const char *to_free = start;
 
 	if (len < 0) {
 		int i;
@@ -1327,9 +1332,10 @@ static const char *rstrip_ref_components(const char *refname, int len)
 
 	while (remaining-- > 0) {
 		char *p = strrchr(start, '/');
-		if (p == NULL)
-			return "";
-		else
+		if (p == NULL) {
+			free((char *)to_free);
+			return xstrdup("");
+		} else
 			p[0] = '\0';
 	}
 	return start;
@@ -1344,7 +1350,7 @@ static const char *show_ref(struct refname_atom *atom, const char *refname)
 	else if (atom->option == R_RSTRIP)
 		return rstrip_ref_components(refname, atom->rstrip);
 	else
-		return refname;
+		return xstrdup(refname);
 }
 
 static void fill_remote_ref_details(struct used_atom *atom, const char *refname,
@@ -1358,7 +1364,7 @@ static void fill_remote_ref_details(struct used_atom *atom, const char *refname,
 				       NULL, AHEAD_BEHIND_FULL) < 0) {
 			*s = xstrdup(msgs.gone);
 		} else if (!num_ours && !num_theirs)
-			*s = "";
+			*s = xstrdup("");
 		else if (!num_ours)
 			*s = xstrfmt(msgs.behind, num_theirs);
 		else if (!num_theirs)
@@ -1373,36 +1379,31 @@ static void fill_remote_ref_details(struct used_atom *atom, const char *refname,
 		}
 	} else if (atom->u.remote_ref.option == RR_TRACKSHORT) {
 		if (stat_tracking_info(branch, &num_ours, &num_theirs,
-				       NULL, AHEAD_BEHIND_FULL) < 0)
+				       NULL, AHEAD_BEHIND_FULL) < 0) {
+			*s = xstrdup("");
 			return;
-
+		}
 		if (!num_ours && !num_theirs)
-			*s = "=";
+			*s = xstrdup("=");
 		else if (!num_ours)
-			*s = "<";
+			*s = xstrdup("<");
 		else if (!num_theirs)
-			*s = ">";
+			*s = xstrdup(">");
 		else
-			*s = "<>";
+			*s = xstrdup("<>");
 	} else if (atom->u.remote_ref.option == RR_REMOTE_NAME) {
 		int explicit;
 		const char *remote = atom->u.remote_ref.push ?
 			pushremote_for_branch(branch, &explicit) :
 			remote_for_branch(branch, &explicit);
-		if (explicit)
-			*s = xstrdup(remote);
-		else
-			*s = "";
+		*s = xstrdup(explicit ? remote : "");
 	} else if (atom->u.remote_ref.option == RR_REMOTE_REF) {
 		int explicit;
 		const char *merge;
 
 		merge = remote_ref_for_branch(branch, atom->u.remote_ref.push,
 					      &explicit);
-		if (explicit)
-			*s = xstrdup(merge);
-		else
-			*s = "";
+		*s = xstrdup(explicit ? merge : "");
 	} else
 		BUG("unhandled RR_* enum");
 }
@@ -1451,7 +1452,7 @@ char *get_head_description(void)
 static const char *get_symref(struct used_atom *atom, struct ref_array_item *ref)
 {
 	if (!ref->symref)
-		return "";
+		return xstrdup("");
 	else
 		return show_ref(&atom->u.refname, ref->symref);
 }
@@ -1510,7 +1511,7 @@ static int populate_value(struct ref_array_item *ref, struct strbuf *err)
 		ref->symref = resolve_refdup(ref->refname, RESOLVE_REF_READING,
 					     NULL, NULL);
 		if (!ref->symref)
-			ref->symref = "";
+			ref->symref = xstrdup("");
 	}
 
 	/* Fill in specials first */
@@ -1536,20 +1537,23 @@ static int populate_value(struct ref_array_item *ref, struct strbuf *err)
 			refname = get_symref(atom, ref);
 		else if (starts_with(name, "upstream")) {
 			const char *branch_name;
-			v->s = "";
 			/* only local branches may have an upstream */
 			if (!skip_prefix(ref->refname, "refs/heads/",
-					 &branch_name))
+					 &branch_name)) {
+				v->s = xstrdup("");
 				continue;
+			}
 			branch = branch_get(branch_name);
 
 			refname = branch_get_upstream(branch, NULL);
 			if (refname)
 				fill_remote_ref_details(atom, refname, branch, &v->s);
+			else
+				v->s = xstrdup("");
 			continue;
 		} else if (atom->u.remote_ref.push) {
 			const char *branch_name;
-			v->s = "";
+			v->s = xstrdup("");
 			if (!skip_prefix(ref->refname, "refs/heads/",
 					 &branch_name))
 				continue;
@@ -1562,10 +1566,12 @@ static int populate_value(struct ref_array_item *ref, struct strbuf *err)
 				if (!refname)
 					continue;
 			}
+			/* We will definitely re-init v->s on the next line. */
+			free((char *)v->s);
 			fill_remote_ref_details(atom, refname, branch, &v->s);
 			continue;
 		} else if (starts_with(name, "color:")) {
-			v->s = atom->u.color;
+			v->s = xstrdup(atom->u.color);
 			continue;
 		} else if (!strcmp(name, "flag")) {
 			char buf[256], *cp = buf;
@@ -1574,7 +1580,7 @@ static int populate_value(struct ref_array_item *ref, struct strbuf *err)
 			if (ref->flag & REF_ISPACKED)
 				cp = copy_advance(cp, ",packed");
 			if (cp == buf)
-				v->s = "";
+				v->s = xstrdup("");
 			else {
 				*cp = '\0';
 				v->s = xstrdup(buf + 1);
@@ -1584,40 +1590,42 @@ static int populate_value(struct ref_array_item *ref, struct strbuf *err)
 			continue;
 		} else if (!strcmp(name, "HEAD")) {
 			if (atom->u.head && !strcmp(ref->refname, atom->u.head))
-				v->s = "*";
+				v->s = xstrdup("*");
 			else
-				v->s = " ";
+				v->s = xstrdup(" ");
 			continue;
 		} else if (starts_with(name, "align")) {
 			v->handler = align_atom_handler;
-			v->s = "";
+			v->s = xstrdup("");
 			continue;
 		} else if (!strcmp(name, "end")) {
 			v->handler = end_atom_handler;
-			v->s = "";
+			v->s = xstrdup("");
 			continue;
 		} else if (starts_with(name, "if")) {
 			const char *s;
-			v->s = "";
 			if (skip_prefix(name, "if:", &s))
 				v->s = xstrdup(s);
+			else
+				v->s = xstrdup("");
 			v->handler = if_atom_handler;
 			continue;
 		} else if (!strcmp(name, "then")) {
 			v->handler = then_atom_handler;
-			v->s = "";
+			v->s = xstrdup("");
 			continue;
 		} else if (!strcmp(name, "else")) {
 			v->handler = else_atom_handler;
-			v->s = "";
+			v->s = xstrdup("");
 			continue;
 		} else
 			continue;
 
 		if (!deref)
-			v->s = refname;
+			v->s = xstrdup(refname);
 		else
 			v->s = xstrfmt("%s^{}", refname);
+		free((char *)refname);
 	}
 
 	for (i = 0; i < used_atom_cnt; i++) {
@@ -1988,6 +1996,10 @@ static int ref_filter_handler(const char *refname, const struct object_id *oid,
 static void free_array_item(struct ref_array_item *item)
 {
 	free((char *)item->symref);
+	if (item->value) {
+		free((char *)item->value->s);
+		free(item->value);
+	}
 	free(item);
 }
 

--
https://github.com/git/git/pull/538