SAL annotation updates & fix warnings #5617

dantraMSFT · 2017-12-04T18:50:15Z

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.

GetRegistryInfo in NativeMsh had incorrect out annotations, remove __opt Fix handling of various out parameters - check for non-null and initialize Udpate and Align SAL annotations for GetFormattedErrorMessage overloads Allow PluginException to accept NULL message.

daxian-dbw · 2017-12-04T19:00:42Z

Note that the psrp.windows nuget package needs to be regenerated. Make sure that the binaries are signed before using them to create the nuget package.

TravisEz13 · 2017-12-04T23:26:28Z

@dantraMSFT Can you please investigate test failures?

dantraMSFT · 2017-12-05T01:08:46Z

@TravisEz13: The failures in these two runs are unrelated to the change; they are timing issues. I've created #5627 to track the issue.

TravisEz13 · 2017-12-05T01:52:01Z

I've restarted windows and mac CI.

dantraMSFT · 2017-12-05T19:34:30Z

@daxian-dbw: I don't expect to be able to get the nuget package signed and published before I leave for vacation (EOD today).

daxian-dbw · 2017-12-06T18:43:18Z

@SteveL-MSFT @TravisEz13 Can you please reivew this PR to see if it's needed for RC2 or GA?

daxian-dbw · 2017-12-06T18:44:43Z

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

