From 144cac3d0ce162444452a5dcc837b904fe149ef4 Mon Sep 17 00:00:00 2001 From: Dan Moseley Date: Sun, 3 Jan 2021 15:28:57 -0800 Subject: [PATCH] Remove some dead stuff (#46494) * Remove some dead stuff * Remove unnecessary test * Remove FCall alias --- .../Protocols/ldap/LdapSessionOptions.cs | 12 +- .../Security/SecurityCriticalAttribute.cs | 4 +- .../System/Security/SecurityRulesAttribute.cs | 8 +- .../Security/SecuritySafeCriticalAttribute.cs | 10 +- .../Security/SecurityTransparentAttribute.cs | 7 +- .../Security/SecurityTreatAsSafeAttribute.cs | 9 +- .../SuppressUnmanagedCodeSecurityAttribute.cs | 3 +- .../src/System/Threading/SemaphoreSlim.cs | 13 +- .../Threading/Tasks/ProducerConsumerQueues.cs | 10 +- .../src/System/Threading/ThreadLocal.cs | 15 +- .../Security/AccessControl/Privilege.cs | 342 +++++++----------- 11 files changed, 149 insertions(+), 284 deletions(-) diff --git a/src/libraries/System.DirectoryServices.Protocols/src/System/DirectoryServices/Protocols/ldap/LdapSessionOptions.cs b/src/libraries/System.DirectoryServices.Protocols/src/System/DirectoryServices/Protocols/ldap/LdapSessionOptions.cs index 39d78ed9604e3e..d96c6cceed644b 100644 --- a/src/libraries/System.DirectoryServices.Protocols/src/System/DirectoryServices/Protocols/ldap/LdapSessionOptions.cs +++ b/src/libraries/System.DirectoryServices.Protocols/src/System/DirectoryServices/Protocols/ldap/LdapSessionOptions.cs @@ -1093,11 +1093,7 @@ private static bool AddLdapHandleRef(LdapConnection ldapConnection) bool success = false; if (ldapConnection != null && ldapConnection._ldapHandle != null && !ldapConnection._ldapHandle.IsInvalid) { - try { } - finally - { - ldapConnection._ldapHandle.DangerousAddRef(ref success); - } + ldapConnection._ldapHandle.DangerousAddRef(ref success); } return success; @@ -1107,11 +1103,7 @@ private static void ReleaseLdapHandleRef(LdapConnection ldapConnection) { if (ldapConnection != null && ldapConnection._ldapHandle != null && !ldapConnection._ldapHandle.IsInvalid) { - try { } - finally - { - ldapConnection._ldapHandle.DangerousRelease(); - } + ldapConnection._ldapHandle.DangerousRelease(); } } } diff --git a/src/libraries/System.Private.CoreLib/src/System/Security/SecurityCriticalAttribute.cs b/src/libraries/System.Private.CoreLib/src/System/Security/SecurityCriticalAttribute.cs index 394b418c3218ed..90668e1979179b 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Security/SecurityCriticalAttribute.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Security/SecurityCriticalAttribute.cs @@ -3,9 +3,7 @@ namespace System.Security { - // SecurityCriticalAttribute - // Indicates that the decorated code or assembly performs security critical operations (e.g. Assert, "unsafe", LinkDemand, etc.) - // The attribute can be placed on most targets, except on arguments/return values. + // Has no effect in .NET Core [AttributeUsage(AttributeTargets.Assembly | AttributeTargets.Class | AttributeTargets.Struct | diff --git a/src/libraries/System.Private.CoreLib/src/System/Security/SecurityRulesAttribute.cs b/src/libraries/System.Private.CoreLib/src/System/Security/SecurityRulesAttribute.cs index 32520efc732582..b38b2786589822 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Security/SecurityRulesAttribute.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Security/SecurityRulesAttribute.cs @@ -3,13 +3,7 @@ namespace System.Security { - // SecurityRulesAttribute - // - // Indicates which set of security rules an assembly was authored against, and therefore which set of - // rules the runtime should enforce on the assembly. For instance, an assembly marked with - // [SecurityRules(SecurityRuleSet.Level1)] will follow the v2.0 transparency rules, where transparent code - // can call a LinkDemand by converting it to a full demand, public critical methods are implicitly - // treat as safe, and the remainder of the v2.0 rules apply. + // Has no effect in .NET Core [AttributeUsage(AttributeTargets.Assembly, AllowMultiple = false)] public sealed class SecurityRulesAttribute : Attribute { diff --git a/src/libraries/System.Private.CoreLib/src/System/Security/SecuritySafeCriticalAttribute.cs b/src/libraries/System.Private.CoreLib/src/System/Security/SecuritySafeCriticalAttribute.cs index f686e08597c8ca..058ab689c79a10 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Security/SecuritySafeCriticalAttribute.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Security/SecuritySafeCriticalAttribute.cs @@ -3,15 +3,7 @@ namespace System.Security { - // SecuritySafeCriticalAttribute: - // Indicates that the code may contain violations to the security critical rules (e.g. transitions from - // critical to non-public transparent, transparent to non-public critical, etc.), has been audited for - // security concerns and is considered security clean. Also indicates that the code is considered SecurityCritical. - // The effect of this attribute is as if the code was marked [SecurityCritical][SecurityTreatAsSafe]. - // At assembly-scope, all rule checks will be suppressed within the assembly and for calls made against the assembly. - // At type-scope, all rule checks will be suppressed for members within the type and for calls made against the type. - // At member level (e.g. field and method) the code will be treated as public - i.e. no rule checks for the members. - + // Has no effect in .NET Core [AttributeUsage(AttributeTargets.Class | AttributeTargets.Struct | AttributeTargets.Enum | diff --git a/src/libraries/System.Private.CoreLib/src/System/Security/SecurityTransparentAttribute.cs b/src/libraries/System.Private.CoreLib/src/System/Security/SecurityTransparentAttribute.cs index 7c02c9970a4bbd..36373553cd1de5 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Security/SecurityTransparentAttribute.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Security/SecurityTransparentAttribute.cs @@ -3,12 +3,7 @@ namespace System.Security { - // SecurityTransparentAttribute: - // Indicates the assembly contains only transparent code. - // Security critical actions will be restricted or converted into less critical actions. For example, - // Assert will be restricted, SuppressUnmanagedCode, LinkDemand, unsafe, and unverifiable code will be converted - // into Full-Demands. - + // Has no effect in .NET Core [AttributeUsage(AttributeTargets.Assembly, AllowMultiple = false, Inherited = false)] public sealed class SecurityTransparentAttribute : Attribute { diff --git a/src/libraries/System.Private.CoreLib/src/System/Security/SecurityTreatAsSafeAttribute.cs b/src/libraries/System.Private.CoreLib/src/System/Security/SecurityTreatAsSafeAttribute.cs index 77f6062b9670f2..c872d7ca2ec0d0 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Security/SecurityTreatAsSafeAttribute.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Security/SecurityTreatAsSafeAttribute.cs @@ -3,14 +3,7 @@ namespace System.Security { - // SecurityTreatAsSafeAttribute: - // Indicates that the code may contain violations to the security critical rules (e.g. transitions from - // critical to non-public transparent, transparent to non-public critical, etc.), has been audited for - // security concerns and is considered security clean. - // At assembly-scope, all rule checks will be suppressed within the assembly and for calls made against the assembly. - // At type-scope, all rule checks will be suppressed for members within the type and for calls made against the type. - // At member level (e.g. field and method) the code will be treated as public - i.e. no rule checks for the members. - + // Has no effect in .NET Core [AttributeUsage(AttributeTargets.Assembly | AttributeTargets.Class | AttributeTargets.Struct | diff --git a/src/libraries/System.Private.CoreLib/src/System/Security/SuppressUnmanagedCodeSecurityAttribute.cs b/src/libraries/System.Private.CoreLib/src/System/Security/SuppressUnmanagedCodeSecurityAttribute.cs index 38e6f53cdd3418..d59de2dbbc914a 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Security/SuppressUnmanagedCodeSecurityAttribute.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Security/SuppressUnmanagedCodeSecurityAttribute.cs @@ -3,8 +3,7 @@ namespace System.Security { - // SuppressUnmanagedCodeSecurityAttribute: - // This attribute has no functional impact in CoreCLR. + // Has no effect in .NET Core [AttributeUsage(AttributeTargets.Method | AttributeTargets.Class | AttributeTargets.Interface | AttributeTargets.Delegate, AllowMultiple = true, Inherited = false)] public sealed class SuppressUnmanagedCodeSecurityAttribute : Attribute { diff --git a/src/libraries/System.Private.CoreLib/src/System/Threading/SemaphoreSlim.cs b/src/libraries/System.Private.CoreLib/src/System/Threading/SemaphoreSlim.cs index dd573007ce2158..b9a28f2e4c4693 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Threading/SemaphoreSlim.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Threading/SemaphoreSlim.cs @@ -341,17 +341,8 @@ public bool Wait(int millisecondsTimeout, CancellationToken cancellationToken) } } } - // entering the lock and incrementing waiters must not suffer a thread-abort, else we cannot - // clean up m_waitCount correctly, which may lead to deadlock due to non-woken waiters. - try { } - finally - { - Monitor.Enter(m_lockObjAndDisposed, ref lockTaken); - if (lockTaken) - { - m_waitCount++; - } - } + Monitor.Enter(m_lockObjAndDisposed, ref lockTaken); + m_waitCount++; // If there are any async waiters, for fairness we'll get in line behind // then by translating our synchronous wait into an asynchronous one that we diff --git a/src/libraries/System.Private.CoreLib/src/System/Threading/Tasks/ProducerConsumerQueues.cs b/src/libraries/System.Private.CoreLib/src/System/Threading/Tasks/ProducerConsumerQueues.cs index b1ef1c1b0339b6..83bef50e8b204b 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Threading/Tasks/ProducerConsumerQueues.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Threading/Tasks/ProducerConsumerQueues.cs @@ -181,14 +181,8 @@ private void EnqueueSlow(T item, ref Segment segment) newSegment.m_state.m_last = 1; newSegment.m_state.m_lastCopy = 1; - try { } - finally - { - // Finally block to protect against corruption due to a thread abort - // between setting m_next and setting m_tail. - Volatile.Write(ref m_tail.m_next, newSegment); // ensure segment not published until item is fully stored - m_tail = newSegment; - } + Volatile.Write(ref m_tail.m_next, newSegment); // ensure segment not published until item is fully stored + m_tail = newSegment; } /// Attempts to dequeue an item from the queue. diff --git a/src/libraries/System.Private.CoreLib/src/System/Threading/ThreadLocal.cs b/src/libraries/System.Private.CoreLib/src/System/Threading/ThreadLocal.cs index 4d7188e8580692..cf803236411993 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Threading/ThreadLocal.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Threading/ThreadLocal.cs @@ -122,17 +122,12 @@ private void Initialize(Func? valueFactory, bool trackAllValues) _valueFactory = valueFactory; _trackAllValues = trackAllValues; - // Assign the ID and mark the instance as initialized. To avoid leaking IDs, we assign the ID and set _initialized - // in a finally block, to avoid a thread abort in between the two statements. - try { } - finally - { - _idComplement = ~s_idManager.GetId(); + // Assign the ID and mark the instance as initialized. + _idComplement = ~s_idManager.GetId(); - // As the last step, mark the instance as fully initialized. (Otherwise, if _initialized=false, we know that an exception - // occurred in the constructor.) - _initialized = true; - } + // As the last step, mark the instance as fully initialized. (Otherwise, if _initialized=false, we know that an exception + // occurred in the constructor.) + _initialized = true; } /// diff --git a/src/libraries/System.Security.AccessControl/src/System/Security/AccessControl/Privilege.cs b/src/libraries/System.Security.AccessControl/src/System/Security/AccessControl/Privilege.cs index de42e45d8b6b3c..49c2d23a33ff1f 100644 --- a/src/libraries/System.Security.AccessControl/src/System/Security/AccessControl/Privilege.cs +++ b/src/libraries/System.Security.AccessControl/src/System/Security/AccessControl/Privilege.cs @@ -1,16 +1,6 @@ // Licensed to the .NET Foundation under one or more agreements. // The .NET Foundation licenses this file to you under the MIT license. -/*============================================================ -** -** Class: Privilege -** -** Purpose: Managed wrapper for NT privileges. -** -** Date: July 1, 2004 -** -===========================================================*/ - using Microsoft.Win32; using Microsoft.Win32.SafeHandles; using System.Collections; @@ -22,15 +12,14 @@ using System.Security.Principal; using System.Threading; using CultureInfo = System.Globalization.CultureInfo; -using FCall = System.Security.Principal.Win32; using Luid = Interop.Advapi32.LUID; +using static System.Security.Principal.Win32; namespace System.Security.AccessControl { -#if false - internal delegate void PrivilegedHelper(); -#endif - + /// + /// Managed wrapper for NT privileges + /// internal sealed class Privilege { [ThreadStatic] @@ -198,85 +187,78 @@ public TlsContents() try { - // Make the sequence non-interruptible - } - finally - { - try + // + // Open the thread token; if there is no thread token, get one from + // the process token by impersonating self. + // + + SafeTokenHandle? threadHandleBefore = this.threadHandle; + error = OpenThreadToken( + TokenAccessLevels.Query | TokenAccessLevels.AdjustPrivileges, + WinSecurityContext.Process, + out this.threadHandle); + unchecked { error &= ~(int)0x80070000; } + + if (error != 0) { - // - // Open the thread token; if there is no thread token, get one from - // the process token by impersonating self. - // - - SafeTokenHandle? threadHandleBefore = this.threadHandle; - error = FCall.OpenThreadToken( - TokenAccessLevels.Query | TokenAccessLevels.AdjustPrivileges, - WinSecurityContext.Process, - out this.threadHandle); - unchecked { error &= ~(int)0x80070000; } - - if (error != 0) + if (success == true) { - if (success == true) - { - this.threadHandle = threadHandleBefore; + this.threadHandle = threadHandleBefore; - if (error != Interop.Errors.ERROR_NO_TOKEN) - { - success = false; - } + if (error != Interop.Errors.ERROR_NO_TOKEN) + { + success = false; + } - System.Diagnostics.Debug.Assert(this.isImpersonating == false, "Incorrect isImpersonating state"); + System.Diagnostics.Debug.Assert(this.isImpersonating == false, "Incorrect isImpersonating state"); - if (success == true) + if (success == true) + { + error = 0; + if (false == Interop.Advapi32.DuplicateTokenEx( + processHandle, + TokenAccessLevels.Impersonate | TokenAccessLevels.Query | TokenAccessLevels.AdjustPrivileges, + IntPtr.Zero, + Interop.Advapi32.SECURITY_IMPERSONATION_LEVEL.SecurityImpersonation, + System.Security.Principal.TokenType.TokenImpersonation, + ref this.threadHandle)) { - error = 0; - if (false == Interop.Advapi32.DuplicateTokenEx( - processHandle, - TokenAccessLevels.Impersonate | TokenAccessLevels.Query | TokenAccessLevels.AdjustPrivileges, - IntPtr.Zero, - Interop.Advapi32.SECURITY_IMPERSONATION_LEVEL.SecurityImpersonation, - System.Security.Principal.TokenType.TokenImpersonation, - ref this.threadHandle)) - { - error = Marshal.GetLastWin32Error(); - success = false; - } + error = Marshal.GetLastWin32Error(); + success = false; } + } - if (success == true) - { - error = FCall.SetThreadToken(this.threadHandle); - unchecked { error &= ~(int)0x80070000; } - - if (error != 0) - { - success = false; - } - } + if (success == true) + { + error = SetThreadToken(this.threadHandle); + unchecked { error &= ~(int)0x80070000; } - if (success == true) + if (error != 0) { - this.isImpersonating = true; + success = false; } } - else + + if (success == true) { - error = cachingError; + this.isImpersonating = true; } } else { - success = true; + error = cachingError; } } - finally + else { - if (!success) - { - Dispose(); - } + success = true; + } + } + finally + { + if (!success) + { + Dispose(); } } @@ -439,92 +421,76 @@ private unsafe void ToggleState(bool enable) throw new InvalidOperationException(SR.InvalidOperation_MustRevertPrivilege); } - // - // Need to make this block of code non-interruptible so that it would preserve - // consistency of thread oken state even in the face of catastrophic exceptions - // - try { // - // The payload is entirely in the finally block - // This is how we ensure that the code will not be - // interrupted by catastrophic exceptions + // Retrieve TLS state // - } - finally - { - try - { - // - // Retrieve TLS state - // - this.tlsContents = t_tlsSlotData; + this.tlsContents = t_tlsSlotData; - if (this.tlsContents == null) - { - this.tlsContents = new TlsContents(); - t_tlsSlotData = this.tlsContents; - } - else - { - this.tlsContents.IncrementReferenceCount(); - } + if (this.tlsContents == null) + { + this.tlsContents = new TlsContents(); + t_tlsSlotData = this.tlsContents; + } + else + { + this.tlsContents.IncrementReferenceCount(); + } - Interop.Advapi32.TOKEN_PRIVILEGE newState; - newState.PrivilegeCount = 1; - newState.Privileges.Luid = this.luid; - newState.Privileges.Attributes = enable ? Interop.Advapi32.SEPrivileges.SE_PRIVILEGE_ENABLED : Interop.Advapi32.SEPrivileges.SE_PRIVILEGE_DISABLED; + Interop.Advapi32.TOKEN_PRIVILEGE newState; + newState.PrivilegeCount = 1; + newState.Privileges.Luid = this.luid; + newState.Privileges.Attributes = enable ? Interop.Advapi32.SEPrivileges.SE_PRIVILEGE_ENABLED : Interop.Advapi32.SEPrivileges.SE_PRIVILEGE_DISABLED; - Interop.Advapi32.TOKEN_PRIVILEGE previousState = default; - uint previousSize = 0; + Interop.Advapi32.TOKEN_PRIVILEGE previousState = default; + uint previousSize = 0; + // + // Place the new privilege on the thread token and remember the previous state. + // + + if (!Interop.Advapi32.AdjustTokenPrivileges( + this.tlsContents.ThreadHandle, + false, + &newState, + (uint)sizeof(Interop.Advapi32.TOKEN_PRIVILEGE), + &previousState, + &previousSize)) + { + error = Marshal.GetLastWin32Error(); + } + else if (Interop.Errors.ERROR_NOT_ALL_ASSIGNED == Marshal.GetLastWin32Error()) + { + error = Interop.Errors.ERROR_NOT_ALL_ASSIGNED; + } + else + { // - // Place the new privilege on the thread token and remember the previous state. + // This is the initial state that revert will have to go back to // - if (!Interop.Advapi32.AdjustTokenPrivileges( - this.tlsContents.ThreadHandle, - false, - &newState, - (uint)sizeof(Interop.Advapi32.TOKEN_PRIVILEGE), - &previousState, - &previousSize)) - { - error = Marshal.GetLastWin32Error(); - } - else if (Interop.Errors.ERROR_NOT_ALL_ASSIGNED == Marshal.GetLastWin32Error()) - { - error = Interop.Errors.ERROR_NOT_ALL_ASSIGNED; - } - else - { - // - // This is the initial state that revert will have to go back to - // - - this.initialState = ((previousState.Privileges.Attributes & Interop.Advapi32.SEPrivileges.SE_PRIVILEGE_ENABLED) != 0); + this.initialState = ((previousState.Privileges.Attributes & Interop.Advapi32.SEPrivileges.SE_PRIVILEGE_ENABLED) != 0); - // - // Remember whether state has changed at all - // + // + // Remember whether state has changed at all + // - this.stateWasChanged = (this.initialState != enable); + this.stateWasChanged = (this.initialState != enable); - // - // If we had to impersonate, or if the privilege state changed we'll need to revert - // + // + // If we had to impersonate, or if the privilege state changed we'll need to revert + // - this.needToRevert = this.tlsContents.IsImpersonating || this.stateWasChanged; - } + this.needToRevert = this.tlsContents.IsImpersonating || this.stateWasChanged; } - finally + } + finally + { + if (!this.needToRevert) { - if (!this.needToRevert) - { - this.Reset(); - } + this.Reset(); } } @@ -562,57 +528,42 @@ public unsafe void Revert() return; } - // - // This code must be eagerly prepared and non-interruptible. - // + bool success = true; try { // - // The payload is entirely in the finally block - // This is how we ensure that the code will not be - // interrupted by catastrophic exceptions + // Only call AdjustTokenPrivileges if we're not going to be reverting to self, + // on this Revert, since doing the latter obliterates the thread token anyway // - } - finally - { - bool success = true; - try + if (this.stateWasChanged && + (this.tlsContents!.ReferenceCountValue > 1 || + !this.tlsContents.IsImpersonating)) { - // - // Only call AdjustTokenPrivileges if we're not going to be reverting to self, - // on this Revert, since doing the latter obliterates the thread token anyway - // + Interop.Advapi32.TOKEN_PRIVILEGE newState; + newState.PrivilegeCount = 1; + newState.Privileges.Luid = this.luid; + newState.Privileges.Attributes = (this.initialState ? Interop.Advapi32.SEPrivileges.SE_PRIVILEGE_ENABLED : Interop.Advapi32.SEPrivileges.SE_PRIVILEGE_DISABLED); - if (this.stateWasChanged && - (this.tlsContents!.ReferenceCountValue > 1 || - !this.tlsContents.IsImpersonating)) + if (!Interop.Advapi32.AdjustTokenPrivileges( + this.tlsContents.ThreadHandle, + false, + &newState, + 0, + null, + null)) { - Interop.Advapi32.TOKEN_PRIVILEGE newState; - newState.PrivilegeCount = 1; - newState.Privileges.Luid = this.luid; - newState.Privileges.Attributes = (this.initialState ? Interop.Advapi32.SEPrivileges.SE_PRIVILEGE_ENABLED : Interop.Advapi32.SEPrivileges.SE_PRIVILEGE_DISABLED); - - if (!Interop.Advapi32.AdjustTokenPrivileges( - this.tlsContents.ThreadHandle, - false, - &newState, - 0, - null, - null)) - { - error = Marshal.GetLastWin32Error(); - success = false; - } + error = Marshal.GetLastWin32Error(); + success = false; } } - finally + } + finally + { + if (success) { - if (success) - { - this.Reset(); - } + this.Reset(); } } @@ -630,35 +581,6 @@ public unsafe void Revert() throw new InvalidOperationException(); } } -#if false - public static void RunWithPrivilege( string privilege, bool enabled, PrivilegedHelper helper ) - { - if ( helper == null ) - { - throw new ArgumentNullException( "helper" ); - } - - Privilege p = new Privilege( privilege ); - - try - { - if (enabled) - { - p.Enable(); - } - else - { - p.Disable(); - } - - helper(); - } - finally - { - p.Revert(); - } - } -#endif private void Reset() {