Re: [PATCH] repository: fix free problem with repo_clear(the_repository)

From: Duy Nguyen <pclouds@gmail.com>
Date: May 9, 2018, 18:00 UTC
Message-ID: <CACsJy8AdJcQpiGrR3p6xfuqU0=qoP3=StgbWRNCkdfka6di+5w@mail.gmail.com>
In-Reply-To: <CAGZ79kbFiULj1NJARm6ObYrqv_Fu+U2sb8h_sNJwdWur+JqrvQ@mail.gmail.com>

On Wed, May 9, 2018 at 7:50 PM, Stefan Beller <sbeller@google.com> wrote:

>>  I was trying to test the new parsed_object_pool_clear() and found this.
>
> So this would go with the latest sb/object-store-alloc ?

No this should be separate because sb/object-store-alloc did not even touch this code. I mistakenly thought you wrote this and sent to you. I should have checked and sent to Brandon instead.

Show 14 quoted lines

> My impression was that we never call repo_clear() on
> the_repository, which would allow us to special case
> the_repository further just as I did in v2 of that series[1] by
> having static allocations for certain objects in case of \
> the_repository.
>
> [1] https://public-inbox.org/git/20180501213403.14643-14-sbeller@google.com/
>
> We could just deal with all the exceptions, but that makes repo_clear
> ugly IMHO.
>
> I would rather special case the_repository even more instead
> of having it allocate all its things on the heap. (However we rather
> want to profile it and argue with data....)

I'm actually going the opposite direction and trying hard to make the_repository normal like everybody else :)

Show 20 quoted lines

>>  repository.c | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/repository.c b/repository.c
>> index a4848c1bd0..f44733524a 100644
>> --- a/repository.c
>> +++ b/repository.c
>> @@ -238,7 +238,9 @@ void repo_clear(struct repository *repo)
>>
>>         if (repo->index) {
>>                 discard_index(repo->index);
>> -               FREE_AND_NULL(repo->index);
>> +               if (repo->index != &the_index)
>> +                       free(repo->index);
>> +               repo->index = NULL;
>
> So after this we have a "dangling" the_index.
> It is not really dangling, but it is not part of the_repository any more
> and many places still use the_index, it might make up for interesting
> bugs?

Hmm.. yeah, as a "clearing" operation, I probaly should not clear repo->index. This way the_repository will be back in initial state and could be reused again. Something like this perhaps?

discard_index(repo->index);
if (repo->index != &the_index)
        FREE_AND_NULL(repo->index);

> What is your use case of repo_clear(the_repository)?

No actual use case right now. See [1] for the code that triggered this. I do want to free some memory in pack-objects and repo_clear() _might_ be the one. I'm not sure yet.

[1] https://gist.github.com/pclouds/86a2df6c28043f1b6fa3d4e72e7a1276

-- 
Duy