{"thread":{"id":"62217","subject":"[PATCH 0/2] read-cache: two small leak fixes","startedAt":"2024-09-30T11:40:27Z","lastAt":"2024-10-02T10:49:21Z","messageCount":11,"participants":["Derrick Stolee via GitGitGadget","Patrick Steinhardt","Derrick Stolee"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"503726","messageId":"pull.1801.git.1727696424.gitgitgadget@gmail.com","threadId":"62217","inReplyTo":null,"subject":"[PATCH 0/2] read-cache: two small leak fixes","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-09-30T11:40:22Z","receivedAt":"2024-09-30T11:40:27Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"I found these two small memory leaks while investigating things involving\nthe index. I thought they were small enough to send independently.\n\nI initially wrote this as two patches because the second patch was more\ninvolved to work around the \"cleanup\" label, and my branch was based on\n2.46.1. This code has changed recently due to d1c53f6703 (read-cache: fix\nleaking hashfile when writing index fails, 2024-08-14), so now the second\npatch is simpler and these could probably be merged into one.\n\nThanks, -Stolee\n\nDerrick Stolee (2):\n read-cache: free threaded memory pool\n read-cache: free hash context in do_write_index()\n\n read-cache.c | 2 ++\n 1 file changed, 2 insertions(+)\n\n\nbase-commit: 6258f68c3c1092c901337895c864073dcdea9213\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1801%2Fderrickstolee%2Fleaks-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1801/derrickstolee/leaks-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1801\n-- \ngitgitgadget\n"},{"id":"503727","messageId":"9a45b15ea4b9864cd3cff066ecd9281c4539d5f7.1727696424.git.gitgitgadget@gmail.com","threadId":"62217","inReplyTo":"pull.1801.git.1727696424.gitgitgadget@gmail.com","subject":"[PATCH 1/2] read-cache: free threaded memory pool","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-09-30T11:40:23Z","receivedAt":"2024-09-30T11:40:28Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nIn load_cache_entries_threaded(), each thread is allocated its own\nmemory pool. This pool needs to be cleaned up while closing the threads\ndown, or it will be leaked.\n\nSigned-off-by: Derrick Stolee \n---\n read-cache.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/read-cache.c b/read-cache.c\nindex 764fdfec465..3c078afadbc 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -2188,6 +2188,7 @@ static unsigned long load_cache_entries_threaded(struct index_state *istate, con\n \t\tif (err)\n \t\t\tdie(_(\"unable to join load_cache_entries thread: %s\"), strerror(err));\n \t\tmem_pool_combine(istate->ce_mem_pool, p->ce_mem_pool);\n+\t\tfree(p->ce_mem_pool);\n \t\tconsumed += p->consumed;\n \t}\n \n-- \ngitgitgadget\n\n"},{"id":"503728","messageId":"b6fe5b3ef7e5f4ac8cc339685d92e3ac39fcb456.1727696424.git.gitgitgadget@gmail.com","threadId":"62217","inReplyTo":"pull.1801.git.1727696424.gitgitgadget@gmail.com","subject":"[PATCH 2/2] read-cache: free hash context in do_write_index()","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-09-30T11:40:24Z","receivedAt":"2024-09-30T11:40:29Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nWhile writing an index, a 'git_hash_ctx' is allocated for hashing the\nfile contents. This should be freed as the method exits.\n\nSigned-off-by: Derrick Stolee \n---\n read-cache.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/read-cache.c b/read-cache.c\nindex 3c078afadbc..51845c2e611 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -3126,6 +3126,7 @@ out:\n \t\tfree_hashfile(f);\n \tstrbuf_release(&sb);\n \tfree(ieot);\n+\tfree(eoie_c);\n \treturn ret;\n }\n \n-- \ngitgitgadget\n"},{"id":"503729","messageId":"ZvqaRIY1OzOIh1bc@pks.im","threadId":"62217","inReplyTo":"b6fe5b3ef7e5f4ac8cc339685d92e3ac39fcb456.1727696424.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 2/2] read-cache: free hash context in do_write_index()","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-30T12:32:10Z","receivedAt":"2024-09-30T12:32:17Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Mon, Sep 30, 2024 at 11:40:24AM +0000, Derrick Stolee via GitGitGadget wrote:\n> From: Derrick Stolee \n> \n> While writing an index, a 'git_hash_ctx' is allocated for hashing the\n> file contents. This should be freed as the method exits.\n> \n> Signed-off-by: Derrick Stolee \n> ---\n> read-cache.c | 1 +\n> 1 file changed, 1 insertion(+)\n> \n> diff --git a/read-cache.c b/read-cache.c\n> index 3c078afadbc..51845c2e611 100644\n> --- a/read-cache.c\n> +++ b/read-cache.c\n> @@ -3126,6 +3126,7 @@ out:\n> \t\tfree_hashfile(f);\n> \tstrbuf_release(&sb);\n> \tfree(ieot);\n> +\tfree(eoie_c);\n> \treturn ret;\n> }\n\nYup, this one looks correct. I've sent out an equivalent patch via [1] a\ncouple hours ago.\n\nPatrick\n\n[1]: https://lore.kernel.org/git/c51f40c5bd0c56967e348363e784222de7884b79.1727687410.git.ps@pks.im/\n"},{"id":"503730","messageId":"ZvqaTqegJxow1x-b@pks.im","threadId":"62217","inReplyTo":"9a45b15ea4b9864cd3cff066ecd9281c4539d5f7.1727696424.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/2] read-cache: free threaded memory pool","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-30T12:32:14Z","receivedAt":"2024-09-30T12:32:18Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Mon, Sep 30, 2024 at 11:40:23AM +0000, Derrick Stolee via GitGitGadget wrote:\n> From: Derrick Stolee \n> \n> In load_cache_entries_threaded(), each thread is allocated its own\n\ns/allocated/allocating/\n\n> memory pool. This pool needs to be cleaned up while closing the threads\n> down, or it will be leaked.\n> \n> Signed-off-by: Derrick Stolee \n> ---\n> read-cache.c | 1 +\n> 1 file changed, 1 insertion(+)\n> \n> diff --git a/read-cache.c b/read-cache.c\n> index 764fdfec465..3c078afadbc 100644\n> --- a/read-cache.c\n> +++ b/read-cache.c\n> @@ -2188,6 +2188,7 @@ static unsigned long load_cache_entries_threaded(struct index_state *istate, con\n> \t\tif (err)\n> \t\t\tdie(_(\"unable to join load_cache_entries thread: %s\"), strerror(err));\n> \t\tmem_pool_combine(istate->ce_mem_pool, p->ce_mem_pool);\n> +\t\tfree(p->ce_mem_pool);\n> \t\tconsumed += p->consumed;\n> \t}\n\nOkay. We move over the contents of the pool, but forgot to free the pool\nitself. As far as I can see the pool is always allocated and only used\nin two functions, both of which assume that it is allocated. So I wonder\nwhy it is allocated in the first place instead of making it a direct\nmember of `struct load_cache_entries_thread_data`.\n\nPatrick\n"},{"id":"503829","messageId":"0f257003-5a8d-4eca-987d-517d9754f1fd@gmail.com","threadId":"62217","inReplyTo":"ZvqaRIY1OzOIh1bc@pks.im","subject":"Re: [PATCH 2/2] read-cache: free hash context in do_write_index()","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2024-10-01T10:01:06Z","receivedAt":"2024-10-01T10:01:10Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 9/30/24 8:32 AM, Patrick Steinhardt wrote:\n> On Mon, Sep 30, 2024 at 11:40:24AM +0000, Derrick Stolee via GitGitGadget wrote:\n>> From: Derrick Stolee \n\n>> diff --git a/read-cache.c b/read-cache.c\n>> index 3c078afadbc..51845c2e611 100644\n>> --- a/read-cache.c\n>> +++ b/read-cache.c\n>> @@ -3126,6 +3126,7 @@ out:\n>> \t\tfree_hashfile(f);\n>> \tstrbuf_release(&sb);\n>> \tfree(ieot);\n>> +\tfree(eoie_c);\n>> \treturn ret;\n>> }\n> \n> Yup, this one looks correct. I've sent out an equivalent patch via [1] a\n> couple hours ago.\n> \n> Patrick\n> \n> [1]: https://lore.kernel.org/git/c51f40c5bd0c56967e348363e784222de7884b79.1727687410.git.ps@pks.im/\n\nSorry for the collision. I had checked when prepping the GGG PR on\nFriday but forgot to check again before submitting. Your patch is\nbetter in substance and context.\n\nThanks,\n-Stolee\n\n"},{"id":"503835","messageId":"ZvvL6bdcV-05tFYY@pks.im","threadId":"62217","inReplyTo":"0f257003-5a8d-4eca-987d-517d9754f1fd@gmail.com","subject":"Re: [PATCH 2/2] read-cache: free hash context in do_write_index()","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-10-01T10:16:09Z","receivedAt":"2024-10-01T10:16:16Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Oct 01, 2024 at 06:01:06AM -0400, Derrick Stolee wrote:\n> On 9/30/24 8:32 AM, Patrick Steinhardt wrote:\n> > On Mon, Sep 30, 2024 at 11:40:24AM +0000, Derrick Stolee via GitGitGadget wrote:\n> > > From: Derrick Stolee \n> \n> > > diff --git a/read-cache.c b/read-cache.c\n> > > index 3c078afadbc..51845c2e611 100644\n> > > --- a/read-cache.c\n> > > +++ b/read-cache.c\n> > > @@ -3126,6 +3126,7 @@ out:\n> > > \t\tfree_hashfile(f);\n> > > \tstrbuf_release(&sb);\n> > > \tfree(ieot);\n> > > +\tfree(eoie_c);\n> > > \treturn ret;\n> > > }\n> > \n> > Yup, this one looks correct. I've sent out an equivalent patch via [1] a\n> > couple hours ago.\n> > \n> > Patrick\n> > \n> > [1]: https://lore.kernel.org/git/c51f40c5bd0c56967e348363e784222de7884b79.1727687410.git.ps@pks.im/\n> \n> Sorry for the collision. I had checked when prepping the GGG PR on\n> Friday but forgot to check again before submitting. Your patch is\n> better in substance and context.\n\nNothing to be sorry about, both series were sent out quite close to one\nanother. Quite on the contrary, thanks for fixing this leak :)\n\nI don't mind much which of these versions lands in the tree, and the\nresulting conflict is trivial to solve anyway.\n\nPatrick\n"},{"id":"503838","messageId":"8ebebcd5-4ddb-4c1e-9bf8-4e9a79944e2b@gmail.com","threadId":"62217","inReplyTo":"ZvqaTqegJxow1x-b@pks.im","subject":"Re: [PATCH 1/2] read-cache: free threaded memory pool","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2024-10-01T13:20:01Z","receivedAt":"2024-10-01T13:20:03Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 9/30/24 8:32 AM, Patrick Steinhardt wrote:\n> On Mon, Sep 30, 2024 at 11:40:23AM +0000, Derrick Stolee via GitGitGadget wrote:\n>> From: Derrick Stolee \n>>\n>> In load_cache_entries_threaded(), each thread is allocated its own\n> \n> s/allocated/allocating/\n\nYou're right that the wording is awkward but I'm not thrilled with the\nsuggested alternative.\n\nPerhaps \"each thread allocates its own\"\n\n>> memory pool. This pool needs to be cleaned up while closing the threads\n>> down, or it will be leaked.\n\n> Okay. We move over the contents of the pool, but forgot to free the pool\n> itself. As far as I can see the pool is always allocated and only used\n> in two functions, both of which assume that it is allocated. So I wonder\n> why it is allocated in the first place instead of making it a direct\n> member of `struct load_cache_entries_thread_data`.\n\nI took a look at what it would take to replace the pointer with an inline\nstruct but found complications with situations such as the find_mem_pool()\nmethod. While we could replace some of the logic to recognize the new\ntype, the existing logic seems to depend on using the NULL pointer as an\nindicator that the pool should be lazily initialized.\n\nIf we were to pull the struct inline, we would either need another boolean\nto indicate initialization or lose lazy initialization.\n\nI'm leaning towards the simpler leak fix over the disruption of that\nchange.\n\nThanks,\n-Stolee\n\n"},{"id":"503841","messageId":"ZvwDEyhe-3DyQp75@pks.im","threadId":"62217","inReplyTo":"8ebebcd5-4ddb-4c1e-9bf8-4e9a79944e2b@gmail.com","subject":"Re: [PATCH 1/2] read-cache: free threaded memory pool","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-10-01T14:11:31Z","receivedAt":"2024-10-01T14:11:38Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Oct 01, 2024 at 09:20:01AM -0400, Derrick Stolee wrote:\n> On 9/30/24 8:32 AM, Patrick Steinhardt wrote:\n> > On Mon, Sep 30, 2024 at 11:40:23AM +0000, Derrick Stolee via GitGitGadget wrote:\n> > > From: Derrick Stolee \n> > > \n> > > In load_cache_entries_threaded(), each thread is allocated its own\n> > \n> > s/allocated/allocating/\n> \n> You're right that the wording is awkward but I'm not thrilled with the\n> suggested alternative.\n> \n> Perhaps \"each thread allocates its own\"\n\nSure, works for me :)\n\n> > > memory pool. This pool needs to be cleaned up while closing the threads\n> > > down, or it will be leaked.\n> \n> > Okay. We move over the contents of the pool, but forgot to free the pool\n> > itself. As far as I can see the pool is always allocated and only used\n> > in two functions, both of which assume that it is allocated. So I wonder\n> > why it is allocated in the first place instead of making it a direct\n> > member of `struct load_cache_entries_thread_data`.\n> \n> I took a look at what it would take to replace the pointer with an inline\n> struct but found complications with situations such as the find_mem_pool()\n> method. While we could replace some of the logic to recognize the new\n> type, the existing logic seems to depend on using the NULL pointer as an\n> indicator that the pool should be lazily initialized.\n> \n> If we were to pull the struct inline, we would either need another boolean\n> to indicate initialization or lose lazy initialization.\n> \n> I'm leaning towards the simpler leak fix over the disruption of that\n> change.\n\nFair enough, no complaint from my side. I thought it would've been easy,\nbut didn't dive deep. So if you say it is harder than I made it out to\nbe with my shallow understanding I'm going to trust your judgement.\nAfter all, the leak fix is a strict improvement by itself.\n\nPatrick\n"},{"id":"503850","messageId":"pull.1801.v2.git.1727804265033.gitgitgadget@gmail.com","threadId":"62217","inReplyTo":"pull.1801.git.1727696424.gitgitgadget@gmail.com","subject":"[PATCH v2] read-cache: free threaded memory pool","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-10-01T17:37:44Z","receivedAt":"2024-10-01T17:37:48Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nIn load_cache_entries_threaded(), each thread allocates its own memory\npool. This pool needs to be cleaned up while closing the threads down,\nor it will be leaked.\n\nThis ce_mem_pool pointer could theoretically be converted to an inline\ncopy of the struct, but the use of a pointer helps with existing lazy-\ninitialization logic. Adjusting that behavior only to avoid this pointer\nwould be a much bigger change.\n\nSigned-off-by: Derrick Stolee \n---\n read-cache: two small leak fixes\n \n This v2 removes the duplicate patch and updates the commit message.\n \n Thanks, -Stolee\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1801%2Fderrickstolee%2Fleaks-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1801/derrickstolee/leaks-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/1801\n\nRange-diff vs v1:\n\n 1: 9a45b15ea4b ! 1: 220a098c93c read-cache: free threaded memory pool\n @@ Metadata\n ## Commit message ##\n read-cache: free threaded memory pool\n \n - In load_cache_entries_threaded(), each thread is allocated its own\n - memory pool. This pool needs to be cleaned up while closing the threads\n - down, or it will be leaked.\n + In load_cache_entries_threaded(), each thread allocates its own memory\n + pool. This pool needs to be cleaned up while closing the threads down,\n + or it will be leaked.\n +\n + This ce_mem_pool pointer could theoretically be converted to an inline\n + copy of the struct, but the use of a pointer helps with existing lazy-\n + initialization logic. Adjusting that behavior only to avoid this pointer\n + would be a much bigger change.\n \n Signed-off-by: Derrick Stolee \n \n 2: b6fe5b3ef7e < -: ----------- read-cache: free hash context in do_write_index()\n\n\n read-cache.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/read-cache.c b/read-cache.c\nindex 764fdfec465..3c078afadbc 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -2188,6 +2188,7 @@ static unsigned long load_cache_entries_threaded(struct index_state *istate, con\n \t\tif (err)\n \t\t\tdie(_(\"unable to join load_cache_entries thread: %s\"), strerror(err));\n \t\tmem_pool_combine(istate->ce_mem_pool, p->ce_mem_pool);\n+\t\tfree(p->ce_mem_pool);\n \t\tconsumed += p->consumed;\n \t}\n \n\nbase-commit: 6258f68c3c1092c901337895c864073dcdea9213\n-- \ngitgitgadget\n"},{"id":"503900","messageId":"Zv0lISjo-5bHzEzW@pks.im","threadId":"62217","inReplyTo":"pull.1801.v2.git.1727804265033.gitgitgadget@gmail.com","subject":"Re: [PATCH v2] read-cache: free threaded memory pool","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-10-02T10:49:12Z","receivedAt":"2024-10-02T10:49:21Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Oct 01, 2024 at 05:37:44PM +0000, Derrick Stolee via GitGitGadget wrote:\n> From: Derrick Stolee \n> \n> In load_cache_entries_threaded(), each thread allocates its own memory\n> pool. This pool needs to be cleaned up while closing the threads down,\n> or it will be leaked.\n> \n> This ce_mem_pool pointer could theoretically be converted to an inline\n> copy of the struct, but the use of a pointer helps with existing lazy-\n> initialization logic. Adjusting that behavior only to avoid this pointer\n> would be a much bigger change.\n\nThanks, this looks good to me.\n\nPatrick\n"}]}