From 57f77edb0e1274322bf7971e72e6366b19f80477 Mon Sep 17 00:00:00 2001 From: Jan Kotas Date: Mon, 9 Sep 2019 20:52:10 -0700 Subject: [PATCH] Delete dead code and fix issues found by static analysis tools (dotnet/coreclr#26604) Commit migrated from https://github.com/dotnet/coreclr/commit/961611fb7acb4f08a89011fec5d1d23f94743ce6 --- src/coreclr/src/debug/daccess/daccess.cpp | 5 +- src/coreclr/src/debug/di/divalue.cpp | 6 +- src/coreclr/src/debug/ee/debugger.h | 1 - src/coreclr/src/debug/ee/rcthread.cpp | 165 --------------------------- src/coreclr/src/inc/utilcode.h | 2 +- src/coreclr/src/md/enc/rwutil.cpp | 2 +- src/coreclr/src/utilcode/securitywrapper.cpp | 2 +- src/coreclr/src/utilcode/util.cpp | 2 +- src/coreclr/src/vm/compile.cpp | 2 +- src/coreclr/src/vm/profilinghelper.cpp | 76 +----------- src/coreclr/src/vm/profilinghelper.h | 9 -- 11 files changed, 13 insertions(+), 259 deletions(-) diff --git a/src/coreclr/src/debug/daccess/daccess.cpp b/src/coreclr/src/debug/daccess/daccess.cpp index 1e8aa3a..2d5a9e2 100644 --- a/src/coreclr/src/debug/daccess/daccess.cpp +++ b/src/coreclr/src/debug/daccess/daccess.cpp @@ -859,7 +859,10 @@ SplitName::FindType(IMDInternalImport* mdInternal) ULONG32 length; WCHAR wszName[MAX_CLASS_NAME]; - ConvertUtf8(m_typeName, MAX_CLASS_NAME, &length, wszName); + if (ConvertUtf8(m_typeName, MAX_CLASS_NAME, &length, wszName) != S_OK) + { + return false; + } WCHAR *pHead; diff --git a/src/coreclr/src/debug/di/divalue.cpp b/src/coreclr/src/debug/di/divalue.cpp index a8ef565..b0eb2f7 100644 --- a/src/coreclr/src/debug/di/divalue.cpp +++ b/src/coreclr/src/debug/di/divalue.cpp @@ -520,7 +520,7 @@ CordbGenericValue::CordbGenericValue(CordbAppDomain * pAppdomain, _ASSERTE(pType->m_elementType < ELEMENT_TYPE_MAX); // We can fill in the size now for generic values. - ULONG32 size; + ULONG32 size = 0; HRESULT hr; hr = pType->GetUnboxedObjectSize(&size); _ASSERTE (!FAILED(hr)); @@ -551,7 +551,7 @@ CordbGenericValue::CordbGenericValue(CordbType * pType) m_pValueHome(NULL) { // The only purpose of a literal value is to hold a RS literal value. - ULONG32 size; + ULONG32 size = 0; HRESULT hr; hr = pType->GetUnboxedObjectSize(&size); _ASSERTE (!FAILED(hr)); @@ -3413,7 +3413,7 @@ HRESULT CordbBoxValue::GetObject(ICorDebugObjectValue **ppObject) FAIL_IF_NEUTERED(this); ATT_REQUIRE_STOPPED_MAY_FAIL(GetProcess()); - ULONG32 size; + ULONG32 size = 0; m_type->GetUnboxedObjectSize(&size); HRESULT hr = S_OK; diff --git a/src/coreclr/src/debug/ee/debugger.h b/src/coreclr/src/debug/ee/debugger.h index 0dc855d..2f3de72 100644 --- a/src/coreclr/src/debug/ee/debugger.h +++ b/src/coreclr/src/debug/ee/debugger.h @@ -876,7 +876,6 @@ private: friend class Debugger; - HRESULT VerifySecurityOnRSCreatedEvents(HANDLE sse, HANDLE lsea, HANDLE lser); Debugger* m_debugger; // IPC_TARGET_* define default targets - if we ever want to do diff --git a/src/coreclr/src/debug/ee/rcthread.cpp b/src/coreclr/src/debug/ee/rcthread.cpp index c041831..1d50058 100644 --- a/src/coreclr/src/debug/ee/rcthread.cpp +++ b/src/coreclr/src/debug/ee/rcthread.cpp @@ -467,171 +467,6 @@ HRESULT DebuggerRCThread::Init(void) return S_OK; } -#ifndef FEATURE_PAL - -// This function is used to verify the security descriptor on an event -// matches our expectation to prevent attack. This should be called when -// we opened an event by name and assumed that the RS creates the event. -// That means the event's dacl should match our default policy - current user -// and admin. It can be narrower. By default, the DACL looks like the debugger -// process user, debuggee user, and admin. -// -HRESULT DebuggerRCThread::VerifySecurityOnRSCreatedEvents( - HANDLE sse, - HANDLE lsea, - HANDLE lser) -{ - - CONTRACTL - { - NOTHROW; - GC_NOTRIGGER; - } - CONTRACTL_END; - - STRESS_LOG0(LF_CORDB,LL_INFO1000,"DRCT::VerifySecurityOnRSCreatedEvents\n"); - - if (lsea == NULL || lser == NULL) - { - // no valid handle, does not need to verify. - // The caller will close the handles - return E_FAIL; - } - - HRESULT hr = S_OK; - - SIZE_T i; - ACCESS_ALLOWED_ACE *pAllowAceSSE = NULL; - ACCESS_ALLOWED_ACE *pAllowAceLSEA = NULL; - ACCESS_ALLOWED_ACE *pAllowAceLSER = NULL; - - - EX_TRY - { - // Get security descriptors for the handles. - Win32SecurityDescriptor sdSSE; - sdSSE.InitFromHandle(sse); - - Win32SecurityDescriptor sdLSEA; - sdLSEA.InitFromHandle(lsea); - - Win32SecurityDescriptor sdLSER; - sdLSER.InitFromHandle(lser); - - - - - // Make sure all 3 have the same creator - // We've already verifed in CreateSetupSyncEvent that the SSE's owner is in the DACL. - if (!Sid::Equals(sdSSE.GetOwner(), sdLSEA.GetOwner()) || - !Sid::Equals(sdSSE.GetOwner(), sdLSER.GetOwner())) - { - // Not equal! return now with failure code. - STRESS_LOG1(LF_CORDB,LL_INFO1000,"DRCT::VSORSCE failed on EqualSid - 0x%08x\n", hr); - ThrowHR(E_FAIL); - } - - // DACL_SECURITY_INFORMATION - // Now verify the DACL. It should be only two of them at most. One of them is the - // target process SID. - Dacl daclSSE = sdSSE.GetDacl(); - Dacl daclLSEA = sdLSEA.GetDacl(); - Dacl daclLSER = sdLSER.GetDacl(); - - - // Now all of these three ACL should be alike. There should be at most two of entries - // there. One if the debugger process's SID and one if debuggee sid. - if ((daclSSE.GetAceCount() != 1) && (daclSSE.GetAceCount() != 2)) - { - ThrowHR(E_FAIL); - } - - - // All of the ace count should equal for all events. - if ((daclSSE.GetAceCount() != daclLSEA.GetAceCount()) || - (daclSSE.GetAceCount() != daclLSER.GetAceCount())) - { - ThrowHR(E_FAIL); - } - - // Now check the ACE inside.These should be all equal - for (i = 0; i < daclSSE.GetAceCount(); i++) - { - ACE_HEADER *pAce; - - // Get the ace from the SSE - pAce = daclSSE.GetAce(i); - if (pAce->AceType != ACCESS_ALLOWED_ACE_TYPE) - { - ThrowHR(E_FAIL); - } - pAllowAceSSE = (ACCESS_ALLOWED_ACE*)pAce; - - // Get the ace from LSEA - pAce = daclLSEA.GetAce(i); - if (pAce->AceType != ACCESS_ALLOWED_ACE_TYPE) - { - ThrowHR(E_FAIL); - } - pAllowAceLSEA = (ACCESS_ALLOWED_ACE*)pAce; - - // This is the SID - // We can call EqualSid on this pAllowAce->SidStart - if (EqualSid((PSID)&(pAllowAceSSE->SidStart), (PSID)&(pAllowAceLSEA->SidStart)) == FALSE) - { - // ACE not equal. Fail out. - ThrowHR(E_FAIL); - } - - // Get the ace from LSER - pAce = daclLSER.GetAce(i); - if (pAce->AceType != ACCESS_ALLOWED_ACE_TYPE) - { - ThrowHR(E_FAIL); - } - pAllowAceLSER = (ACCESS_ALLOWED_ACE*)pAce; - - if (EqualSid((PSID)&(pAllowAceSSE->SidStart), (PSID)&(pAllowAceLSER->SidStart)) == FALSE) - { - // ACE not equal. Fail out. - ThrowHR(E_FAIL); - } - } // end for loop. - - - // The last ACE should be target process. That is it should be - // our process's sid! - // - if (pAllowAceLSER == NULL) - { - ThrowHR(E_FAIL);; // fail if we don't have the ACE. - } - { - SidBuffer sbCurrentProcess; - sbCurrentProcess.InitFromProcess(GetCurrentProcessId()); - if (!Sid::Equals(sbCurrentProcess.GetSid(), (PSID)&(pAllowAceLSER->SidStart))) - { - ThrowHR(E_FAIL); - } - } - } - EX_CATCH - { - // If we threw an exception, then the verification failed. - hr = E_FAIL; - } - EX_END_CATCH(RethrowTerminalExceptions); - - if (FAILED(hr)) - { - STRESS_LOG1(LF_CORDB,LL_INFO1000,"DRCT::VSORSCE failed with - 0x%08x\n", hr); - } - - return hr; -} - -#endif // FEATURE_PAL - //--------------------------------------------------------------------------------------- // // Setup the Runtime Offsets struct. diff --git a/src/coreclr/src/inc/utilcode.h b/src/coreclr/src/inc/utilcode.h index 04f08b7..a9c95c3 100644 --- a/src/coreclr/src/inc/utilcode.h +++ b/src/coreclr/src/inc/utilcode.h @@ -1366,7 +1366,7 @@ public: static BOOL GetLogicalProcessorInformationEx(LOGICAL_PROCESSOR_RELATIONSHIP relationship, SYSTEM_LOGICAL_PROCESSOR_INFORMATION_EX *slpiex, PDWORD count); static BOOL SetThreadGroupAffinity(HANDLE h, - GROUP_AFFINITY *groupAffinity, GROUP_AFFINITY *previousGroupAffinity); + const GROUP_AFFINITY *groupAffinity, GROUP_AFFINITY *previousGroupAffinity); static BOOL GetThreadGroupAffinity(HANDLE h, GROUP_AFFINITY *groupAffinity); static BOOL GetSystemTimes(FILETIME *idleTime, FILETIME *kernelTime, FILETIME *userTime); static void ChooseCPUGroupAffinity(GROUP_AFFINITY *gf); diff --git a/src/coreclr/src/md/enc/rwutil.cpp b/src/coreclr/src/md/enc/rwutil.cpp index e546f49..61eea45 100644 --- a/src/coreclr/src/md/enc/rwutil.cpp +++ b/src/coreclr/src/md/enc/rwutil.cpp @@ -458,7 +458,7 @@ HRESULT MDTOKENMAP::Init( } else { // It has tokens, so we may see a token for every row. - pITables->GetTableInfo(ixTbl, 0, &cRows, 0,0,0); + IfFailGo(pITables->GetTableInfo(ixTbl, 0, &cRows, 0,0,0)); // Safe: cTotal += cRows if (!ClrSafeInt::addition(cTotal, cRows, cTotal)) { diff --git a/src/coreclr/src/utilcode/securitywrapper.cpp b/src/coreclr/src/utilcode/securitywrapper.cpp index 6dc5d76..deb1ff0 100644 --- a/src/coreclr/src/utilcode/securitywrapper.cpp +++ b/src/coreclr/src/utilcode/securitywrapper.cpp @@ -110,7 +110,7 @@ HRESULT GetSidFromProcessWorker(DWORD dwProcessId, SidType sidType, PSID *ppSid) TOKEN_USER *pTokUser = NULL; HANDLE hProc = INVALID_HANDLE_VALUE; HANDLE hToken = INVALID_HANDLE_VALUE; - DWORD dwRetLength; + DWORD dwRetLength = 0; LPVOID pvTokenInfo = NULL; TOKEN_INFORMATION_CLASS tokenInfoClass; PSID pSidFromTokenInfo = NULL; diff --git a/src/coreclr/src/utilcode/util.cpp b/src/coreclr/src/utilcode/util.cpp index 1682bf3..6e1d2a5 100644 --- a/src/coreclr/src/utilcode/util.cpp +++ b/src/coreclr/src/utilcode/util.cpp @@ -824,7 +824,7 @@ BYTE * ClrVirtualAllocWithinRange(const BYTE *pMinAddr, } /*static*/ BOOL CPUGroupInfo::SetThreadGroupAffinity(HANDLE h, - GROUP_AFFINITY *groupAffinity, GROUP_AFFINITY *previousGroupAffinity) + const GROUP_AFFINITY *groupAffinity, GROUP_AFFINITY *previousGroupAffinity) { LIMITED_METHOD_CONTRACT; return ::SetThreadGroupAffinity(h, groupAffinity, previousGroupAffinity); diff --git a/src/coreclr/src/vm/compile.cpp b/src/coreclr/src/vm/compile.cpp index 321abae..294dabf 100644 --- a/src/coreclr/src/vm/compile.cpp +++ b/src/coreclr/src/vm/compile.cpp @@ -408,7 +408,7 @@ HRESULT CEECompileInfo::LoadTypeRefWinRT( { LPCSTR psznamespace; LPCSTR pszname; - pAssemblyImport->GetNameOfTypeRef(ref, &psznamespace, &pszname); + IfFailThrow(pAssemblyImport->GetNameOfTypeRef(ref, &psznamespace, &pszname)); AssemblySpec spec; spec.InitializeSpec(tkResolutionScope, pAssemblyImport, NULL); spec.SetWindowsRuntimeType(psznamespace, pszname); diff --git a/src/coreclr/src/vm/profilinghelper.cpp b/src/coreclr/src/vm/profilinghelper.cpp index 1ef25b4..5f989d9 100644 --- a/src/coreclr/src/vm/profilinghelper.cpp +++ b/src/coreclr/src/vm/profilinghelper.cpp @@ -284,13 +284,9 @@ void CurrentProfilerStatus::Set(ProfilerStatus newProfStatus) //--------------------------------------------------------------------------------------- // ProfilingAPIUtility members - // See code:#LoadUnloadCallbackSynchronization. CRITSEC_COOKIE ProfilingAPIUtility::s_csStatus = NULL; - -SidBuffer * ProfilingAPIUtility::s_pSidBuffer = NULL; - // ---------------------------------------------------------------------------- // ProfilingAPIUtility::AppendSupplementaryInformation // @@ -1464,14 +1460,7 @@ void ProfilingAPIUtility::TerminateProfiling() g_profControlBlock.pProfInterface.Store(NULL); } - // NOTE: Intentionally not deleting s_pSidBuffer. Doing so can cause annoying races - // with other threads that lazily create and initialize it when needed. (Example: - // it's used to fill out the "User" field of profiler event log entries.) Keeping - // s_pSidBuffer around after a profiler detaches and before a new one attaches - // consumes a bit more memory unnecessarily, but it'll get paged out if another - // profiler doesn't attach. - - // NOTE: Similarly, intentionally not destroying / NULLing s_csStatus. If + // NOTE: Intentionally not destroying / NULLing s_csStatus. If // s_csStatus is already initialized, we can reuse it each time we do another // attach / detach, so no need to destroy it. @@ -1494,67 +1483,4 @@ void ProfilingAPIUtility::TerminateProfiling() } } -#ifndef FEATURE_PAL - -// ---------------------------------------------------------------------------- -// ProfilingAPIUtility::GetCurrentProcessUserSid -// -// Description: -// Generates a SID of the current user from the current process's token. SID is -// returned in an [out] param, and is also cached for future use. The SID is used for -// two purposes: event log entries (for filling out the User field) and the ACL used -// on the globally named pipe object for attaching profilers. -// -// Arguments: -// * ppsid - [out] Generated (or cached) SID -// -// Return Value: -// HRESULT indicating success or failure. -// - -// static -HRESULT ProfilingAPIUtility::GetCurrentProcessUserSid(PSID * ppsid) -{ - CONTRACTL - { - NOTHROW; - GC_NOTRIGGER; - MODE_ANY; - } - CONTRACTL_END; - - if (s_pSidBuffer == NULL) - { - HRESULT hr; - NewHolder pSidBuffer(new (nothrow) SidBuffer); - if (pSidBuffer == NULL) - { - return E_OUTOFMEMORY; - } - - // This gets the SID of the user from the process token - hr = pSidBuffer->InitFromProcessUserNoThrow(GetCurrentProcessId()); - if (FAILED(hr)) - { - return hr; - } - - if (FastInterlockCompareExchangePointer( - &s_pSidBuffer, - pSidBuffer.GetValue(), - NULL) == NULL) - { - // Lifetime successfully transferred to s_pSidBuffer, so don't delete it here - pSidBuffer.SuppressRelease(); - } - } - - _ASSERTE(s_pSidBuffer != NULL); - _ASSERTE(s_pSidBuffer->GetSid().RawSid() != NULL); - *ppsid = s_pSidBuffer->GetSid().RawSid(); - return S_OK; -} - -#endif // !FEATURE_PAL - #endif // PROFILING_SUPPORTED diff --git a/src/coreclr/src/vm/profilinghelper.h b/src/coreclr/src/vm/profilinghelper.h index ee5f60a..62639fb 100644 --- a/src/coreclr/src/vm/profilinghelper.h +++ b/src/coreclr/src/vm/profilinghelper.h @@ -37,8 +37,6 @@ enum ProfAPIFaultFlags }; #endif // _DEBUG -class SidBuffer; - //--------------------------------------------------------------------------------------- // Static-only class to coordinate initialization of the various profiling API // structures, plus other utility stuff. @@ -73,9 +71,6 @@ public: static void LogProfInfo(int iStringResourceID, ...); static void LogNoInterfaceError(REFIID iidRequested, LPCWSTR wszClsid); INDEBUG(static BOOL ShouldInjectProfAPIFault(ProfAPIFaultFlags faultFlag);) -#ifndef FEATURE_PAL - static HRESULT GetCurrentProcessUserSid(PSID * ppsid); -#endif // !FEATURE_PAL #ifdef FEATURE_PROFAPI_ATTACH_DETACH // ---------------------------------------------------------------------------- @@ -129,10 +124,6 @@ private: kAttachLoad, }; - // Allocated lazily the first time it's needed, and then remains allocated until the - // process exits. - static SidBuffer * s_pSidBuffer; - // See code:ProfilingAPIUtility::InitializeProfiling#LoadUnloadCallbackSynchronization static CRITSEC_COOKIE s_csStatus; -- 2.7.4