-
Notifications
You must be signed in to change notification settings - Fork 8.1k
Fix static analysis issues: null checks, concurrency, and logic errors #26588
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
Technical Review - Critical Issue FoundThanks 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 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):
Action Items:
|
|
This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days. |
|
Thanks for the review. I've identified the critical logic error in New-Object.cs that needs to be fixed. Issue Analysis: The Root Cause: Correct Fix: I'll push a corrected version that:
Will update shortly after signing the CLA. |
|
@microsoft-github-policy-service agree |
Fix Static Analysis Issues
Summary
This PR addresses several static analysis issues identified by PVS-Studio to improve code stability and correctness.
Changes
instancebefore accessing properties (Fixes V3080).logInitializedasvolatileto fix unsafe double-checked locking (Fixes V3054).suppressOpener(Fixes V3025).PSLanguageMode.FullLanguagecase to switch statement (Fixes V3002).other.Members["Module"](Fixes V3095).RunspaceRefnull check inLocalRunspaceproperty (Fixes V3095).vd.mainControlin logging (Fixes V3095).PipelineResultTypes[Flags] enum (Fixes V3121).Verification
Checklist