repository: fix free problem with repo_clear(the_repository)

PATCH Superseded Nguyễn Thái Ngọc Duy 5 participants 11 messages May 9, 2018 – May 10, 2018

Share & tools

Nguyễn Thái Ngọc Duy

author

2018-05-09 17:04 UTC #0 lore raw

PATCH 1/1 repository: fix free problem with repo_clear(the_repository)

the_repository is special. One of the special things about it is that it does not allocate a new index_state object like submodules but points to the global the_index variable instead. As a global variable, the_index cannot be free()'d.

Add an exception for this in repo_clear(). In the future perhaps we would be able to allocate the_repository's index on heap too. Then we can remove revert this.

Signed-off-by Nguyễn Thái Ngọc Duy <[email protected]>

---
I was trying to test the new parsed_object_pool_clear() and found this.

 repository.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

repository.c +3 -1

index a4848c1bd0..f44733524a 100644
@@ -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;
 	}
 }

Signature · 1 line

2.17.0.705.g3525833791

Brandon Williams

2018-05-09 17:17 UTC #1 lore raw

Quoted 5 lines · Nguyễn Thái Ngọc Duy

the_repository is special. One of the special things about it is that it does not allocate a new index_state object like submodules but points to the global the_index variable instead. As a global variable, the_index cannot be free()'d.

Quoted 7 lines

Add an exception for this in repo_clear(). In the future perhaps we would be able to allocate the_repository's index on heap too. Then we can remove revert this.
Signed-off-by Nguyễn Thái Ngọc Duy <[email protected]>
---
I was trying to test the new parsed_object_pool_clear() and found this.

This looks good and I do hope we can get to a state soon where we can not have to special case the_repository.

Quoted 22 lines

 repository.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

repository.c +3 -1

index a4848c1bd0..f44733524a 100644
@@ -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;
 	}
 }

Signature · 1 line

2.17.0.705.g3525833791

Signature · 1 line

Brandon Williams

Elijah Newren

2018-05-09 17:42 UTC #2 lore raw

In reply to Nguyễn Thái Ngọc Duy

Quoted 8 lines · Nguyễn Thái Ngọc Duy

the_repository is special. One of the special things about it is that it does not allocate a new index_state object like submodules but points to the global the_index variable instead. As a global variable, the_index cannot be free()'d.
Add an exception for this in repo_clear(). In the future perhaps we would be able to allocate the_repository's index on heap too. Then we can remove revert this.

"remove revert"?

Duy Nguyen

author

2018-05-09 17:54 UTC #3 lore raw

Quoted 11 lines · Elijah Newren

Quoted 8 lines · Nguyễn Thái Ngọc Duy
the_repository is special. One of the special things about it is that it does not allocate a new index_state object like submodules but points to the global the_index variable instead. As a global variable, the_index cannot be free()'d.
Add an exception for this in repo_clear(). In the future perhaps we would be able to allocate the_repository's index on heap too. Then we can remove revert this.
"remove revert"?

It's obvious that double negatives are below me. I'm going to the next level with double positives! "remove" should be removed.

Signature · 1 line

Duy

Stefan Beller

2018-05-09 17:50 UTC #4 lore raw

In reply to Nguyễn Thái Ngọc Duy

Quoted 4 lines · Nguyễn Thái Ngọc Duy

the_repository is special. One of the special things about it is that it does not allocate a new index_state object like submodules but points to the global the_index variable instead. As a global variable, the_index cannot be free()'d.

ok. That is the situation we're in.

Quoted 8 lines

Add an exception for this in repo_clear(). In the future perhaps we would be able to allocate the_repository's index on heap too. Then we can remove revert this.
Signed-off-by Nguyễn Thái Ngọc Duy <[email protected]>
---
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 ?

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/[email protected]/

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....)

Quoted 8 lines

 repository.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

repository.c +0 -0

index a4848c1bd0..f44733524a 100644

repository.c void repo_clear(struct repository *repo) · line 243

Quoted 7 lines

repository.c +3 -1

@@ -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?

What is your use case of repo_clear(the_repository)?

Thanks,
Stefan

Duy Nguyen

author

2018-05-09 18:01 UTC #5 lore raw

Quoted 3 lines · Stefan Beller

Quoted 1 line
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.

Quoted 6 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.

Quoted 8 lines

[1] https://public-inbox.org/git/[email protected]/
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 :)

Quoted 9 lines

 repository.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

repository.c +0 -0

index a4848c1bd0..f44733524a 100644
@@ -238,7 +238,9 @@ void repo_clear(struct repository *repo)

Quoted 11 lines

repository.c void repo_clear(struct repository *repo) · line 243
Quoted 6 lines
repository.c +3 -1
        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);

Quoted 1 line

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

Signature · 1 line

Duy

Stefan Beller

2018-05-09 18:09 UTC #6 lore raw

Quoted 8 lines · Duy Nguyen

Quoted 3 lines · Stefan Beller
Quoted 1 line
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.

Yes, they do not produce merge conflicts and do not semantically rely on each other. Except as noted below
the object store series complicated things in a non-latest version of it, such that we'd have to add more special casing. Now everything is fine.

Quoted 6 lines

Quoted 3 lines
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 :)

ok, both Brandon and you want to do this, so I guess we'll just go this route for now.

Quoted 10 lines

discard_index(repo->index);
if (repo->index != &the_index)
        FREE_AND_NULL(repo->index);
Quoted 1 line
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.

This diff seems good to me. as any repo is cleared and wil have the minimal amount of memory. the_repository clears its the_index which is also fine as it has the minimal amount of memory then, too

Thanks,
Stefan

Duy Nguyen

author

2018-05-10 06:19 UTC #7 lore raw

In reply to Duy Nguyen

Quoted 9 lines · Duy Nguyen

discard_index(repo->index);
if (repo->index != &the_index)
        FREE_AND_NULL(repo->index);
Quoted 1 line
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.

Another use case for repo_clear(the_repository) is "git worktree move". Part of the reason I did not support moving the main repository with that command is because I wanted to shut down every access to $MAIN_WORK_TREE/.git before moving $MAIN_WORK_TREE. It's probably not a problem for linux/mac platforms to move files (on the same file system [1]) with file descriptors still open, but I'm pretty sure it won't work on Windows. If repo_clear() does its job well, I should be able to safely move $GIT_WORK_TREE after that.

[1] if we move across file systems then another set of problems arise:
if file descriptors remain open, writing to those will not affect the new copies in the target. We do not support moving across filesystems yet, but we should not shut that door now.

Signature · 1 line

Duy

Junio C Hamano

2018-05-10 09:27 UTC #8 lore raw

In reply to Stefan Beller

Quoted 8 lines · Stefan Beller

So this would go with the latest sb/object-store-alloc ?
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.

Quoted 4 lines

[1] https://public-inbox.org/git/[email protected]/
We could just deal with all the exceptions, but that makes repo_clear ugly IMHO.

So perhaps

         void repo_clear(...)
         {
        +       if (repo == the_repository)
        +               BUG("repo_clear() called on the repository");
                ...

or something?

Stefan Beller

2018-05-10 16:34 UTC #9 lore raw

Hi Junio,

Quoted 17 lines · Junio C Hamano

Quoted 8 lines · Stefan Beller
So this would go with the latest sb/object-store-alloc ?
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.
Quoted 4 lines
[1] https://public-inbox.org/git/[email protected]/
We could just deal with all the exceptions, but that makes repo_clear ugly IMHO.
So perhaps

Quoted 7 lines

         void repo_clear(...)
         {
        +       if (repo == the_repository)
        +               BUG("repo_clear() called on the repository");
                ...

or something?

This would work, but Duy convinced me to have repo_clear working on the_repository is a good idea by giving a minimal test helper[1], which helped me to find leaks[2][3], so I'd rather go with the solution by Duy in [4] from a developers perspective.

Stefan

[1] https://public-inbox.org/git/CACsJy8C7N2W821H8YR8VaKdCSOSCDtQi_YT7z8hHNDO-VxJmEA@mail.gmail.com/

    https://gist.github.com/pclouds/86a2df6c28043f1b6fa3d4e72e7a1276

[2] https://public-inbox.org/git/[email protected]/
[3] https://public-inbox.org/git/[email protected]/
[4] https://public-inbox.org/git/CACsJy8AdJcQpiGrR3p6xfuqU0=qoP3=StgbWRNCkdfka6di+5w@mail.gmail.com/

Nguyễn Thái Ngọc Duy

author

2018-05-10 06:13 UTC #10 lore raw

PATCH v2 1/1 repository: fix free problem with repo_clear(the_repository)

In reply to Nguyễn Thái Ngọc Duy

Add an exception for this in repo_clear(). In the future perhaps we would be able to allocate the_repository's index on heap too. Then we can revert this.

the_repository->index remains pointed to a clean the_index even after repo_clear() so that it could still be used next time (e.g. in a crazy use case where a dev switches repo in the same process).

Signed-off-by Nguyễn Thái Ngọc Duy <[email protected]>

---
v2 keeps the_repository->index pointed to the_index even after cleared

 repository.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

repository.c +2 -1

index a4848c1bd0..4143073f89 100644
@@ -238,7 +238,8 @@ void repo_clear(struct repository *repo)
 
 	if (repo->index) {
 		discard_index(repo->index);
-		FREE_AND_NULL(repo->index);
+		if (repo->index != &the_index)
+			FREE_AND_NULL(repo->index);
 	}
 }

Signature · 1 line

2.17.0.705.g3525833791