{"thread":{"id":"21801","subject":"[PATCH] get_ref_states: strdup entries and free util in stale list","startedAt":"2009-11-30T23:57:27Z","lastAt":"2009-12-01T19:20:12Z","messageCount":12,"participants":["Bert Wesarg","Junio C Hamano","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"128816","messageId":"0458f16c6ce906997aaf357c0c7368841ae83c36.1259625072.git.bert.wesarg@googlemail.com","threadId":"21801","inReplyTo":null,"subject":"[PATCH] get_ref_states: strdup entries and free util in stale list","fromName":"Bert Wesarg","fromEmail":"bert.wesarg@googlemail.com","sentAt":"2009-11-30T23:57:27Z","receivedAt":"2009-11-30T23:57:27Z","isPatch":true,"sender":{"key":"bert.wesarg@googlemail.com","avatar":"https://avatars.githubusercontent.com/u/111934?v=4"},"body":"Signed-off-by: Bert Wesarg \n---\n builtin-remote.c | 6 ++++--\n 1 files changed, 4 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin-remote.c b/builtin-remote.c\nindex 7916626..bb72e27 100644\n--- a/builtin-remote.c\n+++ b/builtin-remote.c\n@@ -272,7 +272,9 @@ static int get_ref_states(const struct ref *remote_refs, struct ref_states *stat\n \t\t\tdie(\"Could not get fetch map for refspec %s\",\n \t\t\t\tstates->remote->fetch_refspec[i]);\n \n-\tstates->new.strdup_strings = states->tracked.strdup_strings = 1;\n+\tstates->new.strdup_strings =\n+\tstates->tracked.strdup_strings =\n+\tstates->stale.strdup_strings = 1;\n \tfor (ref = fetch_map; ref; ref = ref->next) {\n \t\tunsigned char sha1[20];\n \t\tif (!ref->peer_ref || read_ref(ref->peer_ref->name, sha1))\n@@ -768,7 +770,7 @@ static void clear_push_info(void *util, const char *string)\n static void free_remote_ref_states(struct ref_states *states)\n {\n \tstring_list_clear(&states->new, 0);\n-\tstring_list_clear(&states->stale, 0);\n+\tstring_list_clear(&states->stale, 1);\n \tstring_list_clear(&states->tracked, 0);\n \tstring_list_clear(&states->heads, 0);\n \tstring_list_clear_func(&states->push, clear_push_info);\n-- \n1.6.6.rc0.253.g1ec3\n"},{"id":"128823","messageId":"7viqcrbl22.fsf@alter.siamese.dyndns.org","threadId":"21801","inReplyTo":"0458f16c6ce906997aaf357c0c7368841ae83c36.1259625072.git.bert.wesarg@googlemail.com","subject":"Re: [PATCH] get_ref_states: strdup entries and free util in stale list","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-12-01T00:21:57Z","receivedAt":"2009-12-01T00:21:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Bert Wesarg writes:\n\n> Signed-off-by: Bert Wesarg \n\nHmm, the Subject: matches what the code does, but nobody mentions why it\nis necessary (iow, what breaks in the current code and what becomes better\nif the patch is applied). The blank space before your \"S-o-b:\" line is\nfor you to describe these things.\n\n> ---\n> builtin-remote.c | 6 ++++--\n> 1 files changed, 4 insertions(+), 2 deletions(-)\n>\n> diff --git a/builtin-remote.c b/builtin-remote.c\n> index 7916626..bb72e27 100644\n> --- a/builtin-remote.c\n> +++ b/builtin-remote.c\n> @@ -272,7 +272,9 @@ static int get_ref_states(const struct ref *remote_refs, struct ref_states *stat\n> \t\t\tdie(\"Could not get fetch map for refspec %s\",\n> \t\t\t\tstates->remote->fetch_refspec[i]);\n> \n> -\tstates->new.strdup_strings = states->tracked.strdup_strings = 1;\n> +\tstates->new.strdup_strings =\n> +\tstates->tracked.strdup_strings =\n> +\tstates->stale.strdup_strings = 1;\n> \tfor (ref = fetch_map; ref; ref = ref->next) {\n> \t\tunsigned char sha1[20];\n> \t\tif (!ref->peer_ref || read_ref(ref->peer_ref->name, sha1))\n> @@ -768,7 +770,7 @@ static void clear_push_info(void *util, const char *string)\n> static void free_remote_ref_states(struct ref_states *states)\n> {\n> \tstring_list_clear(&states->new, 0);\n> -\tstring_list_clear(&states->stale, 0);\n> +\tstring_list_clear(&states->stale, 1);\n> \tstring_list_clear(&states->tracked, 0);\n> \tstring_list_clear(&states->heads, 0);\n> \tstring_list_clear_func(&states->push, clear_push_info);\n> -- \n> 1.6.6.rc0.253.g1ec3\n"},{"id":"128834","messageId":"36ca99e90911302249r5f77f031o73cc7bb13dc375cf@mail.gmail.com","threadId":"21801","inReplyTo":"7viqcrbl22.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] get_ref_states: strdup entries and free util in stale list","fromName":"Bert Wesarg","fromEmail":"bert.wesarg@googlemail.com","sentAt":"2009-12-01T06:49:05Z","receivedAt":"2009-12-01T06:49:05Z","isPatch":true,"sender":{"key":"bert.wesarg@googlemail.com","avatar":"https://avatars.githubusercontent.com/u/111934?v=4"},"body":"On Tue, Dec 1, 2009 at 01:21, Junio C Hamano wrote:\n> Bert Wesarg writes:\n>\nThe entries of the stale branches string list are made of the stale\nrefs, which are immediately free'ed after the string list creation.\nTherefore the list entries use memory after free. This resulted for me\nin a corrupted branch list for 'git remote show' (duplicate entries\nand cut-off entries). The .util member of the string list entries are\nalso strdup() of the branch (full)name itself, but at clear time we\nrequest not to free the .util member. Which results in a memory leak.\n\n>> Signed-off-by: Bert Wesarg \n>\n> Hmm, the Subject: matches what the code does, but nobody mentions why it\n> is necessary (iow, what breaks in the current code and what becomes better\n> if the patch is applied).  The blank space before your \"S-o-b:\" line is\n> for you to describe these things.\nSure. unfortunately the code where the string list is filled is not in\nthe patch. But if you look at the code it should be self-explanatory.\n\n>\n>> ---\n\nThere is actually also an other solution: we could first strdup the\nref name to the .util member and take this as the input for the\nabbrev_ref()/string list entry and safe there the strdup.\n\nBert\n>>  builtin-remote.c |    6 ++++--\n>>  1 files changed, 4 insertions(+), 2 deletions(-)\n>>\n>> diff --git a/builtin-remote.c b/builtin-remote.c\n>> index 7916626..bb72e27 100644\n>> --- a/builtin-remote.c\n>> +++ b/builtin-remote.c\n>> @@ -272,7 +272,9 @@ static int get_ref_states(const struct ref *remote_refs, struct ref_states *stat\n>>                       die(\"Could not get fetch map for refspec %s\",\n>>                               states->remote->fetch_refspec[i]);\n>>\n>> -     states->new.strdup_strings = states->tracked.strdup_strings = 1;\n>> +     states->new.strdup_strings =\n>> +     states->tracked.strdup_strings =\n>> +     states->stale.strdup_strings = 1;\n>>       for (ref = fetch_map; ref; ref = ref->next) {\n>>               unsigned char sha1[20];\n>>               if (!ref->peer_ref || read_ref(ref->peer_ref->name, sha1))\n>> @@ -768,7 +770,7 @@ static void clear_push_info(void *util, const char *string)\n>>  static void free_remote_ref_states(struct ref_states *states)\n>>  {\n>>       string_list_clear(&states->new, 0);\n>> -     string_list_clear(&states->stale, 0);\n>> +     string_list_clear(&states->stale, 1);\n>>       string_list_clear(&states->tracked, 0);\n>>       string_list_clear(&states->heads, 0);\n>>       string_list_clear_func(&states->push, clear_push_info);\n>> --\n>> 1.6.6.rc0.253.g1ec3\n>\n"},{"id":"128838","messageId":"7vy6ln2llw.fsf@alter.siamese.dyndns.org","threadId":"21801","inReplyTo":"36ca99e90911302249r5f77f031o73cc7bb13dc375cf@mail.gmail.com","subject":"Re: [PATCH] get_ref_states: strdup entries and free util in stale list","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-12-01T07:34:51Z","receivedAt":"2009-12-01T07:34:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Bert Wesarg writes:\n\n> On Tue, Dec 1, 2009 at 01:21, Junio C Hamano wrote:\n> ...\n>> Hmm, the Subject: matches what the code does, but nobody mentions why it\n>> is necessary (iow, what breaks in the current code and what becomes better\n>> if the patch is applied). The blank space before your \"S-o-b:\" line is\n>> for you to describe these things.\n> Sure. unfortunately the code where the string list is filled is not in\n> the patch. But if you look at the code it should be self-explanatory.\n\nThat is _exactly_ why I want the description in the commit log message. I\ndon't want commit messages (or lack thereof) to force people to look at\nthe code outside the patch.\n\nOtherwise I'll have to ask _you_ to personally give the 7-line explanation\nyou just gave us to anybody who runs \"git log -p\" with the default context\nsize after this patch is applied. I do not think you have the bandwidth\nto handle that ;-).\n\n> There is actually also an other solution: we could first strdup the\n> ref name to the .util member and take this as the input for the\n> abbrev_ref()/string list entry and safe there the strdup.\n\nI too thought something like that may make sense, but it doesn't look like\nso, for two reasons:\n\n - string-list API is a bit cumbersome to use if you allocate the string\n yourself. You will be made responsible for freeing them, and often you\n do so by setting strdup_strings to true immediately before calling\n string_list_clear(), which is kind of ugly;\n\n - The ref abbrev_branch() is called and the address of whose substring is\n taken to be used as \"name\" in handle_one_branch() is refspec.src, but\n what goes to .util is refname that is refspec.dst---they are different\n strings and one is not a substring of the other.\n\nIOW, your patch text is good; it just would have been better with a bit\nmore explanation.\n\nThanks, will queue in 'maint'.\n"},{"id":"128846","messageId":"alpine.DEB.1.00.0912010934120.4985@pacific.mpi-cbg.de","threadId":"21801","inReplyTo":"0458f16c6ce906997aaf357c0c7368841ae83c36.1259625072.git.bert.wesarg@googlemail.com","subject":"Re: [PATCH] get_ref_states: strdup entries and free util in stale list","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2009-12-01T08:35:01Z","receivedAt":"2009-12-01T08:35:01Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Tue, 1 Dec 2009, Bert Wesarg wrote:\n\n> Signed-off-by: Bert Wesarg \n\nThanks. I trust you ran the test suite with valgrind just to make sure?\n\nCiao,\nDscho\n"},{"id":"128851","messageId":"36ca99e90912010105r428a7bfdw63928e8a5515bd1d@mail.gmail.com","threadId":"21801","inReplyTo":"alpine.DEB.1.00.0912010934120.4985@pacific.mpi-cbg.de","subject":"Re: [PATCH] get_ref_states: strdup entries and free util in stale list","fromName":"Bert Wesarg","fromEmail":"bert.wesarg@googlemail.com","sentAt":"2009-12-01T09:05:41Z","receivedAt":"2009-12-01T09:05:41Z","isPatch":true,"sender":{"key":"bert.wesarg@googlemail.com","avatar":"https://avatars.githubusercontent.com/u/111934?v=4"},"body":"On Tue, Dec 1, 2009 at 09:35, Johannes Schindelin\n wrote:\n> Hi,\n>\n> On Tue, 1 Dec 2009, Bert Wesarg wrote:\n>\n>> Signed-off-by: Bert Wesarg \n>\n> Thanks.  I trust you ran the test suite with valgrind just to make sure?\nNot the test suite. But my use case where I found the problem (Ie.\ncut-off branch names) which was 'git remote show '.\nThere are still invalid reads of size 4. I think the problem is the\nflex array member of 'struct ref' and strlen(). If its worth I can\nlook into this.\n\nBert\n>\n> Ciao,\n> Dscho\n>\n"},{"id":"128856","messageId":"36ca99e90912010132iee0d42fo933aeb12833ad1ad@mail.gmail.com","threadId":"21801","inReplyTo":"7vy6ln2llw.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] get_ref_states: strdup entries and free util in stale list","fromName":"Bert Wesarg","fromEmail":"bert.wesarg@googlemail.com","sentAt":"2009-12-01T09:32:37Z","receivedAt":"2009-12-01T09:32:37Z","isPatch":true,"sender":{"key":"bert.wesarg@googlemail.com","avatar":"https://avatars.githubusercontent.com/u/111934?v=4"},"body":"On Tue, Dec 1, 2009 at 08:34, Junio C Hamano wrote:\n> Bert Wesarg writes:\n>\n>> On Tue, Dec 1, 2009 at 01:21, Junio C Hamano wrote:\n>> ...\n>>> Hmm, the Subject: matches what the code does, but nobody mentions why it\n>>> is necessary (iow, what breaks in the current code and what becomes better\n>>> if the patch is applied). The blank space before your \"S-o-b:\" line is\n>>> for you to describe these things.\n>> Sure. unfortunately the code where the string list is filled is not in\n>> the patch. But if you look at the code it should be self-explanatory.\n>\n> That is _exactly_ why I want the description in the commit log message.  I\n> don't want commit messages (or lack thereof) to force people to look at\n> the code outside the patch.\n>\n> Otherwise I'll have to ask _you_ to personally give the 7-line explanation\n> you just gave us to anybody who runs \"git log -p\" with the default context\n> size after this patch is applied.  I do not think you have the bandwidth\n> to handle that ;-).\nYes. That makes perfectly sense. Sorry for the hassle.\n\n>\n>> There is actually also an other solution: we could first strdup the\n>> ref name to the .util member and take this as the input for the\n>> abbrev_ref()/string list entry and safe there the strdup.\n>\n> I too thought something like that may make sense, but it doesn't look like\n> so, for two reasons:\n>\n>  - string-list API is a bit cumbersome to use if you allocate the string\n>   yourself.  You will be made responsible for freeing them, and often you\n>   do so by setting strdup_strings to true immediately before calling\n>   string_list_clear(), which is kind of ugly;\n>\n>  - The ref abbrev_branch() is called and the address of whose substring is\n>   taken to be used as \"name\" in handle_one_branch() is refspec.src, but\n>   what goes to .util is refname that is refspec.dst---they are different\n>   strings and one is not a substring of the other.\nI don't see you point here. The current code is:\n\n\tfor (ref = stale_refs; ref; ref = ref->next) {\n\t\tstruct string_list_item *item =\n\t\t\tstring_list_append(abbrev_branch(ref->name), &states->stale);\n\t\titem->util = xstrdup(ref->name);\n\t}\nSo 0 == strcmp(item->string, abbrev_ref(item->util, \"refs/heads/\"))\nshould be true, shouldn't it?\n\nBert\n"},{"id":"128883","messageId":"36ca99e90912010753g1adc13e5v7e2a2b1eebc75183@mail.gmail.com","threadId":"21801","inReplyTo":"36ca99e90912010105r428a7bfdw63928e8a5515bd1d@mail.gmail.com","subject":"Re: [PATCH] get_ref_states: strdup entries and free util in stale list","fromName":"Bert Wesarg","fromEmail":"bert.wesarg@googlemail.com","sentAt":"2009-12-01T15:53:02Z","receivedAt":"2009-12-01T15:53:02Z","isPatch":true,"sender":{"key":"bert.wesarg@googlemail.com","avatar":"https://avatars.githubusercontent.com/u/111934?v=4"},"body":"On Tue, Dec 1, 2009 at 10:05, Bert Wesarg wrote:\n> On Tue, Dec 1, 2009 at 09:35, Johannes Schindelin\n>> Thanks.  I trust you ran the test suite with valgrind just to make sure?\n> Not the test suite. But my use case where I found the problem (Ie.\n> cut-off branch names) which was 'git remote show '.\n> There are still invalid reads of size 4. I think the problem is the\n> flex array member of 'struct ref' and strlen(). If its worth I can\n> look into this.\nI need this new suppression to run the test suite:\n\ndiff --git i/t/valgrind/default.supp w/t/valgrind/default.supp\nindex 9e013fa..39b080a 100644\n--- i/t/valgrind/default.supp\n+++ w/t/valgrind/default.supp\n@@ -43,3 +43,10 @@\n fun:write_buffer\n fun:write_loose_object\n }\n+\n+{\n+ writing-data-from-zlib-triggers-even-more-errors-2\n+ Memcheck:Param\n+ write(buf)\n+ obj:*libpthread-*.so\n+}\n\nBert\n"},{"id":"128900","messageId":"7v8wdm1ui1.fsf@alter.siamese.dyndns.org","threadId":"21801","inReplyTo":"36ca99e90912010132iee0d42fo933aeb12833ad1ad@mail.gmail.com","subject":"Re: [PATCH] get_ref_states: strdup entries and free util in stale list","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-12-01T17:20:22Z","receivedAt":"2009-12-01T17:20:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Bert Wesarg writes:\n\n>>  - The ref abbrev_branch() is called and the address of whose substring is\n>>   taken to be used as \"name\" in handle_one_branch() is refspec.src, but\n>>   what goes to .util is refname that is refspec.dst---they are different\n>>   strings and one is not a substring of the other.\n> I don't see you point here.\n\nOf course you don't ;-) because we were looking at different versions.\n\nI wanted to apply the same fix to both maint and master. For the code in\n'master' your observation is 100% correct.\n"},{"id":"128907","messageId":"36ca99e90912011014sd7372d0yc234873a26c2ae43@mail.gmail.com","threadId":"21801","inReplyTo":"7v8wdm1ui1.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] get_ref_states: strdup entries and free util in stale list","fromName":"Bert Wesarg","fromEmail":"bert.wesarg@googlemail.com","sentAt":"2009-12-01T18:14:30Z","receivedAt":"2009-12-01T18:14:30Z","isPatch":true,"sender":{"key":"bert.wesarg@googlemail.com","avatar":"https://avatars.githubusercontent.com/u/111934?v=4"},"body":"On Tue, Dec 1, 2009 at 18:20, Junio C Hamano wrote:\n> Bert Wesarg writes:\n>\n>>>  - The ref abbrev_branch() is called and the address of whose substring is\n>>>   taken to be used as \"name\" in handle_one_branch() is refspec.src, but\n>>>   what goes to .util is refname that is refspec.dst---they are different\n>>>   strings and one is not a substring of the other.\n>> I don't see you point here.\n>\n> Of course you don't ;-) because we were looking at different versions.\n>\n> I wanted to apply the same fix to both maint and master.  For the code in\n> 'master' your observation is 100% correct.\nA quick test with my use case does not show errors in the maint\nbranch. So it should not be needed (except the memory leak fix of the\n.util member). And valgrind confirms this.\n\nBert\n\n>\n"},{"id":"128908","messageId":"36ca99e90912011020u4bcc8802qd12049969d162c97@mail.gmail.com","threadId":"21801","inReplyTo":"36ca99e90912010105r428a7bfdw63928e8a5515bd1d@mail.gmail.com","subject":"Re: [PATCH] get_ref_states: strdup entries and free util in stale list","fromName":"Bert Wesarg","fromEmail":"bert.wesarg@googlemail.com","sentAt":"2009-12-01T18:20:55Z","receivedAt":"2009-12-01T18:20:55Z","isPatch":true,"sender":{"key":"bert.wesarg@googlemail.com","avatar":"https://avatars.githubusercontent.com/u/111934?v=4"},"body":"On Tue, Dec 1, 2009 at 10:05, Bert Wesarg wrote:\n> There are still invalid reads of size 4. I think the problem is the\n> flex array member of 'struct ref' and strlen(). If its worth I can\n> look into this.\nA short heads-up, here is the valgrind error I get for this invalid read:\n\n==27305== Invalid read of size 4\n==27305== at 0x4936AF: copy_ref (remote.c:870)\n==27305== by 0x4942E4: get_fetch_map (remote.c:1271)\n==27305== by 0x44473E: get_remote_ref_states (builtin-remote.c:271)\n==27305== by 0x446DCE: cmd_remote (builtin-remote.c:1022)\n==27305== by 0x4045F0: handle_internal_command (git.c:257)\n==27305== by 0x404B8F: main (git.c:482)\n==27305== Address 0x5b5ba38 is 104 bytes inside a block of size 107 alloc'd\n==27305== at 0x4C24477: calloc (vg_replace_malloc.c:418)\n==27305== by 0x4B09AD: xcalloc (wrapper.c:75)\n==27305== by 0x493924: alloc_ref_with_prefix (remote.c:853)\n==27305== by 0x46653B: get_remote_heads (connect.c:96)\n==27305== by 0x4A9347: get_refs_via_connect (transport.c:453)\n==27305== by 0x4A7F14: transport_get_remote_refs (transport.c:895)\n==27305== by 0x4445B6: get_remote_ref_states (builtin-remote.c:810)\n==27305== by 0x446DCE: cmd_remote (builtin-remote.c:1022)\n==27305== by 0x4045F0: handle_internal_command (git.c:257)\n==27305== by 0x404B8F: main (git.c:482)\n"},{"id":"128918","messageId":"7vaay2zekz.fsf@alter.siamese.dyndns.org","threadId":"21801","inReplyTo":"36ca99e90912011014sd7372d0yc234873a26c2ae43@mail.gmail.com","subject":"Re: [PATCH] get_ref_states: strdup entries and free util in stale list","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-12-01T19:20:12Z","receivedAt":"2009-12-01T19:20:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Bert Wesarg writes:\n\n> A quick test with my use case does not show errors in the maint\n> branch. So it should not be needed (except the memory leak fix of the\n> .util member). And valgrind confirms this.\n\nThanks.\n"}]}