From 89e0b8e7f13a91dec744d2593dcd47e2539f9a67 Mon Sep 17 00:00:00 2001 From: Tom Deseyn Date: Tue, 14 Sep 2021 13:33:17 +0200 Subject: [PATCH 1/6] Process.Unix: handle ETXTBSY error when executing file that was just written by .NET. 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. When this error occurs, we wait for all processes to finish exec-ing, which causes the file to be closed (CLOEXEC). Then we start the process again. --- .../tests/TestUtils/AppHostExtensions.cs | 18 +--- ...ConfigureTerminalForChildProcesses.Unix.cs | 2 +- .../src/System/Diagnostics/Process.Unix.cs | 85 ++++++++++++------- .../tests/ProcessTests.Unix.cs | 37 ++++++++ 4 files changed, 92 insertions(+), 50 deletions(-) diff --git a/src/installer/tests/TestUtils/AppHostExtensions.cs b/src/installer/tests/TestUtils/AppHostExtensions.cs index 50ef2ef22e05a..f056f7586d61a 100644 --- a/src/installer/tests/TestUtils/AppHostExtensions.cs +++ b/src/installer/tests/TestUtils/AppHostExtensions.cs @@ -39,40 +39,26 @@ public static class AppHostExtensions public static void SetWindowsGraphicalUserInterfaceBit(string appHostPath) { - // Make a copy of apphost first, replace hash and overwrite app.exe, rather than - // overwrite app.exe and edit in place, because the file is opened as "write" for - // the replacement -- the test fails with ETXTBSY (exit code: 26) in Linux when - // executing a file opened in "write" mode. - string tempPath = appHostPath + ".tmp"; - File.Copy(appHostPath, tempPath, true); - using (var memoryMappedFile = MemoryMappedFile.CreateFromFile(tempPath)) + using (var memoryMappedFile = MemoryMappedFile.CreateFromFile(appHostPath)) { using (MemoryMappedViewAccessor accessor = memoryMappedFile.CreateViewAccessor()) { SetWindowsGraphicalUserInterfaceBit(accessor); } } - File.Move(tempPath, appHostPath, true); } public static void BindAppHost(string appHostPath) { string appDll = $"{Path.GetFileNameWithoutExtension(appHostPath)}.dll"; - // Make a copy of apphost first, replace hash and overwrite app.exe, rather than - // overwrite app.exe and edit in place, because the file is opened as "write" for - // the replacement -- the test fails with ETXTBSY (exit code: 26) in Linux when - // executing a file opened in "write" mode. - string tempPath = appHostPath + ".tmp"; - File.Copy(appHostPath, tempPath, true); using (var sha256 = SHA256.Create()) { // Replace the hash with the managed DLL name. var hash = sha256.ComputeHash(Encoding.UTF8.GetBytes("foobar")); var hashStr = BitConverter.ToString(hash).Replace("-", "").ToLower(); - BinaryUtils.SearchAndReplace(tempPath, Encoding.UTF8.GetBytes(hashStr), Encoding.UTF8.GetBytes(appDll)); + BinaryUtils.SearchAndReplace(appHostPath, Encoding.UTF8.GetBytes(hashStr), Encoding.UTF8.GetBytes(appDll)); } - File.Move(tempPath, appHostPath, true); } /// 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..5218e288a7251 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,65 @@ 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); + if (!throwOnNoExec && errorInfo.Error == Interop.Error.ENOEXEC) + { + return false; + } - throw CreateExceptionForErrorStartingProcess(new Interop.ErrorInfo(errno).GetErrorMessage(), errno, resolvedFilename, cwd); - } + 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 + // which causes the file to be closed (CLOEXEC). + 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..4c988a45ec595 100644 --- a/src/libraries/System.Diagnostics.Process/tests/ProcessTests.Unix.cs +++ b/src/libraries/System.Diagnostics.Process/tests/ProcessTests.Unix.cs @@ -525,6 +525,43 @@ public void TestStartOnUnixWithBadFormat() Assert.NotEqual(0, e.NativeErrorCode); } + [Fact] + public void TestConcurrentExecuteAfterWrite() + { + // Linux fails to execute a process with 'ETXTBSY' when the file is still opened for writing. + // Even though we closed the file in the .NET application, it may still remain open + // for a short time because other processes that are started have forked, but not yet exec-ed. + // .NET specifically supports this case by waiting till all pending forks exec-ed, and then + // retrying to start the process. + + var threads = new Thread[20]; + for (int i = 0; i < threads.Length; i++) + { + string directory = GetTestFileName(i); + Directory.CreateDirectory(directory); + + threads[i] = new Thread(WriteAndExecute) { IsBackground = true }; + threads[i].Start(directory); + } + + for (int i = 0; i < threads.Length; i++) + { + threads[i].Join(); + } + + void WriteAndExecute(object o) + { + var directory = o as string; + for (int i = 0; i < 100; i++) + { + WriteScriptFile(directory, "script", 42); + using var process = Process.Start(Path.Combine(directory, "script")); + process.WaitForExit(); + Assert.Equal(42, process.ExitCode); + } + } + } + [ConditionalFact(typeof(RemoteExecutor), nameof(RemoteExecutor.IsSupported))] public void TestStartWithNonExistingUserThrows() { From 68a9293a5534748b6b6a3da642605225f67d0507 Mon Sep 17 00:00:00 2001 From: Tom Deseyn Date: Tue, 14 Sep 2021 16:18:47 +0200 Subject: [PATCH 2/6] Fix compilation --- .../src/System/Diagnostics/Process.Unix.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 5218e288a7251..24b16b0146035 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 @@ -557,7 +557,7 @@ private bool ForkAndExecProcess( 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 + // 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 // which causes the file to be closed (CLOEXEC). From dbcf8ea7cb1caea616b9386eb2283a29ac15c8f8 Mon Sep 17 00:00:00 2001 From: Tom Deseyn Date: Wed, 15 Sep 2021 08:51:47 +0200 Subject: [PATCH 3/6] Refactor test --- .../src/System/Diagnostics/Process.Unix.cs | 1 + .../tests/ProcessTests.Unix.cs | 16 ++++++---------- 2 files changed, 7 insertions(+), 10 deletions(-) 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 24b16b0146035..fa9fa4fe19c7a 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 @@ -547,6 +547,7 @@ private bool ForkAndExecProcess( else { Interop.ErrorInfo errorInfo = new Interop.ErrorInfo(errno); + if (!throwOnNoExec && errorInfo.Error == Interop.Error.ENOEXEC) { return false; diff --git a/src/libraries/System.Diagnostics.Process/tests/ProcessTests.Unix.cs b/src/libraries/System.Diagnostics.Process/tests/ProcessTests.Unix.cs index 4c988a45ec595..cfe1755f26290 100644 --- a/src/libraries/System.Diagnostics.Process/tests/ProcessTests.Unix.cs +++ b/src/libraries/System.Diagnostics.Process/tests/ProcessTests.Unix.cs @@ -534,22 +534,18 @@ public void TestConcurrentExecuteAfterWrite() // .NET specifically supports this case by waiting till all pending forks exec-ed, and then // retrying to start the process. - var threads = new Thread[20]; - for (int i = 0; i < threads.Length; i++) + var tasks = new Task[20]; + for (int i = 0; i < tasks.Length; i++) { string directory = GetTestFileName(i); Directory.CreateDirectory(directory); - threads[i] = new Thread(WriteAndExecute) { IsBackground = true }; - threads[i].Start(directory); + tasks[i] = Task.Factory.StartNew(WriteAndExecute, directory, TaskCreationOptions.LongRunning); } - for (int i = 0; i < threads.Length; i++) - { - threads[i].Join(); - } + Task.WaitAll(tasks); - void WriteAndExecute(object o) + static void WriteAndExecute(object o) { var directory = o as string; for (int i = 0; i < 100; i++) @@ -1033,7 +1029,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"); From 93b5d772712ccb83e5fb8aa06c6adf09b98e398d Mon Sep 17 00:00:00 2001 From: Tom Deseyn Date: Thu, 16 Sep 2021 13:29:42 +0200 Subject: [PATCH 4/6] Update tests --- .../src/System/Diagnostics/Process.Unix.cs | 6 ++-- .../tests/ProcessTests.Unix.cs | 35 +++++-------------- 2 files changed, 12 insertions(+), 29 deletions(-) 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 fa9fa4fe19c7a..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 @@ -560,8 +560,10 @@ private bool ForkAndExecProcess( // 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 - // which causes the file to be closed (CLOEXEC). + // 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(); diff --git a/src/libraries/System.Diagnostics.Process/tests/ProcessTests.Unix.cs b/src/libraries/System.Diagnostics.Process/tests/ProcessTests.Unix.cs index cfe1755f26290..85cc7cecb415e 100644 --- a/src/libraries/System.Diagnostics.Process/tests/ProcessTests.Unix.cs +++ b/src/libraries/System.Diagnostics.Process/tests/ProcessTests.Unix.cs @@ -526,36 +526,17 @@ public void TestStartOnUnixWithBadFormat() } [Fact] - public void TestConcurrentExecuteAfterWrite() + public async Task ExecutingFileThatIsOpenForWritingDoesntHang() { - // Linux fails to execute a process with 'ETXTBSY' when the file is still opened for writing. - // Even though we closed the file in the .NET application, it may still remain open - // for a short time because other processes that are started have forked, but not yet exec-ed. - // .NET specifically supports this case by waiting till all pending forks exec-ed, and then - // retrying to start the process. - - var tasks = new Task[20]; - for (int i = 0; i < tasks.Length; i++) - { - string directory = GetTestFileName(i); - Directory.CreateDirectory(directory); - - tasks[i] = Task.Factory.StartNew(WriteAndExecute, directory, TaskCreationOptions.LongRunning); - } + // 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. - Task.WaitAll(tasks); + string scriptFile = WriteScriptFile(TestDirectory, GetTestFileName(), 42); + using var handle = File.OpenHandle(scriptFile, FileMode.Open, FileAccess.Write); - static void WriteAndExecute(object o) - { - var directory = o as string; - for (int i = 0; i < 100; i++) - { - WriteScriptFile(directory, "script", 42); - using var process = Process.Start(Path.Combine(directory, "script")); - process.WaitForExit(); - Assert.Equal(42, process.ExitCode); - } - } + await Assert.ThrowsAsync(() => + Task.Run(() => Process.Start(scriptFile)).WaitAsync(TimeSpan.FromSeconds(30))); } [ConditionalFact(typeof(RemoteExecutor), nameof(RemoteExecutor.IsSupported))] From 9e228f02419fdb70fb432145a33cca8deb8a0c3f Mon Sep 17 00:00:00 2001 From: Tom Deseyn Date: Thu, 16 Sep 2021 13:30:53 +0200 Subject: [PATCH 5/6] Revert changes to AppHostExtensions --- .../tests/TestUtils/AppHostExtensions.cs | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) diff --git a/src/installer/tests/TestUtils/AppHostExtensions.cs b/src/installer/tests/TestUtils/AppHostExtensions.cs index f056f7586d61a..50ef2ef22e05a 100644 --- a/src/installer/tests/TestUtils/AppHostExtensions.cs +++ b/src/installer/tests/TestUtils/AppHostExtensions.cs @@ -39,26 +39,40 @@ public static class AppHostExtensions public static void SetWindowsGraphicalUserInterfaceBit(string appHostPath) { - using (var memoryMappedFile = MemoryMappedFile.CreateFromFile(appHostPath)) + // Make a copy of apphost first, replace hash and overwrite app.exe, rather than + // overwrite app.exe and edit in place, because the file is opened as "write" for + // the replacement -- the test fails with ETXTBSY (exit code: 26) in Linux when + // executing a file opened in "write" mode. + string tempPath = appHostPath + ".tmp"; + File.Copy(appHostPath, tempPath, true); + using (var memoryMappedFile = MemoryMappedFile.CreateFromFile(tempPath)) { using (MemoryMappedViewAccessor accessor = memoryMappedFile.CreateViewAccessor()) { SetWindowsGraphicalUserInterfaceBit(accessor); } } + File.Move(tempPath, appHostPath, true); } public static void BindAppHost(string appHostPath) { string appDll = $"{Path.GetFileNameWithoutExtension(appHostPath)}.dll"; + // Make a copy of apphost first, replace hash and overwrite app.exe, rather than + // overwrite app.exe and edit in place, because the file is opened as "write" for + // the replacement -- the test fails with ETXTBSY (exit code: 26) in Linux when + // executing a file opened in "write" mode. + string tempPath = appHostPath + ".tmp"; + File.Copy(appHostPath, tempPath, true); using (var sha256 = SHA256.Create()) { // Replace the hash with the managed DLL name. var hash = sha256.ComputeHash(Encoding.UTF8.GetBytes("foobar")); var hashStr = BitConverter.ToString(hash).Replace("-", "").ToLower(); - BinaryUtils.SearchAndReplace(appHostPath, Encoding.UTF8.GetBytes(hashStr), Encoding.UTF8.GetBytes(appDll)); + BinaryUtils.SearchAndReplace(tempPath, Encoding.UTF8.GetBytes(hashStr), Encoding.UTF8.GetBytes(appDll)); } + File.Move(tempPath, appHostPath, true); } /// From 1f55b77451b585ea85ad986a4fc723afa327024b Mon Sep 17 00:00:00 2001 From: Tom Deseyn Date: Thu, 16 Sep 2021 15:34:46 +0200 Subject: [PATCH 6/6] Mark test as Linux specific. --- .../System.Diagnostics.Process/tests/ProcessTests.Unix.cs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/libraries/System.Diagnostics.Process/tests/ProcessTests.Unix.cs b/src/libraries/System.Diagnostics.Process/tests/ProcessTests.Unix.cs index 85cc7cecb415e..0b0ee0045a134 100644 --- a/src/libraries/System.Diagnostics.Process/tests/ProcessTests.Unix.cs +++ b/src/libraries/System.Diagnostics.Process/tests/ProcessTests.Unix.cs @@ -526,9 +526,10 @@ public void TestStartOnUnixWithBadFormat() } [Fact] + [PlatformSpecific(TestPlatforms.Linux)] // OSX doesn't fail when executing a file that is open for writing. public async Task ExecutingFileThatIsOpenForWritingDoesntHang() { - // A process fails to execute with 'ETXTBSY' when the executable is open for writing. + // 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.