| Message ID | pull.670.git.git.1574433665.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | On Windows, limit which file handles are inherited by spawned child processes | expand |
"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com> writes: > The problem to be solved: files cannot be deleted on Windows when even one > process has an open file handle to it. So when a process opens a temporary > file, then spawns a child process that inherits that file handle by mistake, > and then the parent process tries to delete the temporary file while the > child process is still running, the deletion will fail. (This description is > slightly simplified, see the commit message "spawned processes need to > inherit only standard handles" for more detail.) Makes me wonder if we should be marking these fds with FD_CLOEXEC to solve the issue in a way that is platform agnostic. It may turn out that we'd be better off to make it an explicit choice by the parent when it leaves a FD open while spawning a child process (and by that spawned child when it runs another executable---but I undertand that it is a single-step operation, not a two-step one, on Windows). In any case, synchronizing the differences in compat/ between our trees is good. Queued.
Hi Junio, On Mon, 25 Nov 2019, Junio C Hamano wrote: > "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com> > writes: > > > The problem to be solved: files cannot be deleted on Windows when even one > > process has an open file handle to it. So when a process opens a temporary > > file, then spawns a child process that inherits that file handle by mistake, > > and then the parent process tries to delete the temporary file while the > > child process is still running, the deletion will fail. (This description is > > slightly simplified, see the commit message "spawned processes need to > > inherit only standard handles" for more detail.) > > Makes me wonder if we should be marking these fds with FD_CLOEXEC to > solve the issue in a way that is platform agnostic. I would like that a lot. Having said that, it is too easy to miss when a patch forgets to do that and when the contributor only tests on Linux. So I really also would like to keep the current patch in addition. Thanks, Dscho > It may turn out that we'd be better off to make it an explicit choice by > the parent when it leaves a FD open while spawning a child process (and > by that spawned child when it runs another executable---but I undertand > that it is a single-step operation, not a two-step one, on Windows). > > In any case, synchronizing the differences in compat/ between our > trees is good. Queued. > >
This is yet another of those patch series that matured in Git for Windows for over a year before being "upstreamed". The problem to be solved: files cannot be deleted on Windows when even one process has an open file handle to it. So when a process opens a temporary file, then spawns a child process that inherits that file handle by mistake, and then the parent process tries to delete the temporary file while the child process is still running, the deletion will fail. (This description is slightly simplified, see the commit message "spawned processes need to inherit only standard handles" for more detail.) Technically, we might want to squash "restrict file handle inheritance only on Windows 7 and later" into "spawned processes need to inherit only standard handles", but while preparing this patch series, I found the story easier to follow with them still being separate. The real reason why I submit this now is that I needed some ready-to-submit patch series as an excuse to test GitGitGadget on https://github.com/git/git . Johannes Schindelin (4): mingw: demonstrate that all file handles are inherited by child processes mingw: work around incorrect standard handles mingw: spawned processes need to inherit only standard handles mingw: restrict file handle inheritance only on Windows 7 and later Documentation/config/core.txt | 6 ++ compat/mingw.c | 140 +++++++++++++++++++++++++++++++--- compat/winansi.c | 12 ++- t/helper/test-run-command.c | 44 +++++++++++ t/t0061-run-command.sh | 4 + 5 files changed, 194 insertions(+), 12 deletions(-) base-commit: d9f6f3b6195a0ca35642561e530798ad1469bd41 Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-670%2Fdscho%2Finherit-only-stdhandles-v1 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-670/dscho/inherit-only-stdhandles-v1 Pull-Request: https://github.com/git/git/pull/670