Skip to content

Use .Net Core File.Delete() method to remove symbolic links and file streams #7017

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
daxian-dbw merged 9 commits into from
Jul 1, 2018
Merged

Use .Net Core File.Delete() method to remove symbolic links and file streams #7017

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
26d6ff6
[Feature] Use .Net Core method to remove symbolic links
iSazonov Jun 7, 2018
9682680
[Feature] Use File.Delete() to remove file streams
iSazonov Jun 7, 2018
48ef128
Remove try-catch
iSazonov Jun 9, 2018
8b76a94
Merge branch 'master' into remove-directory-reparsepoint
iSazonov Jun 18, 2018
ee31490
[Feature] Replace 'RemoveItemUnauthorizedAccessError' with 'System.Un…
iSazonov Jun 21, 2018
c7c9c9e
Write non-terminating error for UnauthorizedAccessException
iSazonov Jun 26, 2018
3cd12b3
Fix test
iSazonov Jun 26, 2018
4e8e536
Address feedback
iSazonov Jun 28, 2018
e72ee88
Fix error message
iSazonov Jun 29, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 3 additions & 11 deletions src/Microsoft.PowerShell.Commands.Utility/commands/utility/UnblockFile.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
using System.Collections.ObjectModel;
using System.ComponentModel;
using System.Diagnostics.CodeAnalysis;
using System.IO;
using System.Management.Automation;
using System.Management.Automation.Internal;

Expand Down Expand Up @@ -119,18 +120,9 @@ protected override void ProcessRecord()
{
AlternateDataStreamUtilities.DeleteFileStream(path, "Zone.Identifier");
}
catch (Win32Exception accessException)
catch (Exception e)
{
// NativeErrorCode=2 - File not found.
// If the block stream not found the 'path' was not blocked and we successfully return.
if (accessException.NativeErrorCode != 2)
{
WriteError(new ErrorRecord(accessException, "RemoveItemUnauthorizedAccessError", ErrorCategory.PermissionDenied, path));
Copy link
Member

@daxian-dbw daxian-dbw Jun 25, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This used to write out a non-terminating error, and now throw an exception which is a terminating error. I'm afraid that's not right.

Copy link
Collaborator Author

@iSazonov iSazonov Jun 26, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree. For reference https://docs.microsoft.com/en-us/powershell/developer/cmdlet/cmdlet-error-reporting#terminating-and-nonterminating-errors

Is the error condition related to a specific input object or a subset of input objects? If so, this is a nonterminating error.

}
else
{
WriteVerbose(StringUtil.Format(UtilityCommonStrings.NoZoneIdentifierFileStream, path));
}
WriteError(new ErrorRecord(e, "RemoveItemUnableToAccessFile", ErrorCategory.ResourceUnavailable, path));
}
}
}
Expand Down
3 changes: 0 additions & 3 deletions src/Microsoft.PowerShell.Commands.Utility/resources/UtilityCommonStrings.resx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -174,9 +174,6 @@
<data name="TypeNotSupported" xml:space="preserve">
<value>'{0}' is not supported in this system.</value>
Copy link
Contributor

@bergmeister bergmeister Jun 7, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would '{0}' is not supported on this platform. be better (to me it feels more consistent in terms of language compared to similar errors in .Net Core).

Copy link
Collaborator Author

@iSazonov iSazonov Jun 8, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bergmeister It is out the PR. Please push new PR or open new tracking issue.

</data>
<data name="NoZoneIdentifierFileStream" xml:space="preserve">
<value>The file is not blocked: {0}</value>
</data>
<data name="ConvertToJsonProcessValueVerboseMessage" xml:space="preserve">
<value>Processing object of type [{0}] at depth {1}</value>
</data>
Expand Down
131 changes: 9 additions & 122 deletions src/System.Management.Automation/namespaces/FileSystemProvider.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2874,41 +2874,20 @@ private void RemoveDirectoryInfoItem(DirectoryInfo directory, bool recurse, bool
continueRemoval = ShouldProcess(directory.FullName, action);
Copy link
Member

@daxian-dbw daxian-dbw Jun 27, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be out the scope for this PR, but we don't check continueRemoval before removing the junction point ... that seems wrong.

Copy link
Collaborator Author

@iSazonov iSazonov Jun 28, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's postpone to new PR.

Copy link
Collaborator Author

@iSazonov iSazonov Jul 2, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see a comment before the line says that we ask the confirmation only for removing root directory or recurse - but seems current behavior is that we remove the junction point as an object (not directory and no recurse) so we don't ask confirmation.

}

