Make sure temporary file handles are not inherited by child processes #755
Conversation
Two comments:
I also have to admit that I am reluctant to change all
Do you think you can turn that into a patch to, say, |
bwijen
changed the title
|
Did some more testing... Using O_NOINHERIT for all files was indeed to much. Added a test scenario and a fix for the failed test, now only for temporary files. |
|
@dscho: Any comments on my previous attempt? |
|
I was a bit busy trying to set up some automatic testing. It's still too slow (3.5h for any single run) but at least it showed that your branch passes the test suite: https://gitlab.com/git-for-windows1/git/commit/9251e5b7bad7db1f6541d4c2ac87894546ee4867/builds |
| chmod +x ./custom-merge | ||
|
|
||
| test_expect_success 'custom merge backend with delay' ' | ||
| git reset --hard anchor && |
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
|
Interesting approach. I am not really certain that this is safe, but I guess it is correct code. It certainly passes the test suite now which is necessary to give me a little peace of mind. Could you address my concerns with the test script? After that I will gratefully merge this PR. |
This test was added to verify the behaviour of merge when a merge server is spawned. Because file handles are inherited by child processes, the index.lock is also inherited and - on win32 - can't be released until the child has stopped. Signed-off-by: Ben Wijen <[email protected]>
When testing a merge driver which spawns a merge server (for future merges)
I got the following error:
Rename from 'xxx/.git/index.lock' to 'xxx/.git/index' failed. Should I try again? (y/n)
Only after I stop the merge server the lock is released.
This is caused by windows handle inheritance.
Starting childs with bInheritHandles==FALSE does not work,
because no file handles would be inherited,
not even the hStdXxx handles in STARTUPINFO.
Opening every file with O_NOINHERIT does not work,
Since it is used by git-upload-pack for example,
which expects inherited handles.
This leaves us with only creating temp files with the O_NOINHERIT flag.
Which (currently) only used by lock_file which is exactly what we want.
Signed-off-by: Ben Wijen <[email protected]>
|
I changed the test-script as requested. Furthermore I also tested on a (virtual) linux box, which shows no failed tests... |
|
Thank you! |
Child processes [no longer inherit handles to temporary files](git-for-windows/git#755), which previously could prevent `index.lock` from being deleted. Signed-off-by: Johannes Schindelin <[email protected]>
Prevent child processes from inheriting a handle to index.lock
Prevent child processes from inheriting a handle to index.lock
Prevent child processes from inheriting a handle to index.lock
Prevent child processes from inheriting a handle to index.lock
Prevent child processes from inheriting a handle to index.lock
Prevent child processes from inheriting a handle to index.lock
Prevent child processes from inheriting a handle to index.lock
Prevent child processes from inheriting a handle to index.lock
Prevent child processes from inheriting a handle to index.lock
Prevent child processes from inheriting a handle to index.lock
Prevent child processes from inheriting a handle to index.lock
Prevent child processes from inheriting a handle to index.lock
Prevent child processes from inheriting a handle to index.lock
Prevent child processes from inheriting a handle to index.lock
Prevent child processes from inheriting a handle to index.lock
Prevent child processes from inheriting a handle to index.lock
Prevent child processes from inheriting a handle to index.lock
Prevent child processes from inheriting a handle to index.lock
Prevent child processes from inheriting a handle to index.lock
Prevent child processes from inheriting a handle to index.lock
Prevent child processes from inheriting a handle to index.lock
Prevent child processes from inheriting a handle to index.lock
Prevent child processes from inheriting a handle to index.lock
Prevent child processes from inheriting a handle to index.lock
Prevent child processes from inheriting a handle to index.lock
Prevent child processes from inheriting a handle to index.lock
Prevent child processes from inheriting a handle to index.lock
Prevent child processes from inheriting a handle to index.lock
When testing a merge driver which spawns a merge server (for future merges)
I got the following error:
Only after I stop the merge server the lock is released.
This is caused by windows handle inheritance
Starting childs with bInheritHandles==FALSE does not work.
(For some reason the merge commit window did not pop up.)
However creating the handle with bInheritHandle==FALSE does seem to work.
Although I think this should not break anything, it would be great to get some feedback on this.
mcve: