diff: release all handles before running external diff
cover diff: release all handles before running external diff
On Windows, it is not possible to overwrite a file as long as any process holds a read handle to it. Even keeping regions memory-mapped prevents that.
When git difftool calls git diff, it might be the user's intention to write the file(s) via the diff tool, so let's make sure that they are not memory-mapped at that stage.
Johannes Schindelin (1):
diff: munmap() file contents before running external diff
diff.c | 4 ++++ 1 file changed, 4 insertions(+)
base-commit: aa25c82427ae70aebf3b8f970f2afd54e9a2a8c6
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-213%2Fdscho%2Fmunmap-before-ext-diff-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-213/dscho/munmap-before-ext-diff-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/213
Signature · 1 line
gitgitgadget
PATCH 1/1 diff: munmap() file contents before running external diff
From: Johannes Schindelin <[email protected]>
When running an external diff from, say, a diff tool, it is safe to assume that we want to write the files in question. On Windows, that means that there cannot be any other process holding an open handle to said files.
So let's make sure that git diff itself is not holding any open handle to the files in question.
This fixes https://github.com/git-for-windows/git/issues/1315
--- diff.c | 4 ++++ 1 file changed, 4 insertions(+)
diff.c +4 -0
@@ -4206,6 +4206,10 @@ static void run_external_diff(const char *pgm,argv_array_pushf (&env,"GIT_DIFF_PATH_COUNTER=%d" , ++o->diff_path_counter);argv_array_pushf (&env,"GIT_DIFF_PATH_TOTAL=%d" , q->nr); +if (one && one->should_munmap) +diff_free_filespec_data (one); +if (two && two->should_munmap) +diff_free_filespec_data (two);if (run_command_v_opt_cd_env (argv.argv,RUN_USING_SHELL ,NULL , env.argv))die (_ ("external diff died, stopping at %s" ), name);
Signature · 1 line
gitgitgadget
"Johannes Schindelin via GitGitGadget" <[email protected]>
Quoted 2 lines · writes:
On Windows, it is not possible to overwrite a file as long as any process holds a read handle to it. Even keeping regions memory-mapped prevents that.
That second sentence was quite helpful, as I do recall us closing after mmapping. Without it, the justification becomes quite weak.
Re: [PATCH 1/1] diff: munmap() file contents before running external diff
In reply to Johannes Schindelin via GitGitGadget"Johannes Schindelin via GitGitGadget" <[email protected]>
Quoted 6 lines · writes:
From: Johannes Schindelin <[email protected]>
When running an external diff from, say, a diff tool, it is safe to assume that we want to write the files in question. On Windows, that means that there cannot be any other process holding an open handle to said files.
Please add "It is not enough to close the file descriptor; having a region that is still mmapped keeps the file busy" or something like that at the end.
Quoted 15 lines
So let's make sure that
git diffitself is not holding any open handle to the files in question.This fixes https://github.com/git-for-windows/git/issues/1315
Signed-off-by Johannes Schindelin <[email protected]>--- diff.c | 4 ++++ 1 file changed, 4 insertions(+)diff.c +0
-0
diff.c static void run_external_diff(const char *pgm, · line 4212
Quoted 7 lines
diff.c +4
-0@@ -4206,6 +4206,10 @@ static void run_external_diff(const char *pgm,argv_array_pushf (&env,"GIT_DIFF_PATH_COUNTER=%d" , ++o->diff_path_counter);argv_array_pushf (&env,"GIT_DIFF_PATH_TOTAL=%d" , q->nr); +if (one && one->should_munmap) +diff_free_filespec_data (one); +if (two && two->should_munmap) +diff_free_filespec_data (two);
I wondered if a single diff_filespec instance can be used in two diff_filepair instances (e.g. file A is in-place modified and also used to create file C), and if so after showing the diff for file A, we have problems with showing file C. But I do not think it should pose a problem, as "free data after comparing a pair" is what we do for the in-core codepath in builtin_diff().
We can lose the NULL-ness test for one and two if these "free the resource once we no longer need it" is done inside "if (one && two)". After all, once add_external_diff_name()[*1*] does its thing, we do not need the data for these diff_filespec instances, right?
Also, just like builtin_diff() unconditionally frees the resources held by diff_filespec instances, shouldn't this function do so, even the ones that are not marked with should_munmap?
Quoted 2 lines
diff.c +0
-0if (run_command_v_opt_cd_env (argv.argv,RUN_USING_SHELL ,NULL , env.argv))die (_ ("external diff died, stopping at %s" ), name);
Re: [PATCH 1/1] diff: munmap() file contents before running external diff
Hi Junio,
Quoted 5 lines · Junio C Hamano
"Johannes Schindelin via GitGitGadget" <[email protected]>
Quoted 2 lines · writes:
From: Johannes Schindelin <[email protected]>
Quoted 8 lines
Quoted 4 lines
When running an external diff from, say, a diff tool, it is safe to assume that we want to write the files in question. On Windows, that means that there cannot be any other process holding an open handle to said files.
Please add "It is not enough to close the file descriptor; having a region that is still mmapped keeps the file busy" or something like that at the end.
Good call.
Quoted 18 lines
Quoted 18 lines
So let's make sure that
git diffitself is not holding any open handle to the files in question.This fixes https://github.com/git-for-windows/git/issues/1315
Signed-off-by Johannes Schindelin <[email protected]>--- diff.c | 4 ++++ 1 file changed, 4 insertions(+)diff.c +0
-0@@ -4206,6 +4206,10 @@ static void run_external_diff(const char *pgm,argv_array_pushf (&env,"GIT_DIFF_PATH_COUNTER=%d" , ++o->diff_path_counter);argv_array_pushf (&env,"GIT_DIFF_PATH_TOTAL=%d" , q->nr);
Quoted 11 lines
diff.c static void run_external_diff(const char *pgm, · line 4212
Quoted 4 lines
diff.c +4
-0+if (one && one->should_munmap) +diff_free_filespec_data (one); +if (two && two->should_munmap) +diff_free_filespec_data (two);I wondered if a single
diff_filespecinstance can be used in twodiff_filepairinstances (e.g. file A is in-place modified and also used to create file C), and if so after showing the diff for file A, we have problems with showing file C. But I do not think it should pose a problem, as "free data after comparing a pair" is what we do for the in-core codepath inbuiltin_diff().
Precisely.
Quoted 4 lines
We can lose the NULL-ness test for one and two if these "free the resource once we no longer need it" is done inside "if (one && two)". After all, once add_external_diff_name()[*1*] does its thing, we do not need the data for these
diff_filespecinstances, right?
Yes, but we still need the should_munmap test, I believe. So we have that if anyway.
Quoted 3 lines
Also, just like
builtin_diff()unconditionally frees the resources held bydiff_filespecinstances, shouldn't this function do so, even the ones that are not marked withshould_munmap?
I have not inspected the code path vigorously, nor am I confident that this wouldn't be broken easily. At least when regions are mapped, I am fairly certain that my patch does not break anything.
But if you are confident that this won't break anything, I'll certainly be happy with that assessment.
I'll make that change and trust the CI build to fail if your assumption was incorrect.
Ciao,
Dscho
Quoted 5 lines
Quoted 4 lines
diff.c +0
-0if (run_command_v_opt_cd_env (argv.argv,RUN_USING_SHELL ,NULL , env.argv))die (_ ("external diff died, stopping at %s" ), name);
v2 cover diff: release all handles before running external diff
In reply to Johannes Schindelin via GitGitGadgetOn Windows, it is not possible to overwrite a file as long as any process holds a read handle to it. Even keeping regions memory-mapped prevents that.
When git difftool calls git diff, it might be the user's intention to write the file(s) via the diff tool, so let's make sure that they are not memory-mapped at that stage.
Changes since v1:
* Clarified in the commit message that even mapped regions block writes/deletes.
* The diff file pair is now released unconditionally, not only when it is mapped, for consistency (the CI build did not fail, and a cursory inspection of the code paths indicates that this should be safe, as from this point on only the external command accesses the file pair's contents, and they had to be written out to disk to that end).
Johannes Schindelin (1):
diff: munmap() file contents before running external diff
diff.c | 2 ++ 1 file changed, 2 insertions(+)
base-commit: aa25c82427ae70aebf3b8f970f2afd54e9a2a8c6
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-213%2Fdscho%2Fmunmap-before-ext-diff-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-213/dscho/munmap-before-ext-diff-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/213
Range-diff vs v1:
1: bef83fc20b ! 1: 8a0213291b diff: munmap() file contents before running external diff
@@ -5,11 +5,15 @@
When running an external diff from, say, a diff tool, it is safe to assume that we want to write the files in question. On Windows, that means that there cannot be any other process holding an open handle to
- said files.
+ said files, or even just a mapped region.
So let's make sure that git diff itself is not holding any open handle to the files in question.
+ In fact, we will just release the file pair right away, as the external
+ diff uses the files we just wrote, so we do not need to hold the file
+ contents in memory anymore.
+
This fixes https://github.com/git-for-windows/git/issues/1315
@@ -21,10 +25,8 @@
argv_array_pushf(&env, "GIT_DIFF_PATH_COUNTER=%d", ++o->diff_path_counter); argv_array_pushf(&env, "GIT_DIFF_PATH_TOTAL=%d", q->nr);
-+ if (one && one->should_munmap)
-+ diff_free_filespec_data(one);
-+ if (two && two->should_munmap)
-+ diff_free_filespec_data(two);
++ diff_free_filespec_data(one);
++ diff_free_filespec_data(two);
if (run_command_v_opt_cd_env(argv.argv, RUN_USING_SHELL, NULL, env.argv))
die(_("external diff died, stopping at %s"), name);
Signature · 1 line
gitgitgadget
PATCH v2 1/1 diff: munmap() file contents before running external diff
From: Johannes Schindelin <[email protected]>
When running an external diff from, say, a diff tool, it is safe to assume that we want to write the files in question. On Windows, that means that there cannot be any other process holding an open handle to said files, or even just a mapped region.
So let's make sure that git diff itself is not holding any open handle to the files in question.
In fact, we will just release the file pair right away, as the external diff uses the files we just wrote, so we do not need to hold the file contents in memory anymore.
This fixes https://github.com/git-for-windows/git/issues/1315
--- diff.c | 2 ++ 1 file changed, 2 insertions(+)
diff.c +2 -0
@@ -4206,6 +4206,8 @@ static void run_external_diff(const char *pgm,argv_array_pushf (&env,"GIT_DIFF_PATH_COUNTER=%d" , ++o->diff_path_counter);argv_array_pushf (&env,"GIT_DIFF_PATH_TOTAL=%d" , q->nr); +diff_free_filespec_data (one); +diff_free_filespec_data (two);if (run_command_v_opt_cd_env (argv.argv,RUN_USING_SHELL ,NULL , env.argv))die (_ ("external diff died, stopping at %s" ), name);
Signature · 1 line
gitgitgadget
Re: [PATCH v2 1/1] diff: munmap() file contents before running external diff
"Johannes Schindelin via GitGitGadget" <[email protected]>
Quoted 26 lines · writes:
From: Johannes Schindelin <[email protected]>
When running an external diff from, say, a diff tool, it is safe to assume that we want to write the files in question. On Windows, that means that there cannot be any other process holding an open handle to said files, or even just a mapped region.
So let's make sure that
git diffitself is not holding any open handle to the files in question.In fact, we will just release the file pair right away, as the external diff uses the files we just wrote, so we do not need to hold the file contents in memory anymore.
This fixes https://github.com/git-for-windows/git/issues/1315
Signed-off-by Johannes Schindelin <[email protected]>--- diff.c | 2 ++ 1 file changed, 2 insertions(+)diff.c +0
-0
diff.c static void run_external_diff(const char *pgm, · line 4212
Quoted 7 lines
diff.c +2
-0@@ -4206,6 +4206,8 @@ static void run_external_diff(const char *pgm,argv_array_pushf (&env,"GIT_DIFF_PATH_COUNTER=%d" , ++o->diff_path_counter);argv_array_pushf (&env,"GIT_DIFF_PATH_TOTAL=%d" , q->nr); +diff_free_filespec_data (one); +diff_free_filespec_data (two);if (run_command_v_opt_cd_env (argv.argv,RUN_USING_SHELL ,NULL , env.argv))die (_ ("external diff died, stopping at %s" ), name);
Looks sensible; will queue. Thanks.