From c78adfa55c62b2d13094faf2d97eb8d951b06699 Mon Sep 17 00:00:00 2001 From: Lakshmi Priya Sekar Date: Mon, 21 Sep 2015 23:32:54 -0700 Subject: [PATCH] Use PathString type to allocate large path strings on linux scenario. Respond to PR feedback. Fix test failures in mscoree. Commit migrated from https://github.com/dotnet/coreclr/commit/965e5d6480ebbff3d021665f95a37cb94eb9d56a --- src/coreclr/src/binder/assemblybinder.cpp | 5 +- src/coreclr/src/binder/cdebuglog.cpp | 1 - src/coreclr/src/binder/debuglog.cpp | 1 - src/coreclr/src/binder/inc/bindertypes.hpp | 2 - src/coreclr/src/binder/utils.cpp | 52 ++--------- src/coreclr/src/classlibnative/bcltype/system.cpp | 34 ++++--- src/coreclr/src/debug/ee/debugger.cpp | 4 +- src/coreclr/src/dlls/mscoree/mscoree.cpp | 104 +++++++++++----------- src/coreclr/src/inc/sstring.h | 9 ++ 9 files changed, 86 insertions(+), 126 deletions(-) diff --git a/src/coreclr/src/binder/assemblybinder.cpp b/src/coreclr/src/binder/assemblybinder.cpp index 3dc1d73..767df5f 100644 --- a/src/coreclr/src/binder/assemblybinder.cpp +++ b/src/coreclr/src/binder/assemblybinder.cpp @@ -164,7 +164,7 @@ namespace BINDER_SPACE } else { - PathString fullAssemblyPath; + SString fullAssemblyPath; WCHAR *pwzFullAssemblyPath = fullAssemblyPath.OpenUnicodeBuffer(MAX_LONGPATH); DWORD dwCCFullAssemblyPath = MAX_LONGPATH + 1; // SString allocates extra byte for null. @@ -184,9 +184,6 @@ namespace BINDER_SPACE { assemblyPath.Set(fullAssemblyPath); } - - // Now turn this path into our canonical representation - CanonicalizePath(assemblyPath); } return hr; diff --git a/src/coreclr/src/binder/cdebuglog.cpp b/src/coreclr/src/binder/cdebuglog.cpp index 114a42b..329d54f 100644 --- a/src/coreclr/src/binder/cdebuglog.cpp +++ b/src/coreclr/src/binder/cdebuglog.cpp @@ -334,7 +334,6 @@ namespace BINDER_SPACE CombinePath(g_BinderVariables->logPath, sCategory, logFilePath); CombinePath(logFilePath, m_applicationName, logFilePath); CombinePath(logFilePath, m_logFileName, logFilePath); - CanonicalizePath(logFilePath); BINDER_LOG_STRING(L"logFilePath", logFilePath); diff --git a/src/coreclr/src/binder/debuglog.cpp b/src/coreclr/src/binder/debuglog.cpp index 4f82b7b..2c4fd41 100644 --- a/src/coreclr/src/binder/debuglog.cpp +++ b/src/coreclr/src/binder/debuglog.cpp @@ -90,7 +90,6 @@ namespace BINDER_SPACE kCount2.u.HighPart); PlatformPath(logFilePath); - CanonicalizePath(logFilePath); } fFileExists = (FileOrDirectoryExists(logFilePath) == S_OK); diff --git a/src/coreclr/src/binder/inc/bindertypes.hpp b/src/coreclr/src/binder/inc/bindertypes.hpp index 62c6fa6..6fc4574 100644 --- a/src/coreclr/src/binder/inc/bindertypes.hpp +++ b/src/coreclr/src/binder/inc/bindertypes.hpp @@ -32,8 +32,6 @@ class PEAssembly; namespace BINDER_SPACE { - typedef InlineSString<512> PathString; - class AssemblyVersion; class AssemblyName; class Assembly; diff --git a/src/coreclr/src/binder/utils.cpp b/src/coreclr/src/binder/utils.cpp index a8e5cbc..a6a506d 100644 --- a/src/coreclr/src/binder/utils.cpp +++ b/src/coreclr/src/binder/utils.cpp @@ -260,40 +260,6 @@ namespace BINDER_SPACE BINDER_LOG_LEAVE(W("Utils::PlatformPath")); } - void CanonicalizePath(SString &path, BOOL fAppendPathSeparator) - { - BINDER_LOG_ENTER(W("Utils::CanonicalizePath")); - BINDER_LOG_STRING(W("input path"), path); - - if (!path.IsEmpty()) - { - PathString wszCanonicalPathString; - WCHAR * pwszCanonicalPath = wszCanonicalPathString.OpenUnicodeBuffer(MAX_LONGPATH); - PlatformPath(path); - - // This is also defined in rotor pal - if (PathCanonicalizeW(pwszCanonicalPath, path)) - { - path.Set(pwszCanonicalPath); - } - - wszCanonicalPathString.CloseBuffer(); - - if (fAppendPathSeparator) - { - SString platformPathSeparator(SString::Literal, GetPlatformPathSeparator()); - - if (!path.EndsWith(platformPathSeparator)) - { - path.Append(platformPathSeparator); - } - } - } - - BINDER_LOG_STRING(W("canonicalized path"), path); - BINDER_LOG_LEAVE(W("Utils::CanonicalizePath")); - } - void CombinePath(SString &pathA, SString &pathB, SString &combinedPath) @@ -303,20 +269,16 @@ namespace BINDER_SPACE BINDER_LOG_STRING(W("path A"), pathA); BINDER_LOG_STRING(W("path B"), pathB); - PathString tempResultPathString; - WCHAR * tempResultPath = tempResultPathString.OpenUnicodeBuffer(MAX_LONGPATH); - - if (PathCombineW(tempResultPath, pathA, pathB)) - { - combinedPath.Set(tempResultPath); - BINDER_LOG_STRING(W("combined path"), tempResultPath); - } - else + SString platformPathSeparator(SString::Literal, GetPlatformPathSeparator()); + combinedPath.Set(pathA); + + if (!combinedPath.EndsWith(platformPathSeparator)) { - combinedPath.Clear(); + combinedPath.Append(platformPathSeparator); } - tempResultPathString.CloseBuffer(); + combinedPath.Append(pathB); + BINDER_LOG_LEAVE(W("Utils::CombinePath")); } diff --git a/src/coreclr/src/classlibnative/bcltype/system.cpp b/src/coreclr/src/classlibnative/bcltype/system.cpp index d5772b2..3f27e07 100644 --- a/src/coreclr/src/classlibnative/bcltype/system.cpp +++ b/src/coreclr/src/classlibnative/bcltype/system.cpp @@ -24,6 +24,7 @@ #include "classnames.h" #include "system.h" #include "string.h" +#include "sstring.h" #include "eeconfig.h" #include "assemblynative.hpp" #include "generics.h" @@ -288,34 +289,29 @@ FCIMPL0(StringObject*, SystemNative::_GetModuleFileName) { FCALL_CONTRACT; - WCHAR wszFile[MAX_LONGPATH]; - STRINGREF refRetVal = NULL; - LPCWSTR pFileName = NULL; - DWORD lgth = 0; + STRINGREF refRetVal = NULL; + HELPER_METHOD_FRAME_BEGIN_RET_1(refRetVal); if (g_pCachedModuleFileName) { - pFileName = g_pCachedModuleFileName; - lgth = (DWORD)wcslen(pFileName); + refRetVal = StringObject::NewString(g_pCachedModuleFileName); } else { - HELPER_METHOD_FRAME_BEGIN_RET_1(refRetVal); - lgth = WszGetModuleFileName(NULL, wszFile, MAX_LONGPATH); + SString wszFilePathString; + + WCHAR * wszFile = wszFilePathString.OpenUnicodeBuffer(MAX_LONGPATH); + DWORD lgth = WszGetModuleFileName(NULL, wszFile, MAX_LONGPATH); if (!lgth) { COMPlusThrowWin32(); } - HELPER_METHOD_FRAME_END(); - pFileName = wszFile; - } + wszFilePathString.CloseBuffer(lgth); - if(lgth) - { - HELPER_METHOD_FRAME_BEGIN_RET_1(refRetVal); - refRetVal = StringObject::NewString(pFileName, lgth); - HELPER_METHOD_FRAME_END(); + refRetVal = StringObject::NewString(wszFilePathString.GetUnicode()); } + HELPER_METHOD_FRAME_END(); + return (StringObject*)OBJECTREFToObject(refRetVal); } FCIMPLEND @@ -347,14 +343,16 @@ FCIMPL0(StringObject*, SystemNative::GetRuntimeDirectory) { FCALL_CONTRACT; - wchar_t wszFile[MAX_LONGPATH+1]; STRINGREF refRetVal = NULL; DWORD dwFile = MAX_LONGPATH+1; HELPER_METHOD_FRAME_BEGIN_RET_1(refRetVal); + SString wszFilePathString; + WCHAR * wszFile = wszFilePathString.OpenUnicodeBuffer(dwFile); HRESULT hr = GetInternalSystemDirectory(wszFile, &dwFile); - + wszFilePathString.CloseBuffer(dwFile); + if(FAILED(hr)) COMPlusThrowHR(hr); diff --git a/src/coreclr/src/debug/ee/debugger.cpp b/src/coreclr/src/debug/ee/debugger.cpp index cb757fa..cb32f10 100644 --- a/src/coreclr/src/debug/ee/debugger.cpp +++ b/src/coreclr/src/debug/ee/debugger.cpp @@ -15136,7 +15136,8 @@ HRESULT Debugger::InitAppDomainIPC(void) } hEnsureCleanup(this); DWORD dwStrLen = 0; - WCHAR szExeName[MAX_LONGPATH]; + SString szExeNamePathString; + WCHAR * szExeName = szExeNamePathString.OpenUnicodeBuffer(MAX_LONGPATH); int i; // all fields in the object can be zero initialized. @@ -15188,6 +15189,7 @@ HRESULT Debugger::InitAppDomainIPC(void) szExeName, MAX_LONGPATH); + szExeNamePathString.CloseBuffer(dwStrLen); // If we couldn't get the name, then use a nice default. if (dwStrLen == 0) { diff --git a/src/coreclr/src/dlls/mscoree/mscoree.cpp b/src/coreclr/src/dlls/mscoree/mscoree.cpp index 0be663e..6a1af6d 100644 --- a/src/coreclr/src/dlls/mscoree/mscoree.cpp +++ b/src/coreclr/src/dlls/mscoree/mscoree.cpp @@ -792,7 +792,12 @@ BOOL PAL_GetPALDirectory(__out_ecount(cchBuffer) LPWSTR pbuffer, HRESULT hr = S_OK; - WCHAR pPath[MAX_LONGPATH]; + WCHAR * pPath = new (nothrow) WCHAR[MAX_LONGPATH]; + if (pPath == NULL) + { + return FALSE; + } + DWORD dwPath = MAX_LONGPATH; #ifndef CROSSGEN_COMPILE @@ -800,6 +805,7 @@ BOOL PAL_GetPALDirectory(__out_ecount(cchBuffer) LPWSTR pbuffer, #endif dwPath = WszGetModuleFileName(g_pMSCorEE, pPath, dwPath); + if(dwPath == 0) { hr = HRESULT_FROM_GetLastErrorNA(); @@ -809,7 +815,9 @@ BOOL PAL_GetPALDirectory(__out_ecount(cchBuffer) LPWSTR pbuffer, DWORD dwLength; hr = CopySystemDirectory(pPath, pbuffer, cchBuffer, &dwLength); } - + + delete [] pPath; + return (hr == S_OK); } @@ -1002,7 +1010,6 @@ ErrExit: // S_OK - Output buffer contains the version string. // HRESULT_FROM_WIN32(ERROR_INSUFFICIENT_BUFFER) - *pdwLength contains required size of the buffer in // characters. -// STDAPI GetCORVersionInternal( __out_ecount_z_opt(cchBuffer) LPWSTR pBuffer, @@ -1115,44 +1122,10 @@ __out_ecount_z_opt(cchBuffer) LPWSTR pBuffer, } - #ifndef CROSSGEN_COMPILE +#ifndef FEATURE_CORECLR STDAPI LoadLibraryShimInternal(LPCWSTR szDllName, LPCWSTR szVersion, LPVOID pvReserved, HMODULE *phModDll) { -#ifdef FEATURE_CORECLR - - CONTRACTL { - NOTHROW; - GC_NOTRIGGER; - PRECONDITION(CheckPointer(szDllName, NULL_OK)); - PRECONDITION(CheckPointer(szVersion, NULL_OK)); - PRECONDITION(CheckPointer(pvReserved, NULL_OK)); - PRECONDITION(CheckPointer(phModDll)); - } CONTRACTL_END; - - if (szDllName == NULL) - return E_POINTER; - - HRESULT hr = S_OK; - - BEGIN_ENTRYPOINT_NOTHROW; - - WCHAR szDllPath[MAX_LONGPATH+1]; - - if (!PAL_GetPALDirectoryW(szDllPath, MAX_LONGPATH)) { - IfFailGo(HRESULT_FROM_GetLastError()); - } - wcsncat_s(szDllPath, MAX_LONGPATH+1, szDllName, MAX_LONGPATH - wcslen(szDllPath)); - - if ((*phModDll = WszLoadLibrary(szDllPath)) == NULL) - IfFailGo(HRESULT_FROM_GetLastError()); - -ErrExit: - END_ENTRYPOINT_NOTHROW; - return hr; - -#else // FEATURE_CORECLR - // Simply forward the call to the ICLRRuntimeInfo implementation. STATIC_CONTRACT_WRAPPER; if (g_pCLRRuntime) @@ -1163,7 +1136,7 @@ ErrExit: { // no runtime info, probably loaded directly (e.g. from Fusion) // just look next to ourselves. - WCHAR wszPath[MAX_LONGPATH]; + WCHAR wszPath[MAX_PATH]; DWORD dwLength = WszGetModuleFileName(g_hThisInst, wszPath,NumItems(wszPath)); @@ -1188,15 +1161,12 @@ ErrExit: } return S_OK; } - -#endif // FEATURE_CORECLR - } - -#endif // CROSSGEN_COMPILE +#endif +#endif static DWORD g_dwSystemDirectory = 0; -static WCHAR g_pSystemDirectory[MAX_LONGPATH + 1]; +static WCHAR * g_pSystemDirectory = NULL; HRESULT GetInternalSystemDirectory(__out_ecount_part_opt(*pdwLength,*pdwLength) LPWSTR buffer, __inout DWORD* pdwLength) { @@ -1263,17 +1233,30 @@ HRESULT SetInternalSystemDirectory() DWORD len; // use local buffer for thread safety - WCHAR wzSystemDirectory[COUNTOF(g_pSystemDirectory)]; + NewArrayHolder wzSystemDirectory(new (nothrow) WCHAR[MAX_LONGPATH+1]); + if (wzSystemDirectory == NULL) + { + return HRESULT_FROM_WIN32(ERROR_NOT_ENOUGH_MEMORY); + } - hr = GetCORSystemDirectoryInternal(wzSystemDirectory, COUNTOF(wzSystemDirectory), &len); + hr = GetCORSystemDirectoryInternal(wzSystemDirectory, MAX_LONGPATH+1, &len); if(FAILED(hr)) { - wzSystemDirectory[0] = W('\0'); + *wzSystemDirectory = W('\0'); len = 1; } - + + WCHAR * pSystemDirectory = new (nothrow) WCHAR[len]; + if (pSystemDirectory == NULL) + { + return HRESULT_FROM_WIN32(ERROR_NOT_ENOUGH_MEMORY); + } + + wcscpy_s(pSystemDirectory, len, wzSystemDirectory); + // publish results idempotently with correct memory ordering - memcpy(g_pSystemDirectory, wzSystemDirectory, len * sizeof(WCHAR)); + g_pSystemDirectory = pSystemDirectory; + (void)InterlockedExchange((LONG *)&g_dwSystemDirectory, len); } @@ -1283,10 +1266,21 @@ HRESULT SetInternalSystemDirectory() #if defined(CROSSGEN_COMPILE) && defined(FEATURE_CORECLR) void SetMscorlibPath(LPCWSTR wzSystemDirectory) { - wcscpy_s(g_pSystemDirectory, COUNTOF(g_pSystemDirectory), wzSystemDirectory); - - DWORD len = (DWORD)wcslen(g_pSystemDirectory); - + DWORD len = (DWORD)wcslen(wzSystemDirectory); + if (g_dwSystemDirectory < len+1) + { + delete [] g_pSystemDirectory; + g_pSystemDirectory = new (nothrow) WCHAR[len+1]; + + if (g_pSystemDirectory == NULL) + { + SetLastError(ERROR_NOT_ENOUGH_MEMORY); + return; + } + } + + wcscpy_s(g_pSystemDirectory, len+1, wzSystemDirectory); + if(g_pSystemDirectory[len-1] != '\\') { g_pSystemDirectory[len] = W('\\'); @@ -1294,6 +1288,8 @@ void SetMscorlibPath(LPCWSTR wzSystemDirectory) g_dwSystemDirectory = len + 1; } else + { g_dwSystemDirectory = len; + } } #endif diff --git a/src/coreclr/src/inc/sstring.h b/src/coreclr/src/inc/sstring.h index a42d711..f4ae412 100644 --- a/src/coreclr/src/inc/sstring.h +++ b/src/coreclr/src/inc/sstring.h @@ -1006,6 +1006,15 @@ typedef InlineSString<512> StackSString; // be needed is small and it's preferable not to take up the stack space. typedef InlineSString<32> SmallStackSString; +// To be used specifically for path strings. +#ifdef _DEBUG +// This is a smaller version for debug builds to exercise the buffer allocation path +typedef InlineSString<32> PathString; +#else +// Set it to the current MAX_PATH +typedef InlineSString<260> PathString; +#endif + // ================================================================================ // Quick macro to create an SString around a literal string. // usage: -- 2.7.4