{"thread":{"id":"57827","subject":"[PATCH] clone: plug a miniscule leak","startedAt":"2022-05-01T05:17:24Z","lastAt":"2022-05-02T20:39:56Z","messageCount":4,"participants":["Junio C Hamano","Derrick Stolee","Philip Oakley"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"454695","messageId":"xmqqlevl4ysk.fsf@gitster.g","threadId":"57827","inReplyTo":null,"subject":"[PATCH] clone: plug a miniscule leak","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-05-01T05:17:15Z","receivedAt":"2022-05-01T05:17:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"The remote_name variable is first assigned a copy of the value of\nthe \"clone.defaultremotename\" configuration variable and then by the\nvalue of the \"--origin\" command line option. The former is prepared\nto see multiple instances of the variable by freeing the current\nvalue of the variable before a copy of the newly discovered value\ngets assigned to it. The latter blindly assigned a copy of the new\nvalue to it, thereby leaking the value read from the configuration.\n\nSigned-off-by: Junio C Hamano \n---\n\n This was discovered by a recently removed bogus coccinelle\n rewrite rule---if you apply an incorrect change suggested by the\n bogus rewrite rule to unconditionally assign to remote_name a\n copy of option_origin, or NULL, the value read from the\n configuration would be lost and never be used, thereby breaking\n a test to ensure the configuration is used, instead of the\n default remote nickname \"origin\".\n\n Perhaps a Coccinelle rule like this might have caught similar\n leaks:\n\n\t@@\n\texpression E;\n\texpression V;\n\t@@\n\t- if (E)\n\t- V = xstrdup(E);\n\t+ if (E) {\n\t+ free(V);\n\t+ V = xstrdup(E);\n\t+ }\n\n The fact that the result of xstrdup() is assigned to V is that V\n is meant to hold a pointer to an allocated piece of memory.\n\n With the preimage of the above semantic patch, it is reasonable\n to expect that V may be initialized to NULL or may be holding a\n pointer to a piece of allocated memory when the control reaches\n here, because otherwise, V will be either need to be freed (when\n E was not NULL, in which case we assigned the result of\n xstrdup() to it) or V has garbage that cannot be freed later.\n \n builtin/clone.c | 4 +++-\n 1 file changed, 3 insertions(+), 1 deletion(-)\n\ndiff --git c/builtin/clone.c w/builtin/clone.c\nindex 5231656379..194d50f75f 100644\n--- c/builtin/clone.c\n+++ w/builtin/clone.c\n@@ -1106,8 +1106,10 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \t * apply the remote name provided by --origin only after this second\n \t * call to git_config, to ensure it overrides all config-based values.\n \t */\n-\tif (option_origin != NULL)\n+\tif (option_origin != NULL) {\n+\t\tfree(remote_name);\n \t\tremote_name = xstrdup(option_origin);\n+\t}\n \n \tif (remote_name == NULL)\n \t\tremote_name = xstrdup(\"origin\");\n"},{"id":"454714","messageId":"5e8eff95-d768-439d-e2df-d575a0038ffd@github.com","threadId":"57827","inReplyTo":"xmqqlevl4ysk.fsf@gitster.g","subject":"Re: [PATCH] clone: plug a miniscule leak","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-05-02T13:43:05Z","receivedAt":"2022-05-02T13:43:11Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 5/1/2022 1:17 AM, Junio C Hamano wrote:\n> Perhaps a Coccinelle rule like this might have caught similar\n> leaks:\n> \n> \t@@\n> \texpression E;\n> \texpression V;\n> \t@@\n> \t- if (E)\n> \t- V = xstrdup(E);\n> \t+ if (E) {\n> \t+ free(V);\n> \t+ V = xstrdup(E);\n> \t+ }\n> \n> The fact that the result of xstrdup() is assigned to V is that V\n> is meant to hold a pointer to an allocated piece of memory.\n> \n> With the preimage of the above semantic patch, it is reasonable\n> to expect that V may be initialized to NULL or may be holding a\n> pointer to a piece of allocated memory when the control reaches\n> here, because otherwise, V will be either need to be freed (when\n> E was not NULL, in which case we assigned the result of\n> xstrdup() to it) or V has garbage that cannot be freed later.\n\nInitially, I did think \"what if V is not initialized to NULL?\" but\nyou are right that the code would already be broken in that case.\n\n> -\tif (option_origin != NULL)\n\nThis technically wouldn't hit your rule, since \"E\" isn't just the\nvariable name, as we typically do with our style. Is that something\nthat Coccinelle automatically simplifies?\n\n> +\tif (option_origin != NULL) {\n\nDo you want to take this opportunity to drop the \"!= NULL\" here?\n\n> +\t\tfree(remote_name);\n> \t\tremote_name = xstrdup(option_origin);\n> +\t}\n> > \tif (remote_name == NULL)\n\nOr do you want to keep similar style from the surrounding code?\n\nEither way, looks good to me.\n\nThanks,\n-Stolee\n"},{"id":"454735","messageId":"xmqqzgjzzwnd.fsf@gitster.g","threadId":"57827","inReplyTo":"5e8eff95-d768-439d-e2df-d575a0038ffd@github.com","subject":"Re: [PATCH] clone: plug a miniscule leak","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-05-02T17:12:22Z","receivedAt":"2022-05-02T17:12:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Derrick Stolee writes:\n\n>> -\tif (option_origin != NULL)\n>\n> This technically wouldn't hit your rule, since \"E\" isn't just the\n> variable name, as we typically do with our style. Is that something\n> that Coccinelle automatically simplifies?\n>\n>> +\tif (option_origin != NULL) {\n>\n> Do you want to take this opportunity to drop the \"!= NULL\" here?\n>\n>> +\t\tfree(remote_name);\n>> \t\tremote_name = xstrdup(option_origin);\n>> +\t}\n>> > \tif (remote_name == NULL)\n>\n> Or do you want to keep similar style from the surrounding code?\n\nI think that it is better to leave that particular clean-up to\nthe equals-null.cocci topic started by Elia; I know having them\nseparate would cause a merge conflict, but even if I change them\nhere, it will result in the same merge conflict anyway ;-)\n"},{"id":"454749","messageId":"225433fb-d5ae-fc92-8d50-03162e75780e@iee.email","threadId":"57827","inReplyTo":"xmqqzgjzzwnd.fsf@gitster.g","subject":"Re: [PATCH] clone: plug a miniscule leak","fromName":"Philip Oakley","fromEmail":"philipoakley@iee.email","sentAt":"2022-05-02T20:39:38Z","receivedAt":"2022-05-02T20:39:56Z","isPatch":true,"sender":{"key":"philipoakley@iee.email","avatar":"https://avatars.githubusercontent.com/u/914343?v=4"},"body":"CC'ing Elia as per the mention. Thread\nhttps://lore.kernel.org/git/xmqqzgjzzwnd.fsf@gitster.g/t/#u.\n\nOn 02/05/2022 18:12, Junio C Hamano wrote:  \n> Derrick Stolee writes:\n>\n>>> -\tif (option_origin != NULL)\n>> This technically wouldn't hit your rule, since \"E\" isn't just the\n>> variable name, as we typically do with our style. Is that something\n>> that Coccinelle automatically simplifies?\n>>\n>>> +\tif (option_origin != NULL) {\n>> Do you want to take this opportunity to drop the \"!= NULL\" here?\n>>\n>>> +\t\tfree(remote_name);\n>>> \t\tremote_name = xstrdup(option_origin);\n>>> +\t}\n>>>> \tif (remote_name == NULL)\n>> Or do you want to keep similar style from the surrounding code?\n> I think that it is better to leave that particular clean-up to\n> the equals-null.cocci topic started by Elia; I know having them\n> separate would cause a merge conflict, but even if I change them\n> here, it will result in the same merge conflict anyway ;-)\n\n"}]}