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.

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,
+            __out PWSTR * pwszRuntimeVersion,
-            __deref_out_opt PWSTR * pwszConsoleHostAssemblyName);
+            __out PWSTR * pwszConsoleHostAssemblyName);
 
         unsigned int LaunchCoreCLR(
             ClrHostWrapper* hostWrapper,

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,
+        __out PWSTR * pwszRuntimeVersion,
-        __deref_out_opt PWSTR * pwszConsoleHostAssemblyName)
+        __out PWSTR * pwszConsoleHostAssemblyName)
     {
         return GetRegistryInfo(pwszMonadVersion,
             lpMonadMajorVersion,

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)

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)
+_Success_(return > 0)
-#pragma prefast (disable: 28196)
+DWORD GetFormattedErrorMessage(__out PWSTR * pwszErrorMessage, DWORD dwMessageId, va_list* arguments)
-DWORD GetFormattedErrorMessage(__deref_out PWSTR * pwszErrorMessage, DWORD dwMessageId, va_list* arguments)
-#pragma prefast(pop)
 {
     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)
         {
+            break;
+        }
+
         *pwszErrorMessage = new WCHAR[dwLength + 1];
-            if (NULL != *pwszErrorMessage)
+        if (*pwszErrorMessage == NULL)
         {
-                //string function
+            dwLength = 0;
+            break;
+        }
+
         if (FAILED(StringCchCopyW(*pwszErrorMessage, dwLength + 1, wszSystemErrorMessage)))
         {
             dwLength = 0;
             delete [] (*pwszErrorMessage);
             *pwszErrorMessage = NULL;
         }
-            }
-            LocalFree(wszSystemErrorMessage);
-        }
 
     }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);
+    if (GetFormattedErrorMessage(&pwszErrorMessage, errorCode) == 0)
-
-    result = WSManPluginOperationComplete(requestDetails, 0, errorCode, pwszErrorMessage);
-
-    if (NULL != pwszErrorMessage)
     {
+        /* 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;
     }
 

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);
 

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;
     }

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);
             }
         }

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;