From 261e5b74cc51b93692a138b23effdb2987444ad5 Mon Sep 17 00:00:00 2001 From: Jordan Borean Date: Sat, 15 Apr 2023 07:49:13 +1000 Subject: Ansible.Basic - Improve temporary file cleanup process (#80293) (#80325) * Ansible.Basic - Improve temporary file cleanup process * Add comment on struct value used (cherry picked from commit ba4505f5cb2fb52cda450a06679ddea3599e3e70) --- changelogs/fragments/win-temp-cleanup.yml | 8 + lib/ansible/module_utils/csharp/Ansible.Basic.cs | 269 ++++++++++++++++++++- .../library/ansible_basic_tests.ps1 | 162 +++++++++++++ 3 files changed, 433 insertions(+), 6 deletions(-) create mode 100644 changelogs/fragments/win-temp-cleanup.yml diff --git a/changelogs/fragments/win-temp-cleanup.yml b/changelogs/fragments/win-temp-cleanup.yml new file mode 100644 index 0000000000..4283f2a1b6 --- /dev/null +++ b/changelogs/fragments/win-temp-cleanup.yml @@ -0,0 +1,8 @@ +bugfixes: +- >- + Windows - Improve temporary file cleanup used by modules. Will use a more reliable delete operation on Windows + Server 2016 and newer to delete files that might still be open by other software like Anti Virus scanners. There are + still scenarios where a file or directory cannot be deleted but the new method should work in more scenarios. +- >- + Windows - Display a warning if the module failed to cleanup any temporary files rather than failing the task. The + warning contains a brief description of what failed to be deleted. diff --git a/lib/ansible/module_utils/csharp/Ansible.Basic.cs b/lib/ansible/module_utils/csharp/Ansible.Basic.cs index 16480992e8..97f5f3e2d7 100644 --- a/lib/ansible/module_utils/csharp/Ansible.Basic.cs +++ b/lib/ansible/module_utils/csharp/Ansible.Basic.cs @@ -1,6 +1,8 @@ +using Microsoft.Win32.SafeHandles; using System; using System.Collections; using System.Collections.Generic; +using System.ComponentModel; using System.Diagnostics; using System.IO; using System.Linq; @@ -310,8 +312,8 @@ namespace Ansible.Basic public void ExitJson() { - WriteLine(GetFormattedResults(Result)); CleanupFiles(null, null); + WriteLine(GetFormattedResults(Result)); Exit(0); } @@ -338,8 +340,8 @@ namespace Ansible.Basic Result["exception"] = exception.ToString(); } - WriteLine(GetFormattedResults(Result)); CleanupFiles(null, null); + WriteLine(GetFormattedResults(Result)); Exit(1); } @@ -1445,10 +1447,22 @@ namespace Ansible.Basic { foreach (string path in cleanupFiles) { - if (File.Exists(path)) - File.Delete(path); - else if (Directory.Exists(path)) - Directory.Delete(path, true); + try + { +#if WINDOWS + FileCleaner.Delete(path); +#else + if (File.Exists(path)) + File.Delete(path); + else if (Directory.Exists(path)) + Directory.Delete(path, true); +#endif + } + catch (Exception e) + { + Warn(string.Format("Failure cleaning temp path '{0}': {1} {2}", + path, e.GetType().Name, e.Message)); + } } cleanupFiles = new List(); } @@ -1487,4 +1501,247 @@ namespace Ansible.Basic Console.WriteLine(line); } } + +#if WINDOWS + // Windows is tricky as AVs and other software might still + // have an open handle to files causing a failure. Use a + // custom deletion mechanism to remove the files/dirs. + // https://github.com/ansible/ansible/pull/80247 + internal static class FileCleaner + { + private const int FileDispositionInformation = 13; + private const int FileDispositionInformationEx = 64; + + private const int ERROR_INVALID_PARAMETER = 0x00000057; + private const int ERROR_DIR_NOT_EMPTY = 0x00000091; + + private static bool? _supportsPosixDelete = null; + + [Flags()] + public enum DispositionFlags : uint + { + FILE_DISPOSITION_DO_NOT_DELETE = 0x00000000, + FILE_DISPOSITION_DELETE = 0x00000001, + FILE_DISPOSITION_POSIX_SEMANTICS = 0x00000002, + FILE_DISPOSITION_FORCE_IMAGE_SECTION_CHECK = 0x00000004, + FILE_DISPOSITION_ON_CLOSE = 0x00000008, + FILE_DISPOSITION_IGNORE_READONLY_ATTRIBUTE = 0x00000010, + } + + [Flags()] + public enum FileFlags : uint + { + FILE_FLAG_OPEN_NO_RECALL = 0x00100000, + FILE_FLAG_OPEN_REPARSE_POINT = 0x00200000, + FILE_FLAG_SESSION_AWARE = 0x00800000, + FILE_FLAG_POSIX_SEMANTICS = 0x01000000, + FILE_FLAG_BACKUP_SEMANTICS = 0x02000000, + FILE_FLAG_DELETE_ON_CLOSE = 0x04000000, + FILE_FLAG_SEQUENTIAL_SCAN = 0x08000000, + FILE_FLAG_RANDOM_ACCESS = 0x10000000, + FILE_FLAG_NO_BUFFERING = 0x20000000, + FILE_FLAG_OVERLAPPED = 0x40000000, + FILE_FLAG_WRITE_THROUGH = 0x80000000, + } + + [DllImport("Kernel32.dll", CharSet = CharSet.Unicode, SetLastError = true)] + private static extern SafeFileHandle CreateFileW( + [MarshalAs(UnmanagedType.LPWStr)] string lpFileName, + FileSystemRights dwDesiredAccess, + FileShare dwShareMode, + IntPtr lpSecurityAttributes, + FileMode dwCreationDisposition, + uint dwFlagsAndAttributes, + IntPtr hTemplateFile); + + private static SafeFileHandle CreateFile(string path, FileSystemRights access, FileShare share, FileMode mode, + FileAttributes attributes, FileFlags flags) + { + uint flagsAndAttributes = (uint)attributes | (uint)flags; + SafeFileHandle handle = CreateFileW(path, access, share, IntPtr.Zero, mode, flagsAndAttributes, + IntPtr.Zero); + if (handle.IsInvalid) + { + int errCode = Marshal.GetLastWin32Error(); + string msg = string.Format("CreateFileW({0}) failed 0x{1:X8}: {2}", + path, errCode, new Win32Exception(errCode).Message); + throw new Win32Exception(errCode, msg); + } + + return handle; + } + + [DllImport("Ntdll.dll")] + private static extern int NtSetInformationFile( + SafeFileHandle FileHandle, + out IntPtr IoStatusBlock, + ref int FileInformation, + int Length, + int FileInformationClass); + + [DllImport("Ntdll.dll")] + private static extern int RtlNtStatusToDosError( + int Status); + + public static void Delete(string path) + { + if (File.Exists(path)) + { + DeleteEntry(path, FileAttributes.ReadOnly); + } + else if (Directory.Exists(path)) + { + Queue dirQueue = new Queue(); + dirQueue.Enqueue(new DirectoryInfo(path)); + bool nonEmptyDirs = false; + HashSet processedDirs = new HashSet(); + + while (dirQueue.Count > 0) + { + DirectoryInfo currentDir = dirQueue.Dequeue(); + + bool deleteDir = true; + if (processedDirs.Add(currentDir.FullName)) + { + foreach (FileSystemInfo entry in currentDir.EnumerateFileSystemInfos()) + { + // Tries to delete each entry. Failures are ignored + // as they will be picked up when the dir is + // deleted and not empty. + if (entry is DirectoryInfo) + { + if ((entry.Attributes & FileAttributes.ReparsePoint) != 0) + { + // If it's a reparse point, just delete it directly. + DeleteEntry(entry.FullName, entry.Attributes, ignoreFailure: true); + } + else + { + // Add the dir to the queue to delete and it will be processed next round. + dirQueue.Enqueue((DirectoryInfo)entry); + deleteDir = false; + } + } + else + { + DeleteEntry(entry.FullName, entry.Attributes, ignoreFailure: true); + } + } + } + + if (deleteDir) + { + try + { + DeleteEntry(currentDir.FullName, FileAttributes.Directory); + } + catch (Win32Exception e) + { + if (e.NativeErrorCode == ERROR_DIR_NOT_EMPTY) + { + nonEmptyDirs = true; + } + else + { + throw; + } + } + } + else + { + dirQueue.Enqueue(currentDir); + } + } + + if (nonEmptyDirs) + { + throw new IOException("Directory contains files still open by other processes"); + } + } + } + + private static void DeleteEntry(string path, FileAttributes attr, bool ignoreFailure = false) + { + try + { + if ((attr & FileAttributes.ReadOnly) != 0) + { + // Windows does not allow files set with ReadOnly to be + // deleted. Pre-emptively unset the attribute. + // FILE_DISPOSITION_IGNORE_READONLY_ATTRIBUTE is quite new, + // look at using that flag with POSIX delete once Server 2019 + // is the baseline. + File.SetAttributes(path, FileAttributes.Normal); + } + + // REPARSE - Only touch the symlink itself and not the target + // BACKUP - Needed for dir handles, bypasses access checks for admins + // DELETE_ON_CLOSE is not used as it interferes with the POSIX delete + FileFlags flags = FileFlags.FILE_FLAG_OPEN_REPARSE_POINT | + FileFlags.FILE_FLAG_BACKUP_SEMANTICS; + + using (SafeFileHandle fileHandle = CreateFile(path, FileSystemRights.Delete, + FileShare.ReadWrite | FileShare.Delete, FileMode.Open, FileAttributes.Normal, flags)) + { + if (_supportsPosixDelete == null || _supportsPosixDelete == true) + { + // A POSIX delete will delete the filesystem entry even if + // it's still opened by another process so favour that if + // available. + DispositionFlags deleteFlags = DispositionFlags.FILE_DISPOSITION_DELETE | + DispositionFlags.FILE_DISPOSITION_POSIX_SEMANTICS; + + SetInformationFile(fileHandle, FileDispositionInformationEx, (int)deleteFlags); + if (_supportsPosixDelete == true) + { + return; + } + } + + // FileDispositionInformation takes in a struct with only a BOOLEAN value. + // Using an int will also do the same thing to set that flag to true. + SetInformationFile(fileHandle, FileDispositionInformation, Int32.MaxValue); + } + } + catch + { + if (!ignoreFailure) + { + throw; + } + } + } + + private static void SetInformationFile(SafeFileHandle handle, int infoClass, int value) + { + IntPtr ioStatusBlock = IntPtr.Zero; + + int ntStatus = NtSetInformationFile(handle, out ioStatusBlock, ref value, + Marshal.SizeOf(typeof(int)), infoClass); + + if (ntStatus != 0) + { + int errCode = RtlNtStatusToDosError(ntStatus); + + // The POSIX delete was added in Server 2016 (Win 10 14393/Redstone 1) + // Mark this flag so we don't try again. + if (infoClass == FileDispositionInformationEx && _supportsPosixDelete == null && + errCode == ERROR_INVALID_PARAMETER) + { + _supportsPosixDelete = false; + return; + } + + string msg = string.Format("NtSetInformationFile() failed 0x{0:X8}: {1}", + errCode, new Win32Exception(errCode).Message); + throw new Win32Exception(errCode, msg); + } + + if (infoClass == FileDispositionInformationEx) + { + _supportsPosixDelete = true; + } + } + } +#endif } diff --git a/test/integration/targets/module_utils_Ansible.Basic/library/ansible_basic_tests.ps1 b/test/integration/targets/module_utils_Ansible.Basic/library/ansible_basic_tests.ps1 index cfa73c60c2..6170f0464a 100644 --- a/test/integration/targets/module_utils_Ansible.Basic/library/ansible_basic_tests.ps1 +++ b/test/integration/targets/module_utils_Ansible.Basic/library/ansible_basic_tests.ps1 @@ -1747,6 +1747,168 @@ test_no_log - Invoked with: Remove-Item -LiteralPath $actual_tmpdir -Force -Recurse } + "Module tmpdir with symlinks" = { + $remote_tmp = Join-Path -Path $tmpdir -ChildPath "moduletmpdir-$(Get-Random)" + New-Item -Path $remote_tmp -ItemType Directory > $null + Set-Variable -Name complex_args -Scope Global -Value @{ + _ansible_remote_tmp = $remote_tmp.ToString() + } + $m = [Ansible.Basic.AnsibleModule]::Create(@(), @{}) + + $actual_tmpdir = $m.Tmpdir + + $dir1 = Join-Path $actual_tmpdir Dir1 + $dir2 = Join-Path $actual_tmpdir Dir2 + $dir1, $dir2 | New-Item -Path { $_ } -ItemType Directory > $null + + $file1 = Join-Path $dir1 test.txt + $file2 = Join-Path $dir2 test.txt + $file3 = Join-Path $actual_tmpdir test.txt + Set-Content -LiteralPath $file1 '' + Set-Content -LiteralPath $file2 '' + Set-Content -LiteralPath $file3 '' + + $outside_target = Join-Path -Path $tmpdir -ChildPath "moduleoutsidedir-$(Get-Random)" + $outside_file = Join-Path -Path $outside_target -ChildPath "file" + New-Item -Path $outside_target -ItemType Directory > $null + Set-Content -LiteralPath $outside_file '' + + cmd.exe /c mklink /d "$dir1\missing-dir-link" "$actual_tmpdir\fake" + cmd.exe /c mklink /d "$dir1\good-dir-link" "$dir2" + cmd.exe /c mklink /d "$dir1\recursive-target-link" "$dir1" + cmd.exe /c mklink "$dir1\missing-file-link" "$actual_tmpdir\fake" + cmd.exe /c mklink "$dir1\good-file-link" "$dir2\test.txt" + cmd.exe /c mklink /d "$actual_tmpdir\outside-dir" $outside_target + cmd.exe /c mklink "$actual_tmpdir\outside-file" $outside_file + + try { + $m.ExitJson() + } + catch [System.Management.Automation.RuntimeException] { + $output = [Ansible.Basic.AnsibleModule]::FromJson($_.Exception.InnerException.Output) + } + + $output.warnings.Count | Assert-Equal -Expected 0 + (Test-Path -LiteralPath $actual_tmpdir -PathType Container) | Assert-Equal -Expected $false + (Test-Path -LiteralPath $outside_target -PathType Container) | Assert-Equal -Expected $true + (Test-Path -LiteralPath $outside_file -PathType Leaf) | Assert-Equal -Expected $true + + Remove-Item -LiteralPath $remote_tmp -Force -Recurse + } + + "Module tmpdir with undeletable file" = { + $remote_tmp = Join-Path -Path $tmpdir -ChildPath "moduletmpdir-$(Get-Random)" + New-Item -Path $remote_tmp -ItemType Directory > $null + Set-Variable -Name complex_args -Scope Global -Value @{ + _ansible_remote_tmp = $remote_tmp.ToString() + } + $m = [Ansible.Basic.AnsibleModule]::Create(@(), @{}) + + $actual_tmpdir = $m.Tmpdir + + $dir1 = Join-Path $actual_tmpdir Dir1 + $dir2 = Join-Path $actual_tmpdir Dir2 + $dir1, $dir2 | New-Item -Path { $_ } -ItemType Directory > $null + + $file1 = Join-Path $dir1 test.txt + $file2 = Join-Path $dir2 test.txt + $file3 = Join-Path $actual_tmpdir test.txt + Set-Content -LiteralPath $file1 '' + Set-Content -LiteralPath $file2 '' + Set-Content -LiteralPath $file3 '' + + $fs = [System.IO.File]::Open($file1, "Open", "Read", "None") + try { + $m.ExitJson() + } + catch [System.Management.Automation.RuntimeException] { + $output = [Ansible.Basic.AnsibleModule]::FromJson($_.Exception.InnerException.Output) + } + + $expected_msg = "Failure cleaning temp path '$actual_tmpdir': IOException Directory contains files still open by other processes" + $output.warnings.Count | Assert-Equal -Expected 1 + $output.warnings[0] | Assert-Equal -Expected $expected_msg + + (Test-Path -LiteralPath $actual_tmpdir -PathType Container) | Assert-Equal -Expected $true + (Test-Path -LiteralPath $dir1 -PathType Container) | Assert-Equal -Expected $true + # Test-Path tries to open the file in a way that fails if it's marked as deleted + (Get-ChildItem -LiteralPath $dir1 -File).Count | Assert-Equal -Expected 1 + (Test-Path -LiteralPath $dir2 -PathType Container) | Assert-Equal -Expected $false + (Test-Path -LiteralPath $file3 -PathType Leaf) | Assert-Equal -Expected $false + + # Releasing the file handle releases the lock on the file but as the + # cleanup couldn't access the file to mark as delete on close it is + # still going to be present. + $fs.Dispose() + (Test-Path -LiteralPath $dir1 -PathType Container) | Assert-Equal -Expected $true + (Test-Path -LiteralPath $file1 -PathType Leaf) | Assert-Equal -Expected $true + + Remove-Item -LiteralPath $remote_tmp -Force -Recurse + } + + "Module tmpdir delete with locked handle" = { + $remote_tmp = Join-Path -Path $tmpdir -ChildPath "moduletmpdir-$(Get-Random)" + New-Item -Path $remote_tmp -ItemType Directory > $null + Set-Variable -Name complex_args -Scope Global -Value @{ + _ansible_remote_tmp = $remote_tmp.ToString() + } + $m = [Ansible.Basic.AnsibleModule]::Create(@(), @{}) + + $actual_tmpdir = $m.Tmpdir + + $dir1 = Join-Path $actual_tmpdir Dir1 + $dir2 = Join-Path $actual_tmpdir Dir2 + $dir1, $dir2 | New-Item -Path { $_ } -ItemType Directory > $null + + $file1 = Join-Path $dir1 test.txt + $file2 = Join-Path $dir2 test.txt + $file3 = Join-Path $actual_tmpdir test.txt + Set-Content -LiteralPath $file1 '' + Set-Content -LiteralPath $file2 '' + Set-Content -LiteralPath $file3 '' + + [System.IO.File]::SetAttributes($file1, "ReadOnly") + [System.IO.File]::SetAttributes($file2, "ReadOnly") + [System.IO.File]::SetAttributes($file3, "ReadOnly") + $fs = [System.IO.File]::Open($file1, "Open", "Read", "Delete") + try { + $m.ExitJson() + } + catch [System.Management.Automation.RuntimeException] { + $output = [Ansible.Basic.AnsibleModule]::FromJson($_.Exception.InnerException.Output) + } + + if ([System.Environment]::OSVersion.Version -lt [Version]'10.0') { + # Older hosts can only do delete on close. This means Dir1 and its + # file will still be present but Dir2 should be deleted. + $expected_msg = "Failure cleaning temp path '$actual_tmpdir': IOException Directory contains files still open by other processes" + $output.warnings.Count | Assert-Equal -Expected 1 + $output.warnings[0] | Assert-Equal -Expected $expected_msg + + (Test-Path -LiteralPath $actual_tmpdir -PathType Container) | Assert-Equal -Expected $true + (Test-Path -LiteralPath $dir1 -PathType Container) | Assert-Equal -Expected $true + # Test-Path tries to open the file in a way that fails if it's marked as deleted + (Get-ChildItem -LiteralPath $dir1 -File).Count | Assert-Equal -Expected 1 + (Test-Path -LiteralPath $dir2 -PathType Container) | Assert-Equal -Expected $false + (Test-Path -LiteralPath $file3 -PathType Leaf) | Assert-Equal -Expected $false + + # Releasing the file handle releases the lock on the file deleting + # it. Unfortunately the parent dir will still be present + $fs.Dispose() + (Test-Path -LiteralPath $dir1 -PathType Container) | Assert-Equal -Expected $true + (Test-Path -LiteralPath $file1 -PathType Leaf) | Assert-Equal -Expected $false + } + else { + # Server 2016+ can use the POSIX APIs which will delete it straight away + (Test-Path -LiteralPath $actual_tmpdir -PathType Container) | Assert-Equal -Expected $false + $output.warnings.Count | Assert-Equal -Expected 0 + + $fs.Dispose() + } + + Remove-Item -LiteralPath $remote_tmp -Force -Recurse + } + "Invalid argument spec key" = { $spec = @{ invalid = $true -- cgit v1.2.1