diff --git a/src/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.ConfigureTerminalForChildProcesses.Unix.cs b/src/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.ConfigureTerminalForChildProcesses.Unix.cs index b51957fb881bf..bcfa7ffa36ae0 100644 --- a/src/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.ConfigureTerminalForChildProcesses.Unix.cs +++ b/src/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.ConfigureTerminalForChildProcesses.Unix.cs @@ -17,7 +17,7 @@ internal static void ConfigureTerminalForChildProcesses(int increment, bool conf int childrenUsingTerminalRemaining = Interlocked.Add(ref s_childrenUsingTerminalCount, increment); if (increment > 0) { - Debug.Assert(s_processStartLock.IsReadLockHeld); + Debug.Assert(s_processStartLock.IsUpgradeableReadLockHeld); Debug.Assert(configureConsole); // At least one child is using the terminal. diff --git a/src/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.Unix.cs b/src/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.Unix.cs index 69b4481ae2087..b34c91d04d860 100644 --- a/src/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.Unix.cs +++ b/src/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.Unix.cs @@ -507,7 +507,7 @@ private bool ForkAndExecProcess( // Lock to avoid races with OnSigChild // By using a ReaderWriterLock we allow multiple processes to start concurrently. - s_processStartLock.EnterReadLock(); + s_processStartLock.EnterUpgradeableReadLock(); try { if (usesTerminal) @@ -515,46 +515,68 @@ private bool ForkAndExecProcess( ConfigureTerminalForChildProcesses(1); } - int childPid; - - // Invoke the shim fork/execve routine. It will create pipes for all requested - // redirects, fork a child process, map the pipe ends onto the appropriate stdin/stdout/stderr - // descriptors, and execve to execute the requested process. The shim implementation - // is used to fork/execve as executing managed code in a forked process is not safe (only - // the calling thread will transfer, thread IDs aren't stable across the fork, etc.) - int errno = Interop.Sys.ForkAndExecProcess( - resolvedFilename, argv, envp, cwd, - startInfo.RedirectStandardInput, startInfo.RedirectStandardOutput, startInfo.RedirectStandardError, - setCredentials, userId, groupId, groups, - out childPid, out stdinFd, out stdoutFd, out stderrFd); - - if (errno == 0) + bool retryOnTxtBsy = true; + do { - // Ensure we'll reap this process. - // note: SetProcessId will set this if we don't set it first. - _waitStateHolder = new ProcessWaitState.Holder(childPid, isNewChild: true, usesTerminal); - - // Store the child's information into this Process object. - Debug.Assert(childPid >= 0); - SetProcessId(childPid); - SetProcessHandle(new SafeProcessHandle(_processId, GetSafeWaitHandle())); + int childPid; + + // Invoke the shim fork/execve routine. It will create pipes for all requested + // redirects, fork a child process, map the pipe ends onto the appropriate stdin/stdout/stderr + // descriptors, and execve to execute the requested process. The shim implementation + // is used to fork/execve as executing managed code in a forked process is not safe (only + // the calling thread will transfer, thread IDs aren't stable across the fork, etc.) + int errno = Interop.Sys.ForkAndExecProcess( + resolvedFilename, argv, envp, cwd, + startInfo.RedirectStandardInput, startInfo.RedirectStandardOutput, startInfo.RedirectStandardError, + setCredentials, userId, groupId, groups, + out childPid, out stdinFd, out stdoutFd, out stderrFd); - return true; - } - else - { - if (!throwOnNoExec && - new Interop.ErrorInfo(errno).Error == Interop.Error.ENOEXEC) + if (errno == 0) { - return false; + // Ensure we'll reap this process. + // note: SetProcessId will set this if we don't set it first. + _waitStateHolder = new ProcessWaitState.Holder(childPid, isNewChild: true, usesTerminal); + + // Store the child's information into this Process object. + Debug.Assert(childPid >= 0); + SetProcessId(childPid); + SetProcessHandle(new SafeProcessHandle(_processId, GetSafeWaitHandle())); + + return true; } + else + { + Interop.ErrorInfo errorInfo = new Interop.ErrorInfo(errno); - throw CreateExceptionForErrorStartingProcess(new Interop.ErrorInfo(errno).GetErrorMessage(), errno, resolvedFilename, cwd); - } + if (!throwOnNoExec && errorInfo.Error == Interop.Error.ENOEXEC) + { + return false; + } + + if (retryOnTxtBsy && errorInfo.Error == Interop.Error.ETXTBSY) + { + retryOnTxtBsy = false; + + // ETXTBSY means we're trying to execute a file that is open for write. + // This may be a file that was just written and closed, but is still referenced + // by another process that has forked but not yet exec-ed. + // Using the s_processStartLock we wait for all processes to finish exec-ing. + // 'ForkAndExecProcess' returns when the a pipe gets closed by exec. + // Unfortunately, the handle we care about may be closed after that. So this + // doesn't work all the time. + s_processStartLock.EnterWriteLock(); + s_processStartLock.ExitWriteLock(); + + continue; + } + + throw CreateExceptionForErrorStartingProcess(errorInfo.GetErrorMessage(), errno, resolvedFilename, cwd); + } + } while (true); } finally { - s_processStartLock.ExitReadLock(); + s_processStartLock.ExitUpgradeableReadLock(); if (_waitStateHolder == null && usesTerminal) { diff --git a/src/libraries/System.Diagnostics.Process/tests/ProcessTests.Unix.cs b/src/libraries/System.Diagnostics.Process/tests/ProcessTests.Unix.cs index 63d7f3d6d02b3..0b0ee0045a134 100644 --- a/src/libraries/System.Diagnostics.Process/tests/ProcessTests.Unix.cs +++ b/src/libraries/System.Diagnostics.Process/tests/ProcessTests.Unix.cs @@ -525,6 +525,21 @@ public void TestStartOnUnixWithBadFormat() Assert.NotEqual(0, e.NativeErrorCode); } + [Fact] + [PlatformSpecific(TestPlatforms.Linux)] // OSX doesn't fail when executing a file that is open for writing. + public async Task ExecutingFileThatIsOpenForWritingDoesntHang() + { + // On Linux, a process fails to execute with 'ETXTBSY' when the executable is open for writing. + // Process.Unix has some special handling for that. + // This test verifies it doesn't cause 'Process.Start' to hang indefinitely when the handle remains open. + + string scriptFile = WriteScriptFile(TestDirectory, GetTestFileName(), 42); + using var handle = File.OpenHandle(scriptFile, FileMode.Open, FileAccess.Write); + + await Assert.ThrowsAsync(() => + Task.Run(() => Process.Start(scriptFile)).WaitAsync(TimeSpan.FromSeconds(30))); + } + [ConditionalFact(typeof(RemoteExecutor), nameof(RemoteExecutor.IsSupported))] public void TestStartWithNonExistingUserThrows() { @@ -996,7 +1011,7 @@ private static unsafe HashSet GetGroups() private static readonly string[] s_allowedProgramsToRun = new string[] { "xdg-open", "gnome-open", "kfmclient" }; - private string WriteScriptFile(string directory, string name, int returnValue) + private static string WriteScriptFile(string directory, string name, int returnValue) { string filename = Path.Combine(directory, name); File.WriteAllText(filename, $"#!/bin/sh\nexit {returnValue}\n");