Skip to content

Commit da5d897

Browse files
dantraMSFTTravisEz13
dantraMSFT
authored and
TravisEz13
committed
SAL annotation updates & fix warnings (PowerShell#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.
1 parent 22f4729 commit da5d897

8 files changed

Lines changed: 100 additions & 48 deletions

File tree

‎src/powershell-native/nativemsh/pwrshcommon/NativeMsh.h‎

Lines changed: 9 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -78,25 +78,27 @@ namespace NativeMsh
7878
// Note: During successful calls the following values must be freed by the caller:
7979
// pwszMonadVersion
8080
// pwszRuntimeVersion
81-
// pwzsRegKeyValue
81+
// pwszRegKeyValue
8282
//
8383
// The caller must take care to check to see if they must be freed during error scenarios
8484
// because the function may fail after allocating one or more strings.
8585
//
86+
_Success_(return == 0)
8687
unsigned int GetRegistryInfo(
87-
__deref_out_opt PWSTR * pwszMonadVersion,
88+
__out PWSTR * pwszMonadVersion,
8889
__inout_ecount(1) int * lpMonadMajorVersion,
8990
int monadMinorVersion,
90-
__deref_out_opt PWSTR * pwszRuntimeVersion,
91+
__out PWSTR * pwszRuntimeVersion,
9192
LPCWSTR lpszRegKeyNameToRead,
92-
__deref_out_opt PWSTR * pwzsRegKeyValue);
93+
__out PWSTR * pwszRegKeyValue);
9394

95+
_Success_(return == 0)
9496
unsigned int GetRegistryInfo(
95-
__deref_out_opt PWSTR * pwszMonadVersion,
97+
__out PWSTR * pwszMonadVersion,
9698
__inout_ecount(1) int * lpMonadMajorVersion,
9799
int monadMinorVersion,
98-
__deref_out_opt PWSTR * pwszRuntimeVersion,
99-
__deref_out_opt PWSTR * pwszConsoleHostAssemblyName);
100+
__out PWSTR * pwszRuntimeVersion,
101+
__out PWSTR * pwszConsoleHostAssemblyName);
100102

