Skip to content

Use RegDeleteTree in RegistryKey.DeleteSubKeyTree #82598

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
ViktorHofer merged 5 commits into from
Jul 21, 2023
Merged

Use RegDeleteTree in RegistryKey.DeleteSubKeyTree #82598

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
1404726
Use RegDeleteTree in RegistryKey.DeleteSubKeyTree
huoyaoyuan Feb 28, 2023
da3c445
Restore self delete behavior
huoyaoyuan Mar 2, 2023
a9e72c4
Call RegDeleteTree on subkey to simulate permission behavior.
huoyaoyuan May 2, 2023
880b285
Merge branch 'main' into reg-delete-tree
huoyaoyuan Jun 28, 2023
ec45676
Adjust comment and add tests.
huoyaoyuan Jul 12, 2023
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
20 changes: 20 additions & 0 deletions src/libraries/Common/src/Interop/Windows/Advapi32/Interop.RegDeleteTree.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

#if REGISTRY_ASSEMBLY
using Microsoft.Win32.SafeHandles;
#else
using Internal.Win32.SafeHandles;
#endif
using System.Runtime.InteropServices;

internal static partial class Interop
{
internal static partial class Advapi32
{
[LibraryImport(Libraries.Advapi32, EntryPoint = "RegDeleteTreeW", StringMarshalling = StringMarshalling.Utf16)]
internal static partial int RegDeleteTree(
SafeRegistryHandle hKey,
string lpSubKey);
}
}
2 changes: 2 additions & 0 deletions src/libraries/Microsoft.Win32.Registry/src/Microsoft.Win32.Registry.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,8 @@
Link="Common\Interop\Windows\Interop.RegCreateKeyEx.cs" />
<Compile Include="$(CommonPath)Interop\Windows\Advapi32\Interop.RegDeleteKeyEx.cs"
Link="Common\Interop\Windows\Interop.RegDeleteKeyEx.cs" />
<Compile Include="$(CommonPath)Interop\Windows\Advapi32\Interop.RegDeleteTree.cs"
Link="Common\Interop\Windows\Interop.RegDeleteTree.cs" />
<Compile Include="$(CommonPath)Interop\Windows\Advapi32\Interop.RegDeleteValue.cs"
Link="Common\Interop\Windows\Interop.RegDeleteValue.cs" />
<Compile Include="$(CommonPath)Interop\Windows\Advapi32\Interop.RegEnumKeyEx.cs"
Expand Down
65 changes: 16 additions & 49 deletions src/libraries/Microsoft.Win32.Registry/src/Microsoft/Win32/RegistryKey.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -334,65 +334,32 @@ public void DeleteSubKeyTree(string subkey, bool throwOnMissingSubKey)

subkey = FixupName(subkey); // Fixup multiple slashes to a single slash

RegistryKey? key = InternalOpenSubKeyWithoutSecurityChecks(subkey, true);
if (key != null)
int ret = Interop.Advapi32.RegDeleteTree(_hkey, subkey);
Copy link
Member

@danmoseley danmoseley Feb 27, 2023

Choose a reason for hiding this comment

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

Doc says:

A handle to an open registry key. The key must have been opened with the following access rights: DELETE, KEY_ENUMERATE_SUB_KEYS, and KEY_QUERY_VALUE. For more information, see Registry Key Security and Access Rights.

but for RegDeleteKeyEx it does not require this

A handle to an open registry key. The access rights of this key do not affect the delete operation. For more information about access rights, see Registry Key Security and Access Rights.

Is there any way this change could cause something to fail that worked before?

Copy link
Member Author

@huoyaoyuan huoyaoyuan Feb 28, 2023

Choose a reason for hiding this comment

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

I don't think it can cause deletion to fail. Should we add a manual access check to restore existing behavior?

Copy link
Member

@danmoseley danmoseley Feb 28, 2023

Choose a reason for hiding this comment

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

you mean, you've verified that all codepaths that lead here provide a handle that has DELETE, KEY_ENUMERATE_SUB_KEYS, and KEY_QUERY_VALUE?

Copy link
Member Author

@huoyaoyuan huoyaoyuan Mar 1, 2023

Choose a reason for hiding this comment

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

