{"thread":{"id":"49514","subject":"[PATCH 2/3] ls-remote: release memory instead of UNLEAK","startedAt":"2018-10-09T08:18:25Z","lastAt":"2018-10-18T07:30:55Z","messageCount":11,"participants":["Olga Telezhnaya","Junio C Hamano","Jeff King","Оля Тележная"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"359906","messageId":"0102016657e7d068-16e15ab5-335e-4a2a-8bbb-bf2242514ad2-000000@eu-west-1.amazonses.com","threadId":"49514","inReplyTo":"0102016657e7cfee-f1343b1e-9a85-4cae-990a-cc7177ea8487-000000@eu-west-1.amazonses.com","subject":"[PATCH 2/3] ls-remote: release memory instead of UNLEAK","fromName":"Olga Telezhnaya","fromEmail":"olyatelezhnaya@gmail.com","sentAt":"2018-10-09T08:18:21Z","receivedAt":"2018-10-09T08:18:25Z","isPatch":true,"sender":{"key":"olyatelezhnaya@gmail.com","avatar":"https://avatars.githubusercontent.com/u/11246099?v=4"},"body":"Use ref_array_clear() to release memory instead of UNLEAK macros.\n\nSigned-off-by: Olga Telezhnaia \n---\n builtin/ls-remote.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/ls-remote.c b/builtin/ls-remote.c\nindex 1a25df7ee15b4..6a0cdec30d2d7 100644\n--- a/builtin/ls-remote.c\n+++ b/builtin/ls-remote.c\n@@ -151,6 +151,6 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)\n \t}\n \n \tUNLEAK(sorting);\n-\tUNLEAK(ref_array);\n+\tref_array_clear(&ref_array);\n \treturn status;\n }\n\n--\nhttps://github.com/git/git/pull/538\n"},{"id":"359907","messageId":"0102016657e7cfee-f1343b1e-9a85-4cae-990a-cc7177ea8487-000000@eu-west-1.amazonses.com","threadId":"49514","inReplyTo":"CAL21BmkdUiNgr4NqpwTdi9f47i85s8oXCZMmVx5VyNKotL78uA@mail.gmail.com","subject":"[PATCH 1/3] ref-filter: free memory from used_atom","fromName":"Olga Telezhnaya","fromEmail":"olyatelezhnaya@gmail.com","sentAt":"2018-10-09T08:18:21Z","receivedAt":"2018-10-09T08:18:26Z","isPatch":true,"sender":{"key":"olyatelezhnaya@gmail.com","avatar":"https://avatars.githubusercontent.com/u/11246099?v=4"},"body":"Release memory from used_atom variable.\n\nSigned-off-by: Olga Telezhnaia \n---\n ref-filter.c | 3 +++\n 1 file changed, 3 insertions(+)\n\ndiff --git a/ref-filter.c b/ref-filter.c\nindex e1bcb4ca8a197..1b71d08a43a84 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -1996,6 +1996,9 @@ void ref_array_clear(struct ref_array *array)\n {\n \tint i;\n \n+\tfor (i = 0; i < used_atom_cnt; i++)\n+\t\tfree((char *)used_atom[i].name);\n+\tfree(used_atom);\n \tfor (i = 0; i < array->nr; i++)\n \t\tfree_array_item(array->items[i]);\n \tFREE_AND_NULL(array->items);\n\n--\nhttps://github.com/git/git/pull/538\n"},{"id":"359908","messageId":"0102016657e7d087-be1f65a3-be3e-4b15-95f1-7570d258c70d-000000@eu-west-1.amazonses.com","threadId":"49514","inReplyTo":"0102016657e7cfee-f1343b1e-9a85-4cae-990a-cc7177ea8487-000000@eu-west-1.amazonses.com","subject":"[PATCH 3/3] ref-filter: free item->value and item->value->s","fromName":"Olga Telezhnaya","fromEmail":"olyatelezhnaya@gmail.com","sentAt":"2018-10-09T08:18:21Z","receivedAt":"2018-10-09T08:18:27Z","isPatch":true,"sender":{"key":"olyatelezhnaya@gmail.com","avatar":"https://avatars.githubusercontent.com/u/11246099?v=4"},"body":"Release item->value.\nInitialize item->value->s dynamically and then release its resources.\nRelease some local variables.\n\nSigned-off-by: Olga Telezhnaia \n---\n ref-filter.c | 95 +++++++++++++++++++++++++++++-----------------------\n 1 file changed, 53 insertions(+), 42 deletions(-)\n\ndiff --git a/ref-filter.c b/ref-filter.c\nindex 1b71d08a43a84..4d42c777a9b50 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -875,7 +875,7 @@ static void grab_common_values(struct atom_value *val, int deref, struct expand_\n \t\tif (deref)\n \t\t\tname++;\n \t\tif (!strcmp(name, \"objecttype\"))\n-\t\t\tv->s = type_name(oi->type);\n+\t\t\tv->s = xstrdup(type_name(oi->type));\n \t\telse if (!strcmp(name, \"objectsize\")) {\n \t\t\tv->value = oi->size;\n \t\t\tv->s = xstrfmt(\"%lu\", oi->size);\n@@ -899,9 +899,9 @@ static void grab_tag_values(struct atom_value *val, int deref, struct object *ob\n \t\tif (deref)\n \t\t\tname++;\n \t\tif (!strcmp(name, \"tag\"))\n-\t\t\tv->s = tag->tag;\n+\t\t\tv->s = xstrdup(tag->tag);\n \t\telse if (!strcmp(name, \"type\") && tag->tagged)\n-\t\t\tv->s = type_name(tag->tagged->type);\n+\t\t\tv->s = xstrdup(type_name(tag->tagged->type));\n \t\telse if (!strcmp(name, \"object\") && tag->tagged)\n \t\t\tv->s = xstrdup(oid_to_hex(&tag->tagged->oid));\n \t}\n@@ -1032,7 +1032,7 @@ static void grab_date(const char *buf, struct atom_value *v, const char *atomnam\n \tv->value = timestamp;\n \treturn;\n bad:\n-\tv->s = \"\";\n+\tv->s = xstrdup(\"\");\n \tv->value = 0;\n }\n \n@@ -1227,7 +1227,7 @@ static void fill_missing_values(struct atom_value *val)\n \tfor (i = 0; i < used_atom_cnt; i++) {\n \t\tstruct atom_value *v = &val[i];\n \t\tif (v->s == NULL)\n-\t\t\tv->s = \"\";\n+\t\t\tv->s = xstrdup(\"\");\n \t}\n }\n \n@@ -1273,7 +1273,8 @@ static inline char *copy_advance(char *dst, const char *src)\n static const char *lstrip_ref_components(const char *refname, int len)\n {\n \tlong remaining = len;\n-\tconst char *start = refname;\n+\tconst char *start = xstrdup(refname);\n+\tconst char *to_free = start;\n \n \tif (len < 0) {\n \t\tint i;\n@@ -1294,20 +1295,24 @@ static const char *lstrip_ref_components(const char *refname, int len)\n \twhile (remaining > 0) {\n \t\tswitch (*start++) {\n \t\tcase '\\0':\n-\t\t\treturn \"\";\n+\t\t\tfree((char *)to_free);\n+\t\t\treturn xstrdup(\"\");\n \t\tcase '/':\n \t\t\tremaining--;\n \t\t\tbreak;\n \t\t}\n \t}\n \n+\tstart = xstrdup(start);\n+\tfree((char *)to_free);\n \treturn start;\n }\n \n static const char *rstrip_ref_components(const char *refname, int len)\n {\n \tlong remaining = len;\n-\tchar *start = xstrdup(refname);\n+\tconst char *start = xstrdup(refname);\n+\tconst char *to_free = start;\n \n \tif (len < 0) {\n \t\tint i;\n@@ -1327,9 +1332,10 @@ static const char *rstrip_ref_components(const char *refname, int len)\n \n \twhile (remaining-- > 0) {\n \t\tchar *p = strrchr(start, '/');\n-\t\tif (p == NULL)\n-\t\t\treturn \"\";\n-\t\telse\n+\t\tif (p == NULL) {\n+\t\t\tfree((char *)to_free);\n+\t\t\treturn xstrdup(\"\");\n+\t\t} else\n \t\t\tp[0] = '\\0';\n \t}\n \treturn start;\n@@ -1344,7 +1350,7 @@ static const char *show_ref(struct refname_atom *atom, const char *refname)\n \telse if (atom->option == R_RSTRIP)\n \t\treturn rstrip_ref_components(refname, atom->rstrip);\n \telse\n-\t\treturn refname;\n+\t\treturn xstrdup(refname);\n }\n \n static void fill_remote_ref_details(struct used_atom *atom, const char *refname,\n@@ -1358,7 +1364,7 @@ static void fill_remote_ref_details(struct used_atom *atom, const char *refname,\n \t\t\t\t NULL, AHEAD_BEHIND_FULL) < 0) {\n \t\t\t*s = xstrdup(msgs.gone);\n \t\t} else if (!num_ours && !num_theirs)\n-\t\t\t*s = \"\";\n+\t\t\t*s = xstrdup(\"\");\n \t\telse if (!num_ours)\n \t\t\t*s = xstrfmt(msgs.behind, num_theirs);\n \t\telse if (!num_theirs)\n@@ -1373,36 +1379,31 @@ static void fill_remote_ref_details(struct used_atom *atom, const char *refname,\n \t\t}\n \t} else if (atom->u.remote_ref.option == RR_TRACKSHORT) {\n \t\tif (stat_tracking_info(branch, &num_ours, &num_theirs,\n-\t\t\t\t NULL, AHEAD_BEHIND_FULL) < 0)\n+\t\t\t\t NULL, AHEAD_BEHIND_FULL) < 0) {\n+\t\t\t*s = xstrdup(\"\");\n \t\t\treturn;\n-\n+\t\t}\n \t\tif (!num_ours && !num_theirs)\n-\t\t\t*s = \"=\";\n+\t\t\t*s = xstrdup(\"=\");\n \t\telse if (!num_ours)\n-\t\t\t*s = \"<\";\n+\t\t\t*s = xstrdup(\"<\");\n \t\telse if (!num_theirs)\n-\t\t\t*s = \">\";\n+\t\t\t*s = xstrdup(\">\");\n \t\telse\n-\t\t\t*s = \"<>\";\n+\t\t\t*s = xstrdup(\"<>\");\n \t} else if (atom->u.remote_ref.option == RR_REMOTE_NAME) {\n \t\tint explicit;\n \t\tconst char *remote = atom->u.remote_ref.push ?\n \t\t\tpushremote_for_branch(branch, &explicit) :\n \t\t\tremote_for_branch(branch, &explicit);\n-\t\tif (explicit)\n-\t\t\t*s = xstrdup(remote);\n-\t\telse\n-\t\t\t*s = \"\";\n+\t\t*s = explicit ? xstrdup(remote) : xstrdup(\"\");\n \t} else if (atom->u.remote_ref.option == RR_REMOTE_REF) {\n \t\tint explicit;\n \t\tconst char *merge;\n \n \t\tmerge = remote_ref_for_branch(branch, atom->u.remote_ref.push,\n \t\t\t\t\t &explicit);\n-\t\tif (explicit)\n-\t\t\t*s = xstrdup(merge);\n-\t\telse\n-\t\t\t*s = \"\";\n+\t\t*s = explicit ? xstrdup(merge) : xstrdup(\"\");\n \t} else\n \t\tBUG(\"unhandled RR_* enum\");\n }\n@@ -1451,7 +1452,7 @@ char *get_head_description(void)\n static const char *get_symref(struct used_atom *atom, struct ref_array_item *ref)\n {\n \tif (!ref->symref)\n-\t\treturn \"\";\n+\t\treturn xstrdup(\"\");\n \telse\n \t\treturn show_ref(&atom->u.refname, ref->symref);\n }\n@@ -1510,7 +1511,7 @@ static int populate_value(struct ref_array_item *ref, struct strbuf *err)\n \t\tref->symref = resolve_refdup(ref->refname, RESOLVE_REF_READING,\n \t\t\t\t\t NULL, NULL);\n \t\tif (!ref->symref)\n-\t\t\tref->symref = \"\";\n+\t\t\tref->symref = xstrdup(\"\");\n \t}\n \n \t/* Fill in specials first */\n@@ -1536,20 +1537,23 @@ static int populate_value(struct ref_array_item *ref, struct strbuf *err)\n \t\t\trefname = get_symref(atom, ref);\n \t\telse if (starts_with(name, \"upstream\")) {\n \t\t\tconst char *branch_name;\n-\t\t\tv->s = \"\";\n \t\t\t/* only local branches may have an upstream */\n \t\t\tif (!skip_prefix(ref->refname, \"refs/heads/\",\n-\t\t\t\t\t &branch_name))\n+\t\t\t\t\t &branch_name)) {\n+\t\t\t\tv->s = xstrdup(\"\");\n \t\t\t\tcontinue;\n+\t\t\t}\n \t\t\tbranch = branch_get(branch_name);\n \n \t\t\trefname = branch_get_upstream(branch, NULL);\n \t\t\tif (refname)\n \t\t\t\tfill_remote_ref_details(atom, refname, branch, &v->s);\n+\t\t\telse\n+\t\t\t\tv->s = xstrdup(\"\");\n \t\t\tcontinue;\n \t\t} else if (atom->u.remote_ref.push) {\n \t\t\tconst char *branch_name;\n-\t\t\tv->s = \"\";\n+\t\t\tv->s = xstrdup(\"\");\n \t\t\tif (!skip_prefix(ref->refname, \"refs/heads/\",\n \t\t\t\t\t &branch_name))\n \t\t\t\tcontinue;\n@@ -1562,10 +1566,11 @@ static int populate_value(struct ref_array_item *ref, struct strbuf *err)\n \t\t\t\tif (!refname)\n \t\t\t\t\tcontinue;\n \t\t\t}\n+\t\t\tfree((char *)v->s); // we will definitely re-init it on the next line\n \t\t\tfill_remote_ref_details(atom, refname, branch, &v->s);\n \t\t\tcontinue;\n \t\t} else if (starts_with(name, \"color:\")) {\n-\t\t\tv->s = atom->u.color;\n+\t\t\tv->s = xstrdup(atom->u.color);\n \t\t\tcontinue;\n \t\t} else if (!strcmp(name, \"flag\")) {\n \t\t\tchar buf[256], *cp = buf;\n@@ -1574,7 +1579,7 @@ static int populate_value(struct ref_array_item *ref, struct strbuf *err)\n \t\t\tif (ref->flag & REF_ISPACKED)\n \t\t\t\tcp = copy_advance(cp, \",packed\");\n \t\t\tif (cp == buf)\n-\t\t\t\tv->s = \"\";\n+\t\t\t\tv->s = xstrdup(\"\");\n \t\t\telse {\n \t\t\t\t*cp = '\\0';\n \t\t\t\tv->s = xstrdup(buf + 1);\n@@ -1584,40 +1589,42 @@ static int populate_value(struct ref_array_item *ref, struct strbuf *err)\n \t\t\tcontinue;\n \t\t} else if (!strcmp(name, \"HEAD\")) {\n \t\t\tif (atom->u.head && !strcmp(ref->refname, atom->u.head))\n-\t\t\t\tv->s = \"*\";\n+\t\t\t\tv->s = xstrdup(\"*\");\n \t\t\telse\n-\t\t\t\tv->s = \" \";\n+\t\t\t\tv->s = xstrdup(\" \");\n \t\t\tcontinue;\n \t\t} else if (starts_with(name, \"align\")) {\n \t\t\tv->handler = align_atom_handler;\n-\t\t\tv->s = \"\";\n+\t\t\tv->s = xstrdup(\"\");\n \t\t\tcontinue;\n \t\t} else if (!strcmp(name, \"end\")) {\n \t\t\tv->handler = end_atom_handler;\n-\t\t\tv->s = \"\";\n+\t\t\tv->s = xstrdup(\"\");\n \t\t\tcontinue;\n \t\t} else if (starts_with(name, \"if\")) {\n \t\t\tconst char *s;\n-\t\t\tv->s = \"\";\n \t\t\tif (skip_prefix(name, \"if:\", &s))\n \t\t\t\tv->s = xstrdup(s);\n+\t\t\telse\n+\t\t\t\tv->s = xstrdup(\"\");\n \t\t\tv->handler = if_atom_handler;\n \t\t\tcontinue;\n \t\t} else if (!strcmp(name, \"then\")) {\n \t\t\tv->handler = then_atom_handler;\n-\t\t\tv->s = \"\";\n+\t\t\tv->s = xstrdup(\"\");\n \t\t\tcontinue;\n \t\t} else if (!strcmp(name, \"else\")) {\n \t\t\tv->handler = else_atom_handler;\n-\t\t\tv->s = \"\";\n+\t\t\tv->s = xstrdup(\"\");\n \t\t\tcontinue;\n \t\t} else\n \t\t\tcontinue;\n \n \t\tif (!deref)\n-\t\t\tv->s = refname;\n+\t\t\tv->s = xstrdup(refname);\n \t\telse\n \t\t\tv->s = xstrfmt(\"%s^{}\", refname);\n+\t\tfree((char *)refname);\n \t}\n \n \tfor (i = 0; i < used_atom_cnt; i++) {\n@@ -1988,6 +1995,10 @@ static int ref_filter_handler(const char *refname, const struct object_id *oid,\n static void free_array_item(struct ref_array_item *item)\n {\n \tfree((char *)item->symref);\n+\tif (item->value) {\n+\t\tfree((char *)item->value->s);\n+\t\tfree(item->value);\n+\t}\n \tfree(item);\n }\n \n\n--\nhttps://github.com/git/git/pull/538\n"},{"id":"360120","messageId":"xmqq5zy9jnv1.fsf@gitster-ct.c.googlers.com","threadId":"49514","inReplyTo":"0102016657e7cfee-f1343b1e-9a85-4cae-990a-cc7177ea8487-000000@eu-west-1.amazonses.com","subject":"Re: [PATCH 1/3] ref-filter: free memory from used_atom","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-10-11T01:03:46Z","receivedAt":"2018-10-11T01:04:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Olga Telezhnaya writes:\n\n> Release memory from used_atom variable.\n\nThat much readers would know from a quick look of the patch text.\n\nWithout knowing what you are aiming at, it is impossible to judge if\nthe patch is a good change.\n\nSeeing FREE_AND_NULL(array->items) in the same function makes me\nthink that the designer of ref_array_clear() function would want\nthis sequence of events to work correctly in an ideal future:\n\n * Do a ref-filter operation by calling filter_refs(), receiving the\n result into an array..\n\n * Do another ref-filter by calling filter_refs(), using different\n criteria, receiving the result into a different array.\n\n * Iterate over the resulting refs in the first array, and call\n format_ref_array_item().\n\n * ref_array_clear() the first array, as the caller is done with it.\n\n * Iterate over the resulting refs in the second array, and call\n format_ref_array_item().\n\n * ref_array_clear() the second array, as the caller is done with\n it.\n\nHowever, I think it would make it impossible for the second call to\nwork correctly if this code freed used_atom without clearing, and\nnot re-initializing the used_atom_cnt etc.\n\nIf on the other hand, the only thing you are interested in is to\njust discard pieces of memory we no longer use, and you are not\ninterested in helping to move us closer to the world in which we can\ncall filter_refs() twice, then the change this patch makes is\nsufficient.\n\nAnd the place to answer the \"what are you aiming at?\" question is in\nthe proposed commit log message.\n\nIn an ideal future, I _think_ the file-scope static variables in\nref-filter.c like used_atom and used_atom_cnt should become fields\nof a new structure (say \"struct ref_filter\"), with initializer and\nuninitializer ref_filter_new() and ref_filter_destroy(). When that\nhappens, I think FREE_AND_NULL(used_atom) + used_atom_cnt=0 should\nbecome part of ref_filter_destroy(), not part of ref_array_clear().\n\nBut we are not there yet, and a clean-up patch like this does not\nhave to be a step towards that goal.\n\n> Signed-off-by: Olga Telezhnaia \n> ---\n> ref-filter.c | 3 +++\n> 1 file changed, 3 insertions(+)\n>\n> diff --git a/ref-filter.c b/ref-filter.c\n> index e1bcb4ca8a197..1b71d08a43a84 100644\n> --- a/ref-filter.c\n> +++ b/ref-filter.c\n> @@ -1996,6 +1996,9 @@ void ref_array_clear(struct ref_array *array)\n> {\n> \tint i;\n> \n> +\tfor (i = 0; i < used_atom_cnt; i++)\n> +\t\tfree((char *)used_atom[i].name);\n> +\tfree(used_atom);\n> \tfor (i = 0; i < array->nr; i++)\n> \t\tfree_array_item(array->items[i]);\n> \tFREE_AND_NULL(array->items);\n>\n> --\n> https://github.com/git/git/pull/538\n"},{"id":"360121","messageId":"xmqqzhvli8kl.fsf@gitster-ct.c.googlers.com","threadId":"49514","inReplyTo":"0102016657e7d087-be1f65a3-be3e-4b15-95f1-7570d258c70d-000000@eu-west-1.amazonses.com","subject":"Re: [PATCH 3/3] ref-filter: free item->value and item->value->s","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-10-11T01:19:22Z","receivedAt":"2018-10-11T01:19:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Olga Telezhnaya writes:\n\n> Release item->value.\n> Initialize item->value->s dynamically and then release its resources.\n> Release some local variables.\n\nAgain, \"why\" is lacking.\n\n> @@ -1373,36 +1379,31 @@ static void fill_remote_ref_details(struct used_atom *atom, const char *refname,\n> \t\t}\n> \t} else if (atom->u.remote_ref.option == RR_TRACKSHORT) {\n> \t\tif (stat_tracking_info(branch, &num_ours, &num_theirs,\n> -\t\t\t\t NULL, AHEAD_BEHIND_FULL) < 0)\n> +\t\t\t\t NULL, AHEAD_BEHIND_FULL) < 0) {\n> +\t\t\t*s = xstrdup(\"\");\n> \t\t\treturn;\n\nIt is a bit sad that we need to sprinkle xstrdup() all over the\nplace, but I do not offhand think of a better alternative to ensure\nthat it is safe to blindly free the .s field.\n\n> -\t\tif (explicit)\n> -\t\t\t*s = xstrdup(remote);\n> -\t\telse\n> -\t\t\t*s = \"\";\n> +\t\t*s = explicit ? xstrdup(remote) : xstrdup(\"\");\n\nNext time, please avoid mixing this kind of unrelated changes with\nthe main theme (i.e. \"the original had allocated and static pieces\nof memory pointed by the same variable, which made it impossible to\nblindly free it, so make sure everything is allocated\"). It makes\nit harder to let reviewers' eyes coast over the patch.\n\nI say \"Next time\" because the change is already written this time,\nand I already spent time to see it was an OK change. By the way,\n\n\t*s = xstrdup(explicit ? remote : \"\");\n\nis probably shorter.\n\n> @@ -1562,10 +1566,11 @@ static int populate_value(struct ref_array_item *ref, struct strbuf *err)\n> \t\t\t\tif (!refname)\n> \t\t\t\t\tcontinue;\n> \t\t\t}\n> +\t\t\tfree((char *)v->s); // we will definitely re-init it on the next line\n\nNo // comment, please.\n\n> static void free_array_item(struct ref_array_item *item)\n> {\n> \tfree((char *)item->symref);\n> +\tif (item->value) {\n> +\t\tfree((char *)item->value->s);\n> +\t\tfree(item->value);\n> +\t}\n> \tfree(item);\n> }\n\nOK.\n"},{"id":"360122","messageId":"xmqqva69i8j7.fsf@gitster-ct.c.googlers.com","threadId":"49514","inReplyTo":"0102016657e7d068-16e15ab5-335e-4a2a-8bbb-bf2242514ad2-000000@eu-west-1.amazonses.com","subject":"Re: [PATCH 2/3] ls-remote: release memory instead of UNLEAK","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-10-11T01:20:12Z","receivedAt":"2018-10-11T01:20:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Olga Telezhnaya writes:\n\n> Use ref_array_clear() to release memory instead of UNLEAK macros.\n>\n> Signed-off-by: Olga Telezhnaia \n> ---\n> builtin/ls-remote.c | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n\nOK, this is immediately before the command exits, and we have a way\nto clear and release the resource, so it is obvious we should use\nit.\n\nGood.\n\n\n>\n> diff --git a/builtin/ls-remote.c b/builtin/ls-remote.c\n> index 1a25df7ee15b4..6a0cdec30d2d7 100644\n> --- a/builtin/ls-remote.c\n> +++ b/builtin/ls-remote.c\n> @@ -151,6 +151,6 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)\n> \t}\n> \n> \tUNLEAK(sorting);\n> -\tUNLEAK(ref_array);\n> +\tref_array_clear(&ref_array);\n> \treturn status;\n> }\n>\n> --\n> https://github.com/git/git/pull/538\n"},{"id":"360264","messageId":"xmqqa7njevu2.fsf@gitster-ct.c.googlers.com","threadId":"49514","inReplyTo":"xmqq5zy9jnv1.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 1/3] ref-filter: free memory from used_atom","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-10-12T02:35:01Z","receivedAt":"2018-10-12T02:35:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Olga Telezhnaya writes:\n\nThese three patches seem to cause t6300 to fail with an attempt to\nfree an invalid pointer in \"git for-each-ref --format='%(push)'\"\n(6300.25)\n\n\n*** Error in `/home/gitster/w/git.git/git': free(): invalid pointer: 0x000055cca3a9f920 ***\n======= Backtrace: =========\n/lib/x86_64-linux-gnu/libc.so.6(+0x70bcb)[0x7f052fdacbcb]\n/lib/x86_64-linux-gnu/libc.so.6(+0x76f96)[0x7f052fdb2f96]\n/home/gitster/w/git.git/git(+0x15a866)[0x55cca35ca866]\n/home/gitster/w/git.git/git(+0x15ab48)[0x55cca35cab48]\n/home/gitster/w/git.git/git(+0x15b6d3)[0x55cca35cb6d3]\n/home/gitster/w/git.git/git(+0x15b7dd)[0x55cca35cb7dd]\n/home/gitster/w/git.git/git(+0x49e18)[0x55cca34b9e18]\n/home/gitster/w/git.git/git(+0x19b20)[0x55cca3489b20]\n/home/gitster/w/git.git/git(+0x1aab5)[0x55cca348aab5]\n/home/gitster/w/git.git/git(+0x19809)[0x55cca3489809]\n/lib/x86_64-linux-gnu/libc.so.6(__libc_start_main+0xf1)[0x7f052fd5c2b1]\n/home/gitster/w/git.git/git(+0x1984a)[0x55cca348984a]\n======= Memory map: ========\n55cca3470000-55cca36cc000 r-xp 00000000 fe:00 2760695 /home/gitster/w/git.git/git\n55cca38cc000-55cca38cf000 r--p 0025c000 fe:00 2760695 /home/gitster/w/git.git/git\n55cca38cf000-55cca38de000 rw-p 0025f000 fe:00 2760695 /home/gitster/w/git.git/git\n55cca38de000-55cca3921000 rw-p 00000000 00:00 0 \n55cca3a9e000-55cca3abf000 rw-p 00000000 00:00 0 [heap]\n7f052fb24000-7f052fb3b000 r-xp 00000000 fe:00 393287 /lib/x86_64-linux-gnu/libgcc_s.so.1\n7f052fb3b000-7f052fd3a000 ---p 00017000 fe:00 393287 /lib/x86_64-linux-gnu/libgcc_s.so.1\n7f052fd3a000-7f052fd3b000 r--p 00016000 fe:00 393287 /lib/x86_64-linux-gnu/libgcc_s.so.1\n7f052fd3b000-7f052fd3c000 rw-p 00017000 fe:00 393287 /lib/x86_64-linux-gnu/libgcc_s.so.1\n7f052fd3c000-7f052fed1000 r-xp 00000000 fe:00 392469 /lib/x86_64-linux-gnu/libc-2.24.so\n7f052fed1000-7f05300d1000 ---p 00195000 fe:00 392469 /lib/x86_64-linux-gnu/libc-2.24.so\n7f05300d1000-7f05300d5000 r--p 00195000 fe:00 392469 /lib/x86_64-linux-gnu/libc-2.24.so\n7f05300d5000-7f05300d7000 rw-p 00199000 fe:00 392469 /lib/x86_64-linux-gnu/libc-2.24.so\n7f05300d7000-7f05300db000 rw-p 00000000 00:00 0 \n7f05300db000-7f05300e2000 r-xp 00000000 fe:00 392487 /lib/x86_64-linux-gnu/librt-2.24.so\n7f05300e2000-7f05302e1000 ---p 00007000 fe:00 392487 /lib/x86_64-linux-gnu/librt-2.24.so\n7f05302e1000-7f05302e2000 r--p 00006000 fe:00 392487 /lib/x86_64-linux-gnu/librt-2.24.so\n7f05302e2000-7f05302e3000 rw-p 00007000 fe:00 392487 /lib/x86_64-linux-gnu/librt-2.24.so\n7f05302e3000-7f05302fb000 r-xp 00000000 fe:00 392485 /lib/x86_64-linux-gnu/libpthread-2.24.so\n7f05302fb000-7f05304fa000 ---p 00018000 fe:00 392485 /lib/x86_64-linux-gnu/libpthread-2.24.so\n7f05304fa000-7f05304fb000 r--p 00017000 fe:00 392485 /lib/x86_64-linux-gnu/libpthread-2.24.so\n7f05304fb000-7f05304fc000 rw-p 00018000 fe:00 392485 /lib/x86_64-linux-gnu/libpthread-2.24.so\n7f05304fc000-7f0530500000 rw-p 00000000 00:00 0 \n7f0530500000-7f0530519000 r-xp 00000000 fe:00 392698 /lib/x86_64-linux-gnu/libz.so.1.2.8\n7f0530519000-7f0530718000 ---p 00019000 fe:00 392698 /lib/x86_64-linux-gnu/libz.so.1.2.8\n7f0530718000-7f0530719000 r--p 00018000 fe:00 392698 /lib/x86_64-linux-gnu/libz.so.1.2.8\n7f0530719000-7f053071a000 rw-p 00019000 fe:00 392698 /lib/x86_64-linux-gnu/libz.so.1.2.8\n7f053071a000-7f053073d000 r-xp 00000000 fe:00 392461 /lib/x86_64-linux-gnu/ld-2.24.so\n7f0530916000-7f0530918000 rw-p 00000000 00:00 0 \n7f0530939000-7f053093d000 rw-p 00000000 00:00 0 \n7f053093d000-7f053093e000 r--p 00023000 fe:00 392461 /lib/x86_64-linux-gnu/ld-2.24.so\n7f053093e000-7f053093f000 rw-p 00024000 fe:00 392461 /lib/x86_64-linux-gnu/ld-2.24.so\n7f053093f000-7f0530940000 rw-p 00000000 00:00 0 \n7ffe894ee000-7ffe89510000 rw-p 00000000 00:00 0 [stack]\n7ffe8959e000-7ffe895a1000 r--p 00000000 00:00 0 [vvar]\n7ffe895a1000-7ffe895a3000 r-xp 00000000 00:00 0 [vdso]\n./test-lib.sh: line 631: 262132 Aborted git for-each-ref --format='%(push)' refs/heads/master > actual\nnot ok 25 - basic atom: head push\n#\t\n#\t\t\tgit for-each-ref --format='%(push)' refs/heads/master >actual &&\n#\t\t\tsanitize_pgp actual.clean &&\n#\t\t\ttest_cmp expected actual.clean\n#\t\t\n\n"},{"id":"360336","messageId":"20181012190920.GD4917@sigill.intra.peff.net","threadId":"49514","inReplyTo":"xmqqzhvli8kl.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 3/3] ref-filter: free item->value and item->value->s","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-10-12T19:09:20Z","receivedAt":"2018-10-12T19:09:24Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Oct 11, 2018 at 10:19:22AM +0900, Junio C Hamano wrote:\n\n> > @@ -1373,36 +1379,31 @@ static void fill_remote_ref_details(struct used_atom *atom, const char *refname,\n> > \t\t}\n> > \t} else if (atom->u.remote_ref.option == RR_TRACKSHORT) {\n> > \t\tif (stat_tracking_info(branch, &num_ours, &num_theirs,\n> > -\t\t\t\t NULL, AHEAD_BEHIND_FULL) < 0)\n> > +\t\t\t\t NULL, AHEAD_BEHIND_FULL) < 0) {\n> > +\t\t\t*s = xstrdup(\"\");\n> > \t\t\treturn;\n> \n> It is a bit sad that we need to sprinkle xstrdup() all over the\n> place, but I do not offhand think of a better alternative to ensure\n> that it is safe to blindly free the .s field.\n\nI think the root of the problem is that the current code needs\n_something_ in the \"s\" field to know that the value has already been\nfilled in.\n\nIf there were a separate flag for \"filled\", then this could just be\nNULL (and the later code that builds the output would have to realize\nto replace that with an empty string).\n\nI think in the long run, we'd ideally have one of two code structures:\n\n - a single pass, without iterating over the used atoms list\n repeatedly. E.g., the way oid_object_info() takes a struct\n representing the set of items that the caller is interested in, and\n then fills it in as it digs for information.\n\n - individual atoms could write directly to an output strbuf, avoiding\n the extra allocation and copy altogether (that would help these\n cases that are just copying an empty string, but also the ones that\n really are allocating a piece of data and end up copying it.\n\nI'm OK with this approach in the meantime, though. There's a fair bit of\nrefactoring to get to either of those end-states, and this clears up the\nconfusing memory ownership issues. And I don't think for-each-ref is\n_too_ sensitive to a few extra mallocs (as compared to say, cat-file's\nformatter, which is often run once per object).\n\nI didn't dig into the valgrind errors you saw, but I'd guess they are\nthe result of this patch missing one of these cases (if not a string\nliteral like \"\", perhaps a const pointer into another heap string).\n\n-Peff\n"},{"id":"360817","messageId":"20181018063306.GA23497@sigill.intra.peff.net","threadId":"49514","inReplyTo":"xmqqa7njevu2.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 1/3] ref-filter: free memory from used_atom","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-10-18T06:33:06Z","receivedAt":"2018-10-18T06:33:10Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Oct 12, 2018 at 11:35:01AM +0900, Junio C Hamano wrote:\n\n> Junio C Hamano writes:\n> \n> > Olga Telezhnaya writes:\n> \n> These three patches seem to cause t6300 to fail with an attempt to\n> free an invalid pointer in \"git for-each-ref --format='%(push)'\"\n> (6300.25)\n\nI dug into this a bit. I think it's actually a misapplication of the\npatches on your side. Applying them locally works, but your\not/ref-filter-plug-leaks branch does not.\n\nThe patch below on top of your branch helps. :)\n\ndiff --git a/ref-filter.c b/ref-filter.c\nindex f4ff80eca0..4255de1d75 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -1567,7 +1567,6 @@ static int populate_value(struct ref_array_item *ref, struct strbuf *err)\n \t\t\t\t\tcontinue;\n \t\t\t}\n \t\t\tfree((char *)v->s); /* we will definitely re-init it on the next line */\n-\t\t\tfree((char *)v->s);\n \t\t\tfill_remote_ref_details(atom, refname, branch, &v->s);\n \t\t\tcontinue;\n \t\t} else if (starts_with(name, \"color:\")) {\n\nPresumably it came from the manual comment-style fixup.\n\nWith that fix, the tests run fine for me under ASan/UBSan (with the\nexception of t5310, but that's fixed already in a parallel topic).\n\n-Peff\n"},{"id":"360820","messageId":"xmqqr2gnu4sb.fsf@gitster-ct.c.googlers.com","threadId":"49514","inReplyTo":"20181018063306.GA23497@sigill.intra.peff.net","subject":"Re: [PATCH 1/3] ref-filter: free memory from used_atom","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-10-18T06:50:44Z","receivedAt":"2018-10-18T06:50:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Presumably it came from the manual comment-style fixup.\n\nWow, that was embarrassing. Thanks for catching it.\n\n>\n> With that fix, the tests run fine for me under ASan/UBSan (with the\n> exception of t5310, but that's fixed already in a parallel topic).\n>\n> -Peff\n"},{"id":"360829","messageId":"CAL21BmkK+xtFPn7zdBY7VF2Fb_=92MEwAHh4HSrNa4VKjf-b3w@mail.gmail.com","threadId":"49514","inReplyTo":"xmqqr2gnu4sb.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 1/3] ref-filter: free memory from used_atom","fromName":"Оля Тележная","fromEmail":"olyatelezhnaya@gmail.com","sentAt":"2018-10-18T07:26:58Z","receivedAt":"2018-10-18T07:30:55Z","isPatch":true,"sender":{"key":"olyatelezhnaya@gmail.com","avatar":"https://avatars.githubusercontent.com/u/11246099?v=4"},"body":"чт, 18 окт. 2018 г. в 9:51, Junio C Hamano :\n>\n> Jeff King writes:\n>\n> > Presumably it came from the manual comment-style fixup.\n>\n> Wow, that was embarrassing. Thanks for catching it.\n\nJeff, thanks a lot!\nI just sent new version where I fixed all known issues including that comment.\n>\n> >\n> > With that fix, the tests run fine for me under ASan/UBSan (with the\n> > exception of t5310, but that's fixed already in a parallel topic).\n> >\n> > -Peff\n"}]}