* [PATCH 0/3] ref-filter: reduce memory leaks @ 2018-10-09 8:12 Оля Тележная 2018-10-09 8:18 ` [PATCH 1/3] ref-filter: free memory from used_atom Olga Telezhnaya 0 siblings, 1 reply; 15+ messages in thread From: Оля Тележная @ 2018-10-09 8:12 UTC (permalink / raw) To: git, Jeff King, Christian Couder 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 1/3] ref-filter: free memory from used_atom 2018-10-09 8:12 [PATCH 0/3] ref-filter: reduce memory leaks Оля Тележная @ 2018-10-09 8:18 ` Olga Telezhnaya 2018-10-09 8:18 ` [PATCH 3/3] ref-filter: free item->value and item->value->s Olga Telezhnaya ` (3 more replies) 0 siblings, 4 replies; 15+ messages in thread From: Olga Telezhnaya @ 2018-10-09 8:18 UTC (permalink / raw) To: git 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 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH 3/3] ref-filter: free item->value and item->value->s 2018-10-09 8:18 ` [PATCH 1/3] ref-filter: free memory from used_atom Olga Telezhnaya @ 2018-10-09 8:18 ` Olga Telezhnaya 2018-10-11 1:19 ` Junio C Hamano 2018-10-09 8:18 ` [PATCH 2/3] ls-remote: release memory instead of UNLEAK Olga Telezhnaya ` (2 subsequent siblings) 3 siblings, 1 reply; 15+ messages in thread From: Olga Telezhnaya @ 2018-10-09 8:18 UTC (permalink / raw) To: git 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 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH 3/3] ref-filter: free item->value and item->value->s 2018-10-09 8:18 ` [PATCH 3/3] ref-filter: free item->value and item->value->s Olga Telezhnaya @ 2018-10-11 1:19 ` Junio C Hamano 2018-10-12 19:09 ` Jeff King 0 siblings, 1 reply; 15+ messages in thread From: Junio C Hamano @ 2018-10-11 1:19 UTC (permalink / raw) To: Olga Telezhnaya; +Cc: git 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. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 3/3] ref-filter: free item->value and item->value->s 2018-10-11 1:19 ` Junio C Hamano @ 2018-10-12 19:09 ` Jeff King 0 siblings, 0 replies; 15+ messages in thread From: Jeff King @ 2018-10-12 19:09 UTC (permalink / raw) To: Junio C Hamano; +Cc: Olga Telezhnaya, git 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 2/3] ls-remote: release memory instead of UNLEAK 2018-10-09 8:18 ` [PATCH 1/3] ref-filter: free memory from used_atom Olga Telezhnaya 2018-10-09 8:18 ` [PATCH 3/3] ref-filter: free item->value and item->value->s Olga Telezhnaya @ 2018-10-09 8:18 ` Olga Telezhnaya 2018-10-11 1:20 ` Junio C Hamano 2018-10-11 1:03 ` [PATCH 1/3] ref-filter: free memory from used_atom Junio C Hamano 2018-10-18 7:28 ` [PATCH v2 " Olga Telezhnaya 3 siblings, 1 reply; 15+ messages in thread From: Olga Telezhnaya @ 2018-10-09 8:18 UTC (permalink / raw) To: git 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 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH 2/3] ls-remote: release memory instead of UNLEAK 2018-10-09 8:18 ` [PATCH 2/3] ls-remote: release memory instead of UNLEAK Olga Telezhnaya @ 2018-10-11 1:20 ` Junio C Hamano 0 siblings, 0 replies; 15+ messages in thread From: Junio C Hamano @ 2018-10-11 1:20 UTC (permalink / raw) To: Olga Telezhnaya; +Cc: git 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/3] ref-filter: free memory from used_atom 2018-10-09 8:18 ` [PATCH 1/3] ref-filter: free memory from used_atom Olga Telezhnaya 2018-10-09 8:18 ` [PATCH 3/3] ref-filter: free item->value and item->value->s Olga Telezhnaya 2018-10-09 8:18 ` [PATCH 2/3] ls-remote: release memory instead of UNLEAK Olga Telezhnaya @ 2018-10-11 1:03 ` Junio C Hamano 2018-10-12 2:35 ` Junio C Hamano 2018-10-18 7:28 ` [PATCH v2 " Olga Telezhnaya 3 siblings, 1 reply; 15+ messages in thread From: Junio C Hamano @ 2018-10-11 1:03 UTC (permalink / raw) To: Olga Telezhnaya; +Cc: git 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/3] ref-filter: free memory from used_atom 2018-10-11 1:03 ` [PATCH 1/3] ref-filter: free memory from used_atom Junio C Hamano @ 2018-10-12 2:35 ` Junio C Hamano 2018-10-18 6:33 ` Jeff King 0 siblings, 1 reply; 15+ messages in thread From: Junio C Hamano @ 2018-10-12 2:35 UTC (permalink / raw) To: Olga Telezhnaya; +Cc: git 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 # ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/3] ref-filter: free memory from used_atom 2018-10-12 2:35 ` Junio C Hamano @ 2018-10-18 6:33 ` Jeff King 2018-10-18 6:50 ` Junio C Hamano 0 siblings, 1 reply; 15+ messages in thread From: Jeff King @ 2018-10-18 6:33 UTC (permalink / raw) To: Junio C Hamano; +Cc: Olga Telezhnaya, git 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 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH 1/3] ref-filter: free memory from used_atom 2018-10-18 6:33 ` Jeff King @ 2018-10-18 6:50 ` Junio C Hamano 2018-10-18 7:26 ` Оля Тележная 0 siblings, 1 reply; 15+ messages in thread From: Junio C Hamano @ 2018-10-18 6:50 UTC (permalink / raw) To: Jeff King; +Cc: Olga Telezhnaya, git 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/3] ref-filter: free memory from used_atom 2018-10-18 6:50 ` Junio C Hamano @ 2018-10-18 7:26 ` Оля Тележная 0 siblings, 0 replies; 15+ messages in thread From: Оля Тележная @ 2018-10-18 7:26 UTC (permalink / raw) To: Junio C Hamano; +Cc: Jeff King, git чт, 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v2 1/3] ref-filter: free memory from used_atom 2018-10-09 8:18 ` [PATCH 1/3] ref-filter: free memory from used_atom Olga Telezhnaya ` (2 preceding siblings ...) 2018-10-11 1:03 ` [PATCH 1/3] ref-filter: free memory from used_atom Junio C Hamano @ 2018-10-18 7:28 ` Olga Telezhnaya 2018-10-18 7:28 ` [PATCH v2 2/3] ls-remote: release memory instead of UNLEAK Olga Telezhnaya 2018-10-18 7:28 ` [PATCH v2 3/3] ref-filter: free item->value and item->value->s Olga Telezhnaya 3 siblings, 2 replies; 15+ messages in thread From: Olga Telezhnaya @ 2018-10-18 7:28 UTC (permalink / raw) To: git 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 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH v2 2/3] ls-remote: release memory instead of UNLEAK 2018-10-18 7:28 ` [PATCH v2 " Olga Telezhnaya @ 2018-10-18 7:28 ` Olga Telezhnaya 2018-10-18 7:28 ` [PATCH v2 3/3] ref-filter: free item->value and item->value->s Olga Telezhnaya 1 sibling, 0 replies; 15+ messages in thread From: Olga Telezhnaya @ 2018-10-18 7:28 UTC (permalink / raw) To: git 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 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH v2 3/3] ref-filter: free item->value and item->value->s 2018-10-18 7:28 ` [PATCH v2 " Olga Telezhnaya 2018-10-18 7:28 ` [PATCH v2 2/3] ls-remote: release memory instead of UNLEAK Olga Telezhnaya @ 2018-10-18 7:28 ` Olga Telezhnaya 1 sibling, 0 replies; 15+ messages in thread From: Olga Telezhnaya @ 2018-10-18 7:28 UTC (permalink / raw) To: git 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 ^ permalink raw reply related [flat|nested] 15+ messages in thread
end of thread, other threads:[~2018-10-18 7:30 UTC | newest] Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-10-09 8:12 [PATCH 0/3] ref-filter: reduce memory leaks Оля Тележная 2018-10-09 8:18 ` [PATCH 1/3] ref-filter: free memory from used_atom Olga Telezhnaya 2018-10-09 8:18 ` [PATCH 3/3] ref-filter: free item->value and item->value->s Olga Telezhnaya 2018-10-11 1:19 ` Junio C Hamano 2018-10-12 19:09 ` Jeff King 2018-10-09 8:18 ` [PATCH 2/3] ls-remote: release memory instead of UNLEAK Olga Telezhnaya 2018-10-11 1:20 ` Junio C Hamano 2018-10-11 1:03 ` [PATCH 1/3] ref-filter: free memory from used_atom Junio C Hamano 2018-10-12 2:35 ` Junio C Hamano 2018-10-18 6:33 ` Jeff King 2018-10-18 6:50 ` Junio C Hamano 2018-10-18 7:26 ` Оля Тележная 2018-10-18 7:28 ` [PATCH v2 " Olga Telezhnaya 2018-10-18 7:28 ` [PATCH v2 2/3] ls-remote: release memory instead of UNLEAK Olga Telezhnaya 2018-10-18 7:28 ` [PATCH v2 3/3] ref-filter: free item->value and item->value->s Olga Telezhnaya
Code repositories for project(s) associated with this public inbox https://80x24.org/mirrors/git.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).