if ((directory.Attributes & FileAttributes.ReparsePoint) != 0)
if (directory.Attributes.HasFlag(FileAttributes.ReparsePoint))
{
bool success = InternalSymbolicLinkLinkCodeMethods.DeleteJunction(directory.FullName);

if (!success)
{
string error = StringUtil.Format(FileSystemProviderStrings.CannotRemoveItem, directory.FullName);
Exception e = new IOException(error);
WriteError(new ErrorRecord(e, "DeleteJunctionFailed", ErrorCategory.WriteError, directory));
return;
}

try
{
if (!Utils.ItemExists(directory.FullName, out bool _))
{
// Directory does not exist
return;
}
directory.Delete();
}
catch (Exception accessException)
catch (Exception e)
{
ErrorRecord errorRecord = new ErrorRecord(accessException, "RemoveFileSystemItemUnAuthorizedAccess", ErrorCategory.PermissionDenied, directory);

ErrorDetails errorDetails =
new ErrorDetails(this, "FileSystemProviderStrings",
"CannotRemoveItem",
directory.FullName,
accessException.Message);

errorRecord.ErrorDetails = errorDetails;

WriteError(errorRecord);
return;
string error = StringUtil.Format(FileSystemProviderStrings.CannotRemoveItem, directory.FullName);
Exception exception = new IOException(error, e);
WriteError(new ErrorRecord(exception, "DeleteSymbolicLinkFailed", ErrorCategory.WriteError, directory));
}

return;
}

if (continueRemoval)
Expand Down Expand Up @@ -8336,91 +8315,6 @@ private static bool WinCreateJunction(string path, string target)
}
}

[System.Diagnostics.CodeAnalysis.SuppressMessage("Microsoft.Reliability", "CA2001:AvoidCallingProblematicMethods")]
internal static bool DeleteJunction(string junctionPath)
{
bool result = false;

if (!String.IsNullOrEmpty(junctionPath))
{
if (!Platform.IsWindows)
{
// For non-Windows platform, treat it as a file. Just delete it.
try
{
File.Delete(junctionPath);
return true;
}
catch
{
return false;
}
}

using (SafeHandle handle = OpenReparsePoint(junctionPath, FileDesiredAccess.GenericWrite))
{
bool success = false;
int inOutBufferSize = Marshal.SizeOf<REPARSE_GUID_DATA_BUFFER>();
IntPtr outBuffer = Marshal.AllocHGlobal(inOutBufferSize);
IntPtr inBuffer = Marshal.AllocHGlobal(inOutBufferSize);

try
{
handle.DangerousAddRef(ref success);
IntPtr dangerousHandle = handle.DangerousGetHandle();
int bytesReturned;

// Do a FSCTL_GET_REPARSE_POINT first because the ReparseTag could be
// IO_REPARSE_TAG_MOUNT_POINT or IO_REPARSE_TAG_SYMLINK.
// Using the wrong one results in mismatched-tag error.

REPARSE_GUID_DATA_BUFFER junctionData = new REPARSE_GUID_DATA_BUFFER();
Marshal.StructureToPtr<REPARSE_GUID_DATA_BUFFER>(junctionData, outBuffer, false);

result = DeviceIoControl(dangerousHandle, FSCTL_GET_REPARSE_POINT, IntPtr.Zero, 0,
outBuffer, inOutBufferSize, out bytesReturned, IntPtr.Zero);
if (!result)
{
int lastError = Marshal.GetLastWin32Error();
throw new Win32Exception(lastError);
}

junctionData = Marshal.PtrToStructure<REPARSE_GUID_DATA_BUFFER>(outBuffer);
junctionData.ReparseDataLength = 0;
junctionData.DataBuffer = new char[MAX_REPARSE_SIZE];

Marshal.StructureToPtr<REPARSE_GUID_DATA_BUFFER>(junctionData, inBuffer, false);

// To delete a reparse point:
// ReparseDataLength must be 0
// inBufferSize must be REPARSE_GUID_DATA_BUFFER_HEADER_SIZE
result = DeviceIoControl(dangerousHandle, FSCTL_DELETE_REPARSE_POINT, inBuffer, REPARSE_GUID_DATA_BUFFER_HEADER_SIZE, IntPtr.Zero, 0, out bytesReturned, IntPtr.Zero);
if (!result)
{
int lastError = Marshal.GetLastWin32Error();
throw new Win32Exception(lastError);
}
}
finally
{
if (success)
{
handle.DangerousRelease();
}

Marshal.FreeHGlobal(outBuffer);
Marshal.FreeHGlobal(inBuffer);
}
}
}
else
{
throw new ArgumentNullException("junctionPath");
}

return result;
}

private static SafeFileHandle OpenReparsePoint(string reparsePoint, FileDesiredAccess accessMode)
{
#if UNIX
Expand Down Expand Up @@ -8585,11 +8479,7 @@ internal static void DeleteFileStream(string path, string streamName)
}
string resultPath = path + adjustedStreamName;

if (!NativeMethods.DeleteFile(resultPath))
{
int error = Marshal.GetLastWin32Error();
throw new Win32Exception(error);
}
File.Delete(resultPath);
}

internal static void SetZoneOfOrigin(string path, SecurityZone securityZone)
Expand Down Expand Up @@ -8617,9 +8507,6 @@ internal static extern SafeFileHandle CreateFile(string lpFileName,
IntPtr lpSecurityAttributes, FileMode dwCreationDisposition,
int dwFlagsAndAttributes, IntPtr hTemplateFile);

[DllImport(PinvokeDllNames.DeleteFileDllName, CharSet = CharSet.Unicode, SetLastError = true)]
internal static extern bool DeleteFile(string lpFileName);

[DllImport(PinvokeDllNames.FindFirstStreamDllName, ExactSpelling = true, CharSet = CharSet.Unicode, SetLastError = true)]
[SuppressMessage("Microsoft.Globalization", "CA2101:SpecifyMarshalingForPInvokeStringArguments", MessageId = "AlternateStreamNativeData.Name")]
internal static extern SafeFindHandle FindFirstStreamW(
Expand Down
2 changes: 2 additions & 0 deletions test/powershell/Modules/Microsoft.PowerShell.Management/New-Item.Tests.ps1
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -204,6 +204,8 @@ Describe "New-Item with links" -Tags @('CI', 'RequireAdminOnWindows') {

# Remove the link explicitly to avoid broken symlink issue
Remove-Item $FullyQualifiedLink -Force
# Test a code path removing a symbolic link (reparse point)
Test-Path $FullyQualifiedLink | Should -BeFalse
}

It "New-Item -ItemType SymbolicLink should understand directory path ending with slash" {
Expand Down
2 changes: 1 addition & 1 deletion test/powershell/Modules/Microsoft.PowerShell.Utility/Unblock-File.Tests.ps1
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,6 @@ Describe "Unblock-File" -Tags "CI" {
$TestFileCreated = Get-ChildItem $TestFile
$TestFileCreated.IsReadOnly | Should -BeTrue

{ Unblock-File -LiteralPath $TestFile -ErrorAction Stop } | Should -Throw -ErrorId "RemoveItemUnauthorizedAccessError,Microsoft.PowerShell.Commands.UnblockFileCommand"
{ Unblock-File -LiteralPath $TestFile -ErrorAction Stop } | Should -Throw -ErrorId "RemoveItemUnableToAccessFile,Microsoft.PowerShell.Commands.UnblockFileCommand"
}
}