Skip to content

Conversation

@aaron-seq
Copy link

Fix Static Analysis Issues

Summary

This PR addresses several static analysis issues identified by PVS-Studio to improve code stability and correctness.

Changes

  • CimGetInstance.cs: Added null check for instance before accessing properties (Fixes V3080).
  • Utils.cs: Marked logInitialized as volatile to fix unsafe double-checked locking (Fixes V3054).
  • GetEventCommand.cs: Fixed incorrect format string in suppressOpener (Fixes V3025).
  • New-Object.cs: Added missing PSLanguageMode.FullLanguage case to switch statement (Fixes V3002).
  • ShowCommandCommandInfo.cs: Added null propagation for other.Members["Module"] (Fixes V3095).
  • ConsoleHost.cs: Reordered RunspaceRef null check in LocalRunspace property (Fixes V3095).
  • typeDataQuery.cs: Added safe access for vd.mainControl in logging (Fixes V3095).
  • StringUtil.cs: Added parentheses to fix operator precedence in string concatenation (Fixes V3123).
  • Command.cs: Added explicit initializers to PipelineResultTypes [Flags] enum (Fixes V3121).

Verification

  • Code changes were reviewed for correctness.
  • Note: A full build verification could not be completed in the current environment due to missing .NET SDK, but changes are targeted and low-risk.

Checklist

  • Null checks added where appropriate.
  • Logic errors corrected.
  • Concurrency issues addressed.

@aaron-seq
Copy link
Author

aaron-seq commented Dec 9, 2025

Technical Review - Critical Issue Found

Thanks for addressing these static analysis issues. I've reviewed all 9 file changes in detail. Most fixes are correct, but there's a critical logic error in New-Object.cs that breaks the code flow.

Critical Issue: New-Object.cs (Lines 192-211)

Problem: The SystemPolicy.LogWDACAuditMessage call is now placed outside the switch statement, causing it to execute for ALL language modes instead of only ConstrainedLanguage + Audit mode.

Original Logic:

case PSLanguageMode.ConstrainedLanguage:
    if (!CoreTypes.Contains(type))
    {
        if (SystemPolicy.GetSystemLockdownPolicy() != SystemEnforcementMode.Audit)
        {
            ThrowTerminatingError(...);
        }
        SystemPolicy.LogWDACAuditMessage(...); // Inside the if block
    }
    break;

Your Change: Moved the audit log call outside the switch, making it execute unconditionally after the switch completes.

Correct Fix: The switch statement needs proper structure:

case PSLanguageMode.ConstrainedLanguage:
    if (!CoreTypes.Contains(type))
    {
        if (SystemPolicy.GetSystemLockdownPolicy() != SystemEnforcementMode.Audit)
        {
            ThrowTerminatingError(...);
        }
        else // Only log in Audit mode
        {
            SystemPolicy.LogWDACAuditMessage(...);
        }
    }
    break;
case PSLanguageMode.FullLanguage:
    break;

Other File Analysis:

Correct Fixes (8 files):

  1. CimGetInstance.cs: Proper null check before accessing instance.CimSystemProperties.Namespace
  2. Utils.cs: Volatile keyword fixes double-checked locking race condition
  3. ShowCommandCommandInfo.cs: Null-conditional operator prevents NullReferenceException
  4. ConsoleHost.cs: Reordered null check logic is correct
  5. typeDataQuery.cs: Safe null check before accessing vd.mainControl
  6. StringUtil.cs: Parentheses fix operator precedence correctly
  7. Command.cs: Explicit enum values for [Flags] follow best practices
  8. GetEventCommand.cs: Need verification - added Path="{1}" placeholder but should confirm this format string is actually used with string.Format and receives 2 arguments

Action Items:

  1. Fix New-Object.cs logic - The audit message should only log when in Audit mode, not always
  2. Sign CLA - Respond to @microsoft-github-policy-service bot with: @microsoft-github-policy-service agree
  3. Build verification - Install .NET SDK and run full build before final submission
  4. Verify GetEventCommand.cs - Confirm the suppressOpener format string usage

@microsoft-github-policy-service microsoft-github-policy-service bot added the Review - Needed The PR is being reviewed label Dec 21, 2025
@microsoft-github-policy-service
Copy link
Contributor

This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days.
Maintainer, please provide feedback and/or mark it as Waiting on Author

@aaron-seq
Copy link
Author

Thanks for the review. I've identified the critical logic error in New-Object.cs that needs to be fixed.

Issue Analysis:

The SystemPolicy.LogWDACAuditMessage call at lines 210-215 is now orphaned outside the switch statement, causing it to execute for all language modes instead of only when in ConstrainedLanguage + Audit mode.

Root Cause:
When adding the PSLanguageMode.FullLanguage case to fix the V3002 warning, the code structure was incorrectly modified, leaving the audit log call outside its intended conditional block.

Correct Fix:
The audit message should only log when the system is in Audit mode within ConstrainedLanguage, not for all cases. The proper structure should check if we're in Audit mode and only then log, while still throwing errors in Enforce mode.

I'll push a corrected version that:

  1. Properly handles the ConstrainedLanguage mode with correct Audit vs Enforce logic
  2. Adds the FullLanguage case properly
  3. Keeps the audit logging inside the appropriate conditional block

Will update shortly after signing the CLA.

@aaron-seq
Copy link
Author

@microsoft-github-policy-service agree

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Review - Needed The PR is being reviewed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant