threads / patch / 49514

patch, 3 partsls-remote: release memory instead of UNLEAK

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

## tl;dr

11 messages between Oct 9, 2018 and Oct 18, 2018. Diffs are folded; open one to read it.

replies: 10people: 3as markdown or json

Olga Telezhnaya· Oct 9, 2018, 08:18 UTC · lore

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

Release memory from used_atom variable.
Signed-off-by: Olga Telezhnaia <olyatelezhnaya@gmail.com>
---
 ref-filter.c | 3 +++
 1 file changed, 3 insertions(+)
Show changes to ref-filter.c +3 −1
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
Olga Telezhnaya· Oct 9, 2018, 08:18 UTC · re: Olga Telezhnaya · lore
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(-)
Show changes to builtin/ls-remote.c +1 −2
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
Junio C Hamano· Oct 11, 2018, 01:20 UTC · re: Olga Telezhnaya · lore

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

Olga Telezhnaya <olyatelezhnaya@gmail.com> writes:
Show 6 quoted lines
> 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.
Show 16 quoted lines
>
> 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
Olga Telezhnaya· Oct 9, 2018, 08:18 UTC · re: Olga Telezhnaya · lore

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

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(-)
Show changes to ref-filter.c +53 −43
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
Junio C Hamano· Oct 11, 2018, 01:19 UTC · re: Olga Telezhnaya · lore

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

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.
Show 8 quoted lines
> @@ -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.

Show 5 quoted lines
> -		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.
Show 5 quoted lines
> @@ -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.
Show 9 quoted lines
>  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.
Jeff King· Oct 12, 2018, 19:09 UTC · re: Junio C Hamano · lore

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

On Thu, Oct 11, 2018 at 10:19:22AM +0900, Junio C Hamano wrote:
Show 12 quoted lines
> > @@ -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
Junio C Hamano· Oct 11, 2018, 01:03 UTC · re: Olga Telezhnaya · lore

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

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.

Show 22 quoted lines
> 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
Junio C Hamano· Oct 12, 2018, 02:35 UTC · re: Junio C Hamano · lore

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

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 #

Jeff King· Oct 18, 2018, 06:33 UTC · re: Junio C Hamano · lore

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

On Fri, Oct 12, 2018 at 11:35:01AM +0900, Junio C Hamano wrote:
Show 7 quoted lines
> 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. :)
Show changes to ref-filter.c +0 −2
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
Junio C Hamano· Oct 18, 2018, 06:50 UTC · re: Jeff King · lore

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

Jeff King <peff@peff.net> writes:
> Presumably it came from the manual comment-style fixup.
Wow, that was embarrassing.  Thanks for catching it.
Show 5 quoted lines
>
> 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
Оля Тележная· Oct 18, 2018, 07:26 UTC · re: Junio C Hamano · lore

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

чт, 18 окт. 2018 г. в 9:51, Junio C Hamano <gitster@pobox.com>:
Show 6 quoted lines
>
> 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.

Show 6 quoted lines
>
> >
> > 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

← back to recent threads