From da5d897823f4f79e2fedd976774a294e03e8a927 Mon Sep 17 00:00:00 2001 From: Dan Travison Date: Fri, 5 Jan 2018 16:53:27 -0800 Subject: [PATCH] SAL annotation updates & fix warnings (#5617) Various SAL annotations were either incorrect or not backed up by code. This PR address these issues. NOTE: By default, Start-BuildNativeWindowsBinaries does not enable code analysis and issues detected by SAL annotations are not reported. To identify these issues, run Start-BuildNativeWindowsBinaries to generate the solution and vcxproj files. Launch a Visual Studio developer prompt, cd to src\powershell-native and run msbuild manually. > msbuild PowerShellNative.sln /p:RunCodeAnalysis=true /p:Configuration=RelWithDebInfo /p:Platform=x64 The following changes address all code analysis warnings: - GetRegistryInfo in NativeMsh had incorrect out annotations, remove __opt - Fix handling of various out parameters - check for non-null and initialize - Update and Align SAL annotations for GetFormattedErrorMessage overloads - Allow PluginException to accept NULL message. --- .../nativemsh/pwrshcommon/NativeMsh.h | 16 +++-- .../nativemsh/pwrshcommon/pwrshcommon.cpp | 35 +++++++--- .../nativemsh/pwrshexe/CssMainEntry.cpp | 2 +- .../nativemsh/pwrshplugin/entrypoints.cpp | 66 ++++++++++++------- .../nativemsh/pwrshplugin/entrypoints.h | 2 +- .../nativemsh/pwrshplugin/pwrshclrhost.cpp | 5 +- .../nativemsh/pwrshplugin/pwrshplugin.h | 22 +++++-- .../nativemsh/pwrshplugin/pwrshplugindefs.h | 2 +- 8 files changed, 101 insertions(+), 49 deletions(-) diff --git a/src/powershell-native/nativemsh/pwrshcommon/NativeMsh.h b/src/powershell-native/nativemsh/pwrshcommon/NativeMsh.h index 26896d0df..d08620b73 100644 --- a/src/powershell-native/nativemsh/pwrshcommon/NativeMsh.h +++ b/src/powershell-native/nativemsh/pwrshcommon/NativeMsh.h @@ -78,25 +78,27 @@ namespace NativeMsh // Note: During successful calls the following values must be freed by the caller: // pwszMonadVersion // pwszRuntimeVersion - // pwzsRegKeyValue + // pwszRegKeyValue // // The caller must take care to check to see if they must be freed during error scenarios // because the function may fail after allocating one or more strings. // + _Success_(return == 0) unsigned int GetRegistryInfo( - __deref_out_opt PWSTR * pwszMonadVersion, + __out PWSTR * pwszMonadVersion, __inout_ecount(1) int * lpMonadMajorVersion, int monadMinorVersion, - __deref_out_opt PWSTR * pwszRuntimeVersion, + __out PWSTR * pwszRuntimeVersion, LPCWSTR lpszRegKeyNameToRead, - __deref_out_opt PWSTR * pwzsRegKeyValue); + __out PWSTR * pwszRegKeyValue); + _Success_(return == 0) unsigned int GetRegistryInfo( - __deref_out_opt PWSTR * pwszMonadVersion, + __out PWSTR * pwszMonadVersion, __inout_ecount(1) int * lpMonadMajorVersion, int monadMinorVersion, - __deref_out_opt PWSTR * pwszRuntimeVersion, - __deref_out_opt PWSTR * pwszConsoleHostAssemblyName); + __out PWSTR * pwszRuntimeVersion, + __out PWSTR * pwszConsoleHostAssemblyName); unsigned int LaunchCoreCLR( ClrHostWrapper* hostWrapper, diff --git a/src/powershell-native/nativemsh/pwrshcommon/pwrshcommon.cpp b/src/powershell-native/nativemsh/pwrshcommon/pwrshcommon.cpp index dce1a721f..81d9d903a 100644 --- a/src/powershell-native/nativemsh/pwrshcommon/pwrshcommon.cpp +++ b/src/powershell-native/nativemsh/pwrshcommon/pwrshcommon.cpp @@ -1238,18 +1238,19 @@ namespace NativeMsh // Note: During successful calls the following values must be freed by the caller: // pwszMonadVersion // pwszRuntimeVersion - // pwzsRegKeyValue + // pwszRegKeyValue // // The caller must take care to check to see if they must be freed during error scenarios // because the function may fail after allocating one or more strings. // + _Success_(return == 0) unsigned int PwrshCommon::GetRegistryInfo( - __deref_out_opt PWSTR * pwszMonadVersion, + __out PWSTR * pwszMonadVersion, __inout_ecount(1) int * lpMonadMajorVersion, int monadMinorVersion, - __deref_out_opt PWSTR * pwszRuntimeVersion, + __out PWSTR * pwszRuntimeVersion, LPCWSTR lpszRegKeyNameToRead, - __deref_out_opt PWSTR * pwzsRegKeyValue) + __out PWSTR * pwszRegKeyValue) { HKEY hEngineKey = NULL; bool bEngineKeyOpened = true; @@ -1257,12 +1258,27 @@ namespace NativeMsh wchar_t * wszMshEngineRegKeyPath = NULL; LPWSTR wszFullMonadVersion = NULL; + if (NULL != pwszRegKeyValue) + { + *pwszRegKeyValue = NULL; + } + + if (NULL != pwszRuntimeVersion) + { + *pwszRuntimeVersion = NULL; + } + + if (NULL != pwszMonadVersion) + { + *pwszMonadVersion = NULL; + } + do { if (NULL == pwszMonadVersion || NULL == lpMonadMajorVersion || NULL == pwszRuntimeVersion || - NULL == pwzsRegKeyValue) + NULL == pwszRegKeyValue) { exitCode = EXIT_CODE_READ_REGISTRY_FAILURE; break; @@ -1315,7 +1331,7 @@ namespace NativeMsh { LPCWSTR wszRequestedRegValueName = lpszRegKeyNameToRead; if (!this->RegQueryREG_SZValue(hEngineKey, wszRequestedRegValueName, - wszMshEngineRegKeyPath, pwzsRegKeyValue)) + wszMshEngineRegKeyPath, pwszRegKeyValue)) { exitCode = EXIT_CODE_READ_REGISTRY_FAILURE; break; @@ -1361,12 +1377,13 @@ namespace NativeMsh return exitCode; } + _Success_(return == 0) unsigned int PwrshCommon::GetRegistryInfo( - __deref_out_opt PWSTR * pwszMonadVersion, + __out PWSTR * pwszMonadVersion, __inout_ecount(1) int * lpMonadMajorVersion, int monadMinorVersion, - __deref_out_opt PWSTR * pwszRuntimeVersion, - __deref_out_opt PWSTR * pwszConsoleHostAssemblyName) + __out PWSTR * pwszRuntimeVersion, + __out PWSTR * pwszConsoleHostAssemblyName) { return GetRegistryInfo(pwszMonadVersion, lpMonadMajorVersion, diff --git a/src/powershell-native/nativemsh/pwrshexe/CssMainEntry.cpp b/src/powershell-native/nativemsh/pwrshexe/CssMainEntry.cpp index 113180536..7ea1bb6d9 100644 --- a/src/powershell-native/nativemsh/pwrshexe/CssMainEntry.cpp +++ b/src/powershell-native/nativemsh/pwrshexe/CssMainEntry.cpp @@ -217,7 +217,7 @@ int __cdecl wmain(const int argc, const wchar_t* argv[]) if (debug) { ::wprintf(L" Attach the debugger to powershell.exe and press any key to continue\n"); - ::getchar(); + (void) ::getchar(); } if (helpRequested) diff --git a/src/powershell-native/nativemsh/pwrshplugin/entrypoints.cpp b/src/powershell-native/nativemsh/pwrshplugin/entrypoints.cpp index ee7685a0c..74355bb6a 100644 --- a/src/powershell-native/nativemsh/pwrshplugin/entrypoints.cpp +++ b/src/powershell-native/nativemsh/pwrshplugin/entrypoints.cpp @@ -31,15 +31,18 @@ LPCWSTR g_MAIN_BINARY_NAME = L"pwrshplugin.dll"; // the caller should free pwszErrorMessage using LocalFree(). // returns: If the function succeeds the return value is the number of CHARs stored int the output // buffer, excluding the terminating null character. If the function fails the return value is zero. -#pragma prefast(push) -#pragma prefast (disable: 28196) -DWORD GetFormattedErrorMessage(__deref_out PWSTR * pwszErrorMessage, DWORD dwMessageId, va_list* arguments) -#pragma prefast(pop) +_Success_(return > 0) +DWORD GetFormattedErrorMessage(__out PWSTR * pwszErrorMessage, DWORD dwMessageId, va_list* arguments) { DWORD dwLength = 0; + LPWSTR wszSystemErrorMessage = NULL; do { + if (NULL == pwszErrorMessage) + { + break; + } *pwszErrorMessage = NULL; if (NULL == g_hResourceInstance) @@ -51,8 +54,6 @@ DWORD GetFormattedErrorMessage(__deref_out PWSTR * pwszErrorMessage, DWORD dwMes #endif } - LPWSTR wszSystemErrorMessage = NULL; - //string function dwLength = FormatMessageW( FORMAT_MESSAGE_FROM_HMODULE | FORMAT_MESSAGE_ALLOCATE_BUFFER, g_hResourceInstance, @@ -62,28 +63,36 @@ DWORD GetFormattedErrorMessage(__deref_out PWSTR * pwszErrorMessage, DWORD dwMes 0, arguments); - if (dwLength > 0) + if (dwLength == 0) { - *pwszErrorMessage = new WCHAR[dwLength + 1]; - if (NULL != *pwszErrorMessage) - { - //string function - if (FAILED(StringCchCopyW(*pwszErrorMessage, dwLength + 1, wszSystemErrorMessage))) - { - dwLength = 0; - delete [] (*pwszErrorMessage); - *pwszErrorMessage = NULL; - } - } - LocalFree(wszSystemErrorMessage); + break; + } + + *pwszErrorMessage = new WCHAR[dwLength + 1]; + if (*pwszErrorMessage == NULL) + { + dwLength = 0; + break; + } + + if (FAILED(StringCchCopyW(*pwszErrorMessage, dwLength + 1, wszSystemErrorMessage))) + { + dwLength = 0; + delete [] (*pwszErrorMessage); + *pwszErrorMessage = NULL; } }while(false); + if (NULL != wszSystemErrorMessage) + { + LocalFree(wszSystemErrorMessage); + } + return dwLength; } -DWORD GetFormattedErrorMessage(__deref_out PZPWSTR pwszErrorMessage, DWORD dwMessageId, ...) +DWORD GetFormattedErrorMessage(__out PWSTR * pwszErrorMessage, DWORD dwMessageId, ...) { DWORD result = 0; @@ -307,12 +316,19 @@ DWORD ReportOperationComplete(WSMAN_PLUGIN_REQUEST *requestDetails, DWORD errorC DWORD result = EXIT_CODE_SUCCESS; PWSTR pwszErrorMessage = NULL; - GetFormattedErrorMessage(&pwszErrorMessage, errorCode); - - result = WSManPluginOperationComplete(requestDetails, 0, errorCode, pwszErrorMessage); - - if (NULL != pwszErrorMessage) + if (GetFormattedErrorMessage(&pwszErrorMessage, errorCode) == 0) { + /* if an error message could not be loaded, generate a fallback + message to provide a level of diagnosability. + */ + WCHAR szFallbackMessageMessage[64] = L"\0"; + swprintf_s(szFallbackMessageMessage, _countof(szFallbackMessageMessage), L"ReportOperationComplete: %d", errorCode); + + result = WSManPluginOperationComplete(requestDetails, 0, errorCode, szFallbackMessageMessage); + } + else + { + result = WSManPluginOperationComplete(requestDetails, 0, errorCode, pwszErrorMessage); delete[] pwszErrorMessage; } diff --git a/src/powershell-native/nativemsh/pwrshplugin/entrypoints.h b/src/powershell-native/nativemsh/pwrshplugin/entrypoints.h index 90365cd48..c6105729b 100644 --- a/src/powershell-native/nativemsh/pwrshplugin/entrypoints.h +++ b/src/powershell-native/nativemsh/pwrshplugin/entrypoints.h @@ -13,6 +13,6 @@ const int EXIT_CODE_BAD_INPUT = 100; -extern DWORD GetFormattedErrorMessage(__deref_out PWSTR * pwszErrorMessage, DWORD dwMessageId, ...); +extern DWORD GetFormattedErrorMessage(__out PWSTR * pwszErrorMessage, DWORD dwMessageId, ...); extern unsigned int ConstructPowerShellVersion(int iPSMajorVersion, int iPSMinorVersion, __deref_out_opt PWSTR *pwszMonadVersion); diff --git a/src/powershell-native/nativemsh/pwrshplugin/pwrshclrhost.cpp b/src/powershell-native/nativemsh/pwrshplugin/pwrshclrhost.cpp index 1f2bcf085..edae970c2 100644 --- a/src/powershell-native/nativemsh/pwrshplugin/pwrshclrhost.cpp +++ b/src/powershell-native/nativemsh/pwrshplugin/pwrshclrhost.cpp @@ -270,7 +270,10 @@ unsigned int PowerShellClrWorker::LoadWorkerCallbackPtrs( { PWSTR msg = NULL; exitCode = g_MANAGED_METHOD_RESOLUTION_FAILED; - GetFormattedErrorMessage(&msg, exitCode); + /* NOTE: don't use a string literal for a fallback; PlugInException expects msg to allocated + and will free it but also supports NULL. + */ + (void) GetFormattedErrorMessage(&msg, exitCode); *pPluginException = new PlugInException(exitCode, msg); return exitCode; } diff --git a/src/powershell-native/nativemsh/pwrshplugin/pwrshplugin.h b/src/powershell-native/nativemsh/pwrshplugin/pwrshplugin.h index 5721e80ce..faf7b2378 100644 --- a/src/powershell-native/nativemsh/pwrshplugin/pwrshplugin.h +++ b/src/powershell-native/nativemsh/pwrshplugin/pwrshplugin.h @@ -490,6 +490,11 @@ private: { iMajorVersion = iVPSMajorVersion; + if (NULL != wszMgdPlugInFileName) + { + *wszMgdPlugInFileName = NULL; + } + exitCode = ConstructPowerShellVersion(iVPSMajorVersion, iVPSMinorVersion, &wszMonadVersion); if (EXIT_CODE_SUCCESS != exitCode) { @@ -561,10 +566,19 @@ private: _In_ PWSTR wszMgdPlugInFileName, _In_ PWSTR wszVCLRVersion, // Conditionally set to wszCLRVersion on success _In_ PWSTR wszVAppBase, // Conditionally set to wszAppBase on success - __out_opt PlugInException** pPluginException ) + __out PlugInException** pPluginException ) { unsigned int exitCode = EXIT_CODE_SUCCESS; + if (NULL != pPluginException) + { + *pPluginException = NULL; + } + else + { + return g_INVALID_INPUT; + } + if (bIsPluginLoaded) { return g_MANAGED_PLUGIN_ALREADY_LOADED; @@ -577,8 +591,6 @@ private: do { - *pPluginException = NULL; - // Setting global AppBase and CLR Version wszCLRVersion = wszVCLRVersion; wszAppBase = wszVAppBase; @@ -768,7 +780,9 @@ private: else { PWSTR msg = NULL; - GetFormattedErrorMessage(&msg, g_OPTION_SET_NOT_COMPLY, g_BUILD_VERSION); + (void) GetFormattedErrorMessage(&msg, g_OPTION_SET_NOT_COMPLY, g_BUILD_VERSION); + /* NOTE: Passing possible NULL msg. PlugInExpecption requires allocated + or NULL. Literal strings are not supported. */ throw new PlugInException(exitCode, msg); } } diff --git a/src/powershell-native/nativemsh/pwrshplugin/pwrshplugindefs.h b/src/powershell-native/nativemsh/pwrshplugin/pwrshplugindefs.h index b4b4cc081..7cfa8c5ce 100644 --- a/src/powershell-native/nativemsh/pwrshplugin/pwrshplugindefs.h +++ b/src/powershell-native/nativemsh/pwrshplugin/pwrshplugindefs.h @@ -140,7 +140,7 @@ public: // the memory. PWSTR extendedErrorInformation; - PlugInException(DWORD msgId, __in PWSTR msg) + PlugInException(DWORD msgId, __in_opt PWSTR msg) { dwMessageId = msgId; extendedErrorInformation = msg;