This is a public API. I mean that we may perform additional access check to match the document.
I've checked Windows source code and RegDeleteKeyEx uses the same enumeration process. I'm not sure where the access check happens though.

Copy link
Member

@ericstj ericstj Mar 27, 2023

Choose a reason for hiding this comment

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

Maybe you can try to answer this question through testing. Is there a case where you can specify different values of RegistryRights when opening the key and see if you can create a situation where it would pass before but fails now?

I tend to agree with you that the old case should have needed these permissions - should be pretty easy to test to prove that.

Copy link
Member Author

@huoyaoyuan huoyaoyuan May 21, 2023

Choose a reason for hiding this comment

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

Perhaps we need a matrix for behaviors of the old and new way. The old way should also cause partial deletion in some cases.

Copy link
Member Author

@huoyaoyuan huoyaoyuan Jul 9, 2023

Choose a reason for hiding this comment

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

Notes after reading more about registry:

  • The RegistryKeyPermissionCheck or isWritable check only happens at managed side. It can be easily replicated.
  • Registry checks the access of current user, not desired access of the handle.

So special casing "" should be enough. No matrix needed actually.

Copy link
Member Author

@huoyaoyuan huoyaoyuan Jul 9, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

The following code fails to delete with P/Invoke, but successes with DeleteSubKeyTree on .NET 7:

I can't reproduce the failure now. Maybe I missed something.

Edit: RegDeleteTree returns ACCESS_DENIED when lpSubKey is "" and the key contains values.

Copy link
Member

@danmoseley danmoseley Jul 10, 2023

Choose a reason for hiding this comment

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

I think the key thing here is to have tests that pass on both the old and new ways, if we don't already, to prove there's no visible change in behavior. There are no doubt snippets of code elsewhere in the tests that you can grab to set ACL's on keys, etc.

Copy link
Member Author

@huoyaoyuan huoyaoyuan Jul 12, 2023

Choose a reason for hiding this comment

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

The failures should be the documented case of RegDeleteTree:

If the key has values, it must be opened with KEY_SET_VALUE or this function will fail with ERROR_ACCESS_DENIED.

I think covering the case should be enough. ACL handlings shouldn't be changed.


if (ret == Interop.Errors.ERROR_FILE_NOT_FOUND)
{
using (key)
if (throwOnMissingSubKey)
{
if (key.SubKeyCount > 0)
{
string[] keys = key.GetSubKeyNames();

for (int i = 0; i < keys.Length; i++)
{
key.DeleteSubKeyTreeInternal(keys[i]);
}
}
throw new ArgumentException(SR.Arg_RegSubKeyAbsent);
}

DeleteSubKeyTreeCore(subkey);
}
else if (throwOnMissingSubKey)
else if (ret != 0)
{
throw new ArgumentException(SR.Arg_RegSubKeyAbsent);
Win32Error(ret, null);
}
}

/// <summary>
/// An internal version which does no security checks or argument checking. Skipping the
/// security checks should give us a slight perf gain on large trees.
/// </summary>
private void DeleteSubKeyTreeInternal(string subkey)
{
RegistryKey? key = InternalOpenSubKeyWithoutSecurityChecks(subkey, true);
if (key != null)
if (string.IsNullOrEmpty(subkey))
{
using (key)
// If subkey is empty, the old implementation opens a new handle to current key
// and deletes current key.
// However, RegDeleteTree only delete subkeys and values of current key
// if subkey is null.
// Also delete current key to restore old behavior.
ret = Interop.Advapi32.RegDeleteKeyEx(_hkey, subkey, (int)_regView, 0);
if (ret != 0)
{
if (key.SubKeyCount > 0)
{
string[] keys = key.GetSubKeyNames();
for (int i = 0; i < keys.Length; i++)
{
key.DeleteSubKeyTreeInternal(keys[i]);
}
}
Win32Error(ret, null);
}

DeleteSubKeyTreeCore(subkey);
}
else
{
throw new ArgumentException(SR.Arg_RegSubKeyAbsent);
}
}

private void DeleteSubKeyTreeCore(string subkey)
{
int ret = Interop.Advapi32.RegDeleteKeyEx(_hkey, subkey, (int)_regView, 0);
if (ret != 0)
{
Win32Error(ret, null);
}
}

Expand Down