read-cache: two small leak fixes #1801
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
derrickstolee
force-pushed
the
leaks
branch
from
608f020
to
b6fe5b3
Compare
|
/submit |
|
Submitted as pull.1801.git.1727696424.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
![gitgitgadget[bot]](https://avatars.githubusercontent.com/in/12836?s=60&v=4){kind=small}
read-cache.c
Outdated
| @@ -3125,6 +3126,7 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile, | |||
| free_hashfile(f); | |||
There was a problem hiding this comment.
On the Git mailing list, Patrick Steinhardt wrote (reply to this):
On Mon, Sep 30, 2024 at 11:40:24AM +0000, Derrick Stolee via GitGitGadget wrote:
> From: Derrick Stolee <stolee@gmail.com>
>
> While writing an index, a 'git_hash_ctx' is allocated for hashing the
> file contents. This should be freed as the method exits.
>
> Signed-off-by: Derrick Stolee <stolee@gmail.com>
> ---
> read-cache.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/read-cache.c b/read-cache.c
> index 3c078afadbc..51845c2e611 100644
> --- a/read-cache.c
> +++ b/read-cache.c
> @@ -3126,6 +3126,7 @@ out:
> free_hashfile(f);
> strbuf_release(&sb);
> free(ieot);
> + free(eoie_c);
> return ret;
> }
Yup, this one looks correct. I've sent out an equivalent patch via [1] a
couple hours ago.
Patrick
[1]: https://lore.kernel.org/git/c51f40c5bd0c56967e348363e784222de7884b79.1727687410.git.ps@pks.im/There was a problem hiding this comment.
On the Git mailing list, Derrick Stolee wrote (reply to this):
On 9/30/24 8:32 AM, Patrick Steinhardt wrote:
> On Mon, Sep 30, 2024 at 11:40:24AM +0000, Derrick Stolee via GitGitGadget wrote:
>> From: Derrick Stolee <stolee@gmail.com>
>> diff --git a/read-cache.c b/read-cache.c
>> index 3c078afadbc..51845c2e611 100644
>> --- a/read-cache.c
>> +++ b/read-cache.c
>> @@ -3126,6 +3126,7 @@ out:
>> free_hashfile(f);
>> strbuf_release(&sb);
>> free(ieot);
>> + free(eoie_c);
>> return ret;
>> }
> > Yup, this one looks correct. I've sent out an equivalent patch via [1] a
> couple hours ago.
> > Patrick
> > [1]: https://lore.kernel.org/git/c51f40c5bd0c56967e348363e784222de7884b79.1727687410.git.ps@pks.im/
Sorry for the collision. I had checked when prepping the GGG PR on
Friday but forgot to check again before submitting. Your patch is
better in substance and context.
Thanks,
-Stolee
There was a problem hiding this comment.
On the Git mailing list, Patrick Steinhardt wrote (reply to this):
On Tue, Oct 01, 2024 at 06:01:06AM -0400, Derrick Stolee wrote:
> On 9/30/24 8:32 AM, Patrick Steinhardt wrote:
> > On Mon, Sep 30, 2024 at 11:40:24AM +0000, Derrick Stolee via GitGitGadget wrote:
> > > From: Derrick Stolee <stolee@gmail.com>
>
> > > diff --git a/read-cache.c b/read-cache.c
> > > index 3c078afadbc..51845c2e611 100644
> > > --- a/read-cache.c
> > > +++ b/read-cache.c
> > > @@ -3126,6 +3126,7 @@ out:
> > > free_hashfile(f);
> > > strbuf_release(&sb);
> > > free(ieot);
> > > + free(eoie_c);
> > > return ret;
> > > }
> >
> > Yup, this one looks correct. I've sent out an equivalent patch via [1] a
> > couple hours ago.
> >
> > Patrick
> >
> > [1]: https://lore.kernel.org/git/c51f40c5bd0c56967e348363e784222de7884b79.1727687410.git.ps@pks.im/
>
> Sorry for the collision. I had checked when prepping the GGG PR on
> Friday but forgot to check again before submitting. Your patch is
> better in substance and context.
Nothing to be sorry about, both series were sent out quite close to one
another. Quite on the contrary, thanks for fixing this leak :)
I don't mind much which of these versions lands in the tree, and the
resulting conflict is trivial to solve anyway.
Patrick
| @@ -2188,6 +2188,7 @@ static unsigned long load_cache_entries_threaded(struct index_state *istate, con | |||
| if (err) | |||
There was a problem hiding this comment.
On the Git mailing list, Patrick Steinhardt wrote (reply to this):
On Mon, Sep 30, 2024 at 11:40:23AM +0000, Derrick Stolee via GitGitGadget wrote:
> From: Derrick Stolee <stolee@gmail.com>
>
> In load_cache_entries_threaded(), each thread is allocated its own
s/allocated/allocating/
> memory pool. This pool needs to be cleaned up while closing the threads
> down, or it will be leaked.
>
> Signed-off-by: Derrick Stolee <stolee@gmail.com>
> ---
> read-cache.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/read-cache.c b/read-cache.c
> index 764fdfec465..3c078afadbc 100644
> --- a/read-cache.c
> +++ b/read-cache.c
> @@ -2188,6 +2188,7 @@ static unsigned long load_cache_entries_threaded(struct index_state *istate, con
> if (err)
> die(_("unable to join load_cache_entries thread: %s"), strerror(err));
> mem_pool_combine(istate->ce_mem_pool, p->ce_mem_pool);
> + free(p->ce_mem_pool);
> consumed += p->consumed;
> }
Okay. We move over the contents of the pool, but forgot to free the pool
itself. As far as I can see the pool is always allocated and only used
in two functions, both of which assume that it is allocated. So I wonder
why it is allocated in the first place instead of making it a direct
member of `struct load_cache_entries_thread_data`.
PatrickThere was a problem hiding this comment.
On the Git mailing list, Derrick Stolee wrote (reply to this):
On 9/30/24 8:32 AM, Patrick Steinhardt wrote:
> On Mon, Sep 30, 2024 at 11:40:23AM +0000, Derrick Stolee via GitGitGadget wrote:
>> From: Derrick Stolee <stolee@gmail.com>
>>
>> In load_cache_entries_threaded(), each thread is allocated its own
> > s/allocated/allocating/
You're right that the wording is awkward but I'm not thrilled with the
suggested alternative.
Perhaps "each thread allocates its own"
>> memory pool. This pool needs to be cleaned up while closing the threads
>> down, or it will be leaked.
> Okay. We move over the contents of the pool, but forgot to free the pool
> itself. As far as I can see the pool is always allocated and only used
> in two functions, both of which assume that it is allocated. So I wonder
> why it is allocated in the first place instead of making it a direct
> member of `struct load_cache_entries_thread_data`.
I took a look at what it would take to replace the pointer with an inline
struct but found complications with situations such as the find_mem_pool()
method. While we could replace some of the logic to recognize the new
type, the existing logic seems to depend on using the NULL pointer as an
indicator that the pool should be lazily initialized.
If we were to pull the struct inline, we would either need another boolean
to indicate initialization or lose lazy initialization.
I'm leaning towards the simpler leak fix over the disruption of that
change.
Thanks,
-Stolee
There was a problem hiding this comment.
On the Git mailing list, Patrick Steinhardt wrote (reply to this):
On Tue, Oct 01, 2024 at 09:20:01AM -0400, Derrick Stolee wrote:
> On 9/30/24 8:32 AM, Patrick Steinhardt wrote:
> > On Mon, Sep 30, 2024 at 11:40:23AM +0000, Derrick Stolee via GitGitGadget wrote:
> > > From: Derrick Stolee <stolee@gmail.com>
> > >
> > > In load_cache_entries_threaded(), each thread is allocated its own
> >
> > s/allocated/allocating/
>
> You're right that the wording is awkward but I'm not thrilled with the
> suggested alternative.
>
> Perhaps "each thread allocates its own"
Sure, works for me :)
> > > memory pool. This pool needs to be cleaned up while closing the threads
> > > down, or it will be leaked.
>
> > Okay. We move over the contents of the pool, but forgot to free the pool
> > itself. As far as I can see the pool is always allocated and only used
> > in two functions, both of which assume that it is allocated. So I wonder
> > why it is allocated in the first place instead of making it a direct
> > member of `struct load_cache_entries_thread_data`.
>
> I took a look at what it would take to replace the pointer with an inline
> struct but found complications with situations such as the find_mem_pool()
> method. While we could replace some of the logic to recognize the new
> type, the existing logic seems to depend on using the NULL pointer as an
> indicator that the pool should be lazily initialized.
>
> If we were to pull the struct inline, we would either need another boolean
> to indicate initialization or lose lazy initialization.
>
> I'm leaning towards the simpler leak fix over the disruption of that
> change.
Fair enough, no complaint from my side. I thought it would've been easy,
but didn't dive deep. So if you say it is harder than I made it out to
be with my shallow understanding I'm going to trust your judgement.
After all, the leak fix is a strict improvement by itself.
Patrick
In load_cache_entries_threaded(), each thread allocates its own memory pool. This pool needs to be cleaned up while closing the threads down, or it will be leaked. This ce_mem_pool pointer could theoretically be converted to an inline copy of the struct, but the use of a pointer helps with existing lazy- initialization logic. Adjusting that behavior only to avoid this pointer would be a much bigger change. Signed-off-by: Derrick Stolee <stolee@gmail.com>
|
/submit |
|
Submitted as pull.1801.v2.git.1727804265033.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
|
This patch series was integrated into seen via git@38d328a. |
|
On the Git mailing list, Patrick Steinhardt wrote (reply to this): On Tue, Oct 01, 2024 at 05:37:44PM +0000, Derrick Stolee via GitGitGadget wrote:
> From: Derrick Stolee <stolee@gmail.com>
>
> In load_cache_entries_threaded(), each thread allocates its own memory
> pool. This pool needs to be cleaned up while closing the threads down,
> or it will be leaked.
>
> This ce_mem_pool pointer could theoretically be converted to an inline
> copy of the struct, but the use of a pointer helps with existing lazy-
> initialization logic. Adjusting that behavior only to avoid this pointer
> would be a much bigger change.
Thanks, this looks good to me.
Patrick |
|
This branch is now known as |
|
This patch series was integrated into seen via git@4887e21. |
|
This patch series was integrated into next via git@c0eac89. |
|
There was a status update in the "New Topics" section about the branch Leakfix. Will merge to 'master'. source: <pull.1801.v2.git.1727804265033.gitgitgadget@gmail.com> |
|
This patch series was integrated into seen via git@b4efdfe. |
|
This patch series was integrated into master via git@b4efdfe. |
|
This patch series was integrated into next via git@b4efdfe. |
|
Closed via b4efdfe. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This v2 removes the duplicate patch and updates the commit message.
Thanks, -Stolee
cc: gitster@pobox.com
cc: peff@peff.net
cc: ps@pks.im