+
+        if (NULL != pwszMonadVersion)
+        {
+            pwszMonadVersion = NULL;


The two check above are assigning NULL to *<pointer> while this one is not. Is it intentional?

daxian-dbw · 2017-12-06T18:49:03Z

@mirichmo Can you please review this PR?

mirichmo · 2017-12-07T01:02:30Z

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


 DWORD ReportOperationComplete(WSMAN_PLUGIN_REQUEST *requestDetails, DWORD errorCode)
 {
+    WCHAR szMessage[256] = L"\0";


Why did you make this code change? It looks unnecessary and potentially error-prone.

It is incorrect to provide a fixed-length buffer for the error string. The string is read from pwrshplugin.dll and may be of any length. GetFormattedErrorMessage correctly handles the arbitrary length, but this change will potentially truncate it.

There is an assignment missing, which I'll add in the next commit; szMessage is used when GetFormmatedMessage fails.

The assignment does not address my comment. This change is fundamentally flawed because it will potentially truncate the string. I do not see a benefit over the original code.

mirichmo · 2017-12-07T01:03:51Z

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

    result = WSManPluginOperationComplete(requestDetails, 0, errorCode, pwszErrorMessage);

-    if (NULL != pwszErrorMessage)
+    if (NULL != pwszErrorMessage && pwszErrorMessage != szMessage)


This change will conditionally leak memory. If pwszErrorMessage has a value, it was allocated by GetFormattedErrorMessage and must be freed (via delete[]).

This has not been resolved yet.

mirichmo · 2017-12-07T01:09:30Z

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

        PWSTR msg = NULL;
        exitCode = g_MANAGED_METHOD_RESOLUTION_FAILED;
-        GetFormattedErrorMessage(&msg, exitCode);
+        /* NOTE: dont' use a string literal for a fallback; PlugInException expects msg to allocated


This comment seems unnecessary. The behavior is documented in the class definition.

I added this comment because the constructor annotations were wrong and, since the return value of GetFormmatedErrorMessage was being ignored, I added the comment to document the behavior as intentional.

mirichmo · 2017-12-07T01:11:58Z

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

        {
            iMajorVersion = iVPSMajorVersion;

+            if (NULL != wszMgdPlugInFileName)


This already occurs on line 671. It seems unnecessary to do it twice. Did the scanner require it?

This check is added because wszMgdPlugInFileName was not being set in all paths; specifically error paths.

mirichmo · 2017-12-07T01:16:32Z

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

-                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. */


Comment seems unnecessary here too

mirichmo · 2017-12-15T20:48:33Z

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

    result = WSManPluginOperationComplete(requestDetails, 0, errorCode, pwszErrorMessage);

-    if (NULL != pwszErrorMessage)
+    if (NULL != pwszErrorMessage && pwszErrorMessage != szMessage)


This has not been resolved yet.

mirichmo · 2017-12-15T20:49:54Z

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


 DWORD ReportOperationComplete(WSMAN_PLUGIN_REQUEST *requestDetails, DWORD errorCode)
 {
+    WCHAR szMessage[256] = L"\0";


The assignment does not address my comment. This change is fundamentally flawed because it will potentially truncate the string. I do not see a benefit over the original code.

dantraMSFT · 2017-12-20T01:35:31Z

@mirichmo: If you take a look at the code again, you'll see that szMessage is used to populate a placeholder error when when GetFormattedErrorMessage fails. It does not truncate an arbitrary error string.

As far as a possible leak, there isn't one.

When GetFormattedErrorMessage returns zero; pwszErrorMessage is null and, szMessage is used instead; thus, the delete call only frees memory when pwszErrorMessage is non-null and not szMessage.

Are you seeing something that I have missed?

SteveL-MSFT · 2018-01-03T20:50:33Z

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


 DWORD ReportOperationComplete(WSMAN_PLUGIN_REQUEST *requestDetails, DWORD errorCode)
 {
+    WCHAR szMessage[256] = L"\0";


After talking to Dan, szMessage contains a default error message in the case where the actual error message can't be allocated. So szMessage should be called something else like szFallbackErrorMessage making this more clear. Second, the 256 bytes is arbitrary. Since we know the max length based on line 315 below, we should only allocate that much.

Fixed.
Note that I left the stack buffer at 64.

SteveL-MSFT · 2018-01-03T20:51:58Z

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

+    if (GetFormattedErrorMessage(&pwszErrorMessage, errorCode) == 0)
+    {
+        swprintf_s(szMessage,_countof(szMessage), L"ReportOperationComplete: %d", errorCode);
+        pwszErrorMessage = szMessage;


Perhaps for readability of the code, it would be better to change this to call WSManPluginOperationComplete with szMessage and else use pwszErrorMessage?

This would also remove the change on line 321 below.

Update GetFormattedErrorMessage to ensure all error paths return dwLength = 0

dantraMSFT · 2018-01-03T22:59:13Z

@mirichmo, @SteveL-MSFT
I've updated ReportOperationComplete to clarify the stack buffer usage.
After another review, I found that GetFormattedErrorMessage was still not reliably returning zero for all error paths. I've rewritten it to be explicit in each case and avoid the nested If statements by leveraging the existing do/while(false) construct and moving the localfree call for wszSystemErrorMessage to the end of the function.

My comment #5617 (comment) has been addressed

TravisEz13 · 2018-01-05T22:29:44Z

@dantraMSFT Can you open an issue to publish a new package before I merge this so we don't lose the work?

dantraMSFT · 2018-01-05T22:58:33Z

@TravisEz13 See #5802

dantraMSFT requested review from TravisEz13, adityapatwardhan, daxian-dbw and mirichmo December 4, 2017 18:50

adityapatwardhan approved these changes Dec 4, 2017

View reviewed changes

daxian-dbw mentioned this pull request Dec 5, 2017

build: Enable building for win-arm and win-arm64 #5524

Merged

dantraMSFT assigned daxian-dbw Dec 5, 2017

daxian-dbw previously requested changes Dec 6, 2017

View reviewed changes

mirichmo suggested changes Dec 7, 2017

View reviewed changes

SteveL-MSFT added this to the 6.1.0-MQ milestone Dec 7, 2017

TravisEz13 changed the title ~~SAL annotation updates and fix warnings~~ SAL annotation updates & fix warnings Dec 8, 2017

Address PR feedback.

394d413

mirichmo suggested changes Dec 15, 2017

View reviewed changes

SteveL-MSFT requested changes Jan 3, 2018

View reviewed changes

Rewrite ReportOperationComplete for clarity

97e9982

Update GetFormattedErrorMessage to ensure all error paths return dwLength = 0

SteveL-MSFT approved these changes Jan 4, 2018

View reviewed changes

mirichmo approved these changes Jan 4, 2018

View reviewed changes

dantraMSFT mentioned this pull request Jan 5, 2018

publish updated psrp-windows nuget package to include SAL annotation changes. #5802

Closed

TravisEz13 merged commit da5d897 into PowerShell:master Jan 6, 2018

dantraMSFT deleted the dantra/codeanalysis branch March 8, 2018 19:14

SAL annotation updates & fix warnings #5617

SAL annotation updates & fix warnings #5617

Uh oh!

Conversation

dantraMSFT commented Dec 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

daxian-dbw commented Dec 4, 2017

Uh oh!

TravisEz13 commented Dec 4, 2017

Uh oh!

dantraMSFT commented Dec 5, 2017

Uh oh!

TravisEz13 commented Dec 5, 2017

Uh oh!

dantraMSFT commented Dec 5, 2017

Uh oh!

daxian-dbw commented Dec 6, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

daxian-dbw commented Dec 6, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dantraMSFT commented Dec 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dantraMSFT commented Jan 3, 2018

Uh oh!

TravisEz13 commented Jan 5, 2018

Uh oh!

dantraMSFT commented Jan 5, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

dantraMSFT commented Dec 4, 2017 •

edited

Loading

dantraMSFT commented Dec 20, 2017 •

edited

Loading