101103
unsigned int LaunchCoreCLR(
102104
ClrHostWrapper* hostWrapper,

‎src/powershell-native/nativemsh/pwrshcommon/pwrshcommon.cpp‎

Lines changed: 26 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1238,31 +1238,47 @@ namespace NativeMsh
12381238
// Note: During successful calls the following values must be freed by the caller:
12391239
// pwszMonadVersion
12401240
// pwszRuntimeVersion
1241-
// pwzsRegKeyValue
1241+
// pwszRegKeyValue
12421242
//
12431243
// The caller must take care to check to see if they must be freed during error scenarios
12441244
// because the function may fail after allocating one or more strings.
12451245
//
1246+
_Success_(return == 0)
12461247
unsigned int PwrshCommon::GetRegistryInfo(
1247-
__deref_out_opt PWSTR * pwszMonadVersion,
1248+
__out PWSTR * pwszMonadVersion,
12481249
__inout_ecount(1) int * lpMonadMajorVersion,
12491250
int monadMinorVersion,
1250-
__deref_out_opt PWSTR * pwszRuntimeVersion,
1251+
__out PWSTR * pwszRuntimeVersion,
12511252
LPCWSTR lpszRegKeyNameToRead,
1252-
__deref_out_opt PWSTR * pwzsRegKeyValue)
1253+
__out PWSTR * pwszRegKeyValue)
12531254
{
12541255
HKEY hEngineKey = NULL;
12551256
bool bEngineKeyOpened = true;
12561257
unsigned int exitCode = EXIT_CODE_SUCCESS;
12571258
wchar_t * wszMshEngineRegKeyPath = NULL;
12581259
LPWSTR wszFullMonadVersion = NULL;
12591260

1261+
if (NULL != pwszRegKeyValue)
1262+
{
1263+
*pwszRegKeyValue = NULL;
1264+
}
1265+
1266+
if (NULL != pwszRuntimeVersion)
1267+
{
1268+
*pwszRuntimeVersion = NULL;
1269+
}
1270+
1271+
if (NULL != pwszMonadVersion)
1272+
{
1273+
*pwszMonadVersion = NULL;
1274+
}
1275+
12601276
do
12611277
{
12621278
if (NULL == pwszMonadVersion ||
12631279
NULL == lpMonadMajorVersion ||
12641280
NULL == pwszRuntimeVersion ||
1265-
NULL == pwzsRegKeyValue)
1281+
NULL == pwszRegKeyValue)
12661282
{
12671283
exitCode = EXIT_CODE_READ_REGISTRY_FAILURE;
12681284
break;
@@ -1315,7 +1331,7 @@ namespace NativeMsh
13151331
{
13161332
LPCWSTR wszRequestedRegValueName = lpszRegKeyNameToRead;
13171333
if (!this->RegQueryREG_SZValue(hEngineKey, wszRequestedRegValueName,
1318-
wszMshEngineRegKeyPath, pwzsRegKeyValue))
1334+
wszMshEngineRegKeyPath, pwszRegKeyValue))
13191335
{
13201336
exitCode = EXIT_CODE_READ_REGISTRY_FAILURE;
13211337
break;
@@ -1361,12 +1377,13 @@ namespace NativeMsh
13611377
return exitCode;
13621378
}
13631379

1380+
_Success_(return == 0)
13641381
unsigned int PwrshCommon::GetRegistryInfo(
1365-
__deref_out_opt PWSTR * pwszMonadVersion,
1382+
__out PWSTR * pwszMonadVersion,
13661383
__inout_ecount(1) int * lpMonadMajorVersion,
13671384
int monadMinorVersion,
1368-
__deref_out_opt PWSTR * pwszRuntimeVersion,
1369-
__deref_out_opt PWSTR * pwszConsoleHostAssemblyName)
1385+
__out PWSTR * pwszRuntimeVersion,
1386+
__out PWSTR * pwszConsoleHostAssemblyName)
13701387
{
13711388
return GetRegistryInfo(pwszMonadVersion,
13721389
lpMonadMajorVersion,

‎src/powershell-native/nativemsh/pwrshexe/CssMainEntry.cpp‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -217,7 +217,7 @@ int __cdecl wmain(const int argc, const wchar_t* argv[])
217217
if (debug)
218218
{
219219
::wprintf(L" Attach the debugger to powershell.exe and press any key to continue\n");
220-
::getchar();
220+
(void) ::getchar();
221221
}
222222

223223
if (helpRequested)

‎src/powershell-native/nativemsh/pwrshplugin/entrypoints.cpp‎

Lines changed: 40 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -31,15 +31,18 @@ LPCWSTR g_MAIN_BINARY_NAME = L"pwrshplugin.dll";
3131
// the caller should free pwszErrorMessage using LocalFree().
3232
// returns: If the function succeeds the return value is the number of CHARs stored int the output
3333
// buffer, excluding the terminating null character. If the function fails the return value is zero.
34-
#pragma prefast(push)
35-
#pragma prefast (disable: 28196)
36-
DWORD GetFormattedErrorMessage(__deref_out PWSTR * pwszErrorMessage, DWORD dwMessageId, va_list* arguments)
37-
#pragma prefast(pop)
34+
_Success_(return > 0)
35+
DWORD GetFormattedErrorMessage(__out PWSTR * pwszErrorMessage, DWORD dwMessageId, va_list* arguments)
3836
{
3937
DWORD dwLength = 0;
38+
LPWSTR wszSystemErrorMessage = NULL;
4039

4140
do
4241
{
42+
if (NULL == pwszErrorMessage)
43+
{
44+
break;
45+
}
4346
*pwszErrorMessage = NULL;
4447

4548
if (NULL == g_hResourceInstance)
@@ -51,8 +54,6 @@ DWORD GetFormattedErrorMessage(__deref_out PWSTR * pwszErrorMessage, DWORD dwMes
5154
#endif
5255
}
5356

54-
LPWSTR wszSystemErrorMessage = NULL;
55-
//string function
5657
dwLength = FormatMessageW(
5758
FORMAT_MESSAGE_FROM_HMODULE | FORMAT_MESSAGE_ALLOCATE_BUFFER,
5859
g_hResourceInstance,
@@ -62,28 +63,36 @@ DWORD GetFormattedErrorMessage(__deref_out PWSTR * pwszErrorMessage, DWORD dwMes
6263
0,
6364
arguments);
6465

65-
if (dwLength > 0)
66+
if (dwLength == 0)
6667
{
67-
*pwszErrorMessage = new WCHAR[dwLength + 1];
68-
if (NULL != *pwszErrorMessage)
69-
{
70-
//string function
71-
if (FAILED(StringCchCopyW(*pwszErrorMessage, dwLength + 1, wszSystemErrorMessage)))
72-
{
73-
dwLength = 0;
74-
delete [] (*pwszErrorMessage);
75-
*pwszErrorMessage = NULL;
76-
}
77-
}
78-
LocalFree(wszSystemErrorMessage);
68+
break;
69+
}
70+
71+
*pwszErrorMessage = new WCHAR[dwLength + 1];
72+
if (*pwszErrorMessage == NULL)
73+
{
74+
dwLength = 0;
75+
break;
76+
}
77+
78+
if (FAILED(StringCchCopyW(*pwszErrorMessage, dwLength + 1, wszSystemErrorMessage)))
79+
{
80+
dwLength = 0;
81+
delete [] (*pwszErrorMessage);
82+
*pwszErrorMessage = NULL;
7983
}
8084

8185
}while(false);
8286

87+
if (NULL != wszSystemErrorMessage)
88+
{
89+
LocalFree(wszSystemErrorMessage);
90+
}
91+
8392
return dwLength;
8493
}
8594

86-
DWORD GetFormattedErrorMessage(__deref_out PZPWSTR pwszErrorMessage, DWORD dwMessageId, ...)
95+
DWORD GetFormattedErrorMessage(__out PWSTR * pwszErrorMessage, DWORD dwMessageId, ...)
8796
{
8897
DWORD result = 0;
8998

@@ -307,12 +316,19 @@ DWORD ReportOperationComplete(WSMAN_PLUGIN_REQUEST *requestDetails, DWORD errorC
307316

308317
DWORD result = EXIT_CODE_SUCCESS;
309318
PWSTR pwszErrorMessage = NULL;
310-
GetFormattedErrorMessage(&pwszErrorMessage, errorCode);
311-
312-
result = WSManPluginOperationComplete(requestDetails, 0, errorCode, pwszErrorMessage);
319+
if (GetFormattedErrorMessage(&pwszErrorMessage, errorCode) == 0)
320+
{
321+
/* if an error message could not be loaded, generate a fallback
322+
message to provide a level of diagnosability.
323+
*/
324+
WCHAR szFallbackMessageMessage[64] = L"\0";
325+
swprintf_s(szFallbackMessageMessage, _countof(szFallbackMessageMessage), L"ReportOperationComplete: %d", errorCode);
313326

314-
if (NULL != pwszErrorMessage)
327+
result = WSManPluginOperationComplete(requestDetails, 0, errorCode, szFallbackMessageMessage);
328+
}
329+
else
315330
{
331+
result = WSManPluginOperationComplete(requestDetails, 0, errorCode, pwszErrorMessage);
316332
delete[] pwszErrorMessage;
317333
}
318334

‎src/powershell-native/nativemsh/pwrshplugin/entrypoints.h‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,6 @@
1313

1414
const int EXIT_CODE_BAD_INPUT = 100;
1515

16-
extern DWORD GetFormattedErrorMessage(__deref_out PWSTR * pwszErrorMessage, DWORD dwMessageId, ...);
16+
extern DWORD GetFormattedErrorMessage(__out PWSTR * pwszErrorMessage, DWORD dwMessageId, ...);
1717
extern unsigned int ConstructPowerShellVersion(int iPSMajorVersion, int iPSMinorVersion, __deref_out_opt PWSTR *pwszMonadVersion);
1818

‎src/powershell-native/nativemsh/pwrshplugin/pwrshclrhost.cpp‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -270,7 +270,10 @@ unsigned int PowerShellClrWorker::LoadWorkerCallbackPtrs(
270270
{
271271
PWSTR msg = NULL;
272272
exitCode = g_MANAGED_METHOD_RESOLUTION_FAILED;
273-
GetFormattedErrorMessage(&msg, exitCode);
273+
/* NOTE: don't use a string literal for a fallback; PlugInException expects msg to allocated
274+
and will free it but also supports NULL.
275+
*/
276+
(void) GetFormattedErrorMessage(&msg, exitCode);
274277
*pPluginException = new PlugInException(exitCode, msg);
275278
return exitCode;
276279
}

‎src/powershell-native/nativemsh/pwrshplugin/pwrshplugin.h‎

Lines changed: 18 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -490,6 +490,11 @@ class PwrshPlugInMediator
490490
{
491491
iMajorVersion = iVPSMajorVersion;
492492

493+
if (NULL != wszMgdPlugInFileName)
494+
{
495+
*wszMgdPlugInFileName = NULL;
496+
}
497+
493498
exitCode = ConstructPowerShellVersion(iVPSMajorVersion, iVPSMinorVersion, &wszMonadVersion);
494499
if (EXIT_CODE_SUCCESS != exitCode)
495500
{
@@ -561,10 +566,19 @@ class PwrshPlugInMediator
561566
_In_ PWSTR wszMgdPlugInFileName,
562567
_In_ PWSTR wszVCLRVersion, // Conditionally set to wszCLRVersion on success
563568
_In_ PWSTR wszVAppBase, // Conditionally set to wszAppBase on success
564-
__out_opt PlugInException** pPluginException )
569+
__out PlugInException** pPluginException )
565570
{
566571
unsigned int exitCode = EXIT_CODE_SUCCESS;
567572

573+
if (NULL != pPluginException)
574+
{
575+
*pPluginException = NULL;
576+
}
577+
else
578+
{
579+
return g_INVALID_INPUT;
580+
}
581+
568582
if (bIsPluginLoaded)
569583
{
570584
return g_MANAGED_PLUGIN_ALREADY_LOADED;
@@ -577,8 +591,6 @@ class PwrshPlugInMediator
577591

578592
do
579593
{
580-
*pPluginException = NULL;
581-
582594
// Setting global AppBase and CLR Version
583595
wszCLRVersion = wszVCLRVersion;
584596
wszAppBase = wszVAppBase;
@@ -768,7 +780,9 @@ class PwrshPlugInMediator
768780
else
769781
{
770782
PWSTR msg = NULL;
771-
GetFormattedErrorMessage(&msg, g_OPTION_SET_NOT_COMPLY, g_BUILD_VERSION);
783+
(void) GetFormattedErrorMessage(&msg, g_OPTION_SET_NOT_COMPLY, g_BUILD_VERSION);
784+
/* NOTE: Passing possible NULL msg. PlugInExpecption requires allocated
785+
or NULL. Literal strings are not supported. */
772786
throw new PlugInException(exitCode, msg);
773787
}
774788
}

‎src/powershell-native/nativemsh/pwrshplugin/pwrshplugindefs.h‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -140,7 +140,7 @@ class PlugInException
140140
// the memory.
141141
PWSTR extendedErrorInformation;
142142

143-
PlugInException(DWORD msgId, __in PWSTR msg)
143+
PlugInException(DWORD msgId, __in_opt PWSTR msg)
144144
{
145145
dwMessageId = msgId;
146146
extendedErrorInformation = msg;

0 commit comments

Comments
 (0)