GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
GeezerVison is showing github.com — Fix 6 PVS-Studio static analyzer findings by SufficientDaikon · Pull Request #27035 · PowerShell/PowerShell Conversation
|
| Item | File | Bug | Severity | Fix |
|---|---|---|---|---|
| 6 | ConsoleHost.cs |
RunspaceRef used before null check |
High | Moved null check first |
| 1 | CimGetInstance.cs |
GetCimInstanceParameter can return null, dereferenced |
High | Added null guard |
| 7 | typeDataQuery.cs |
vd.mainControl accessed when vd == null |
High | Split into separate checks |
| 9 | StringUtil.cs |
?? operator precedence — appendStr length silently ignored |
Medium | Added parentheses |
| 5 | ShowCommandCommandInfo.cs |
Members["Module"] without null-conditional |
Medium | Added ?. |
| 2 | CimCmdlets/Utils.cs |
Unsafe double-checked locking | Low | Added volatile |
// RunspaceRef used before null check - if (_isRunspacePushed) - return RunspaceRef.OldRunspace as LocalRunspace; - if (RunspaceRef == null) return null; + if (RunspaceRef == null) return null; + if (_isRunspacePushed) + return RunspaceRef.OldRunspace as LocalRunspace;
CimInstance instance = GetCimInstanceParameter(cmdlet); - nameSpace = instance.CimSystemProperties.Namespace; + if (instance != null) + { + nameSpace = instance.CimSystemProperties.Namespace; + ... + }
// vd.mainControl accessed when vd == null - if (vd == null || mainControlType != vd.mainControl.GetType()) - { - ActiveTracer.WriteLine("NOT MATCH ...", - ControlBase.GetControlShapeName(vd.mainControl), ...); - } + if (vd == null) + { + ActiveTracer.WriteLine("NOT MATCH null view definition"); + continue; + } + if (mainControlType != vd.mainControl.GetType()) + { + ActiveTracer.WriteLine("NOT MATCH ...", ...); + continue; + }
// ?? has lower priority than + - int capacity = length + prependStr?.Length ?? 0 + appendStr?.Length ?? 0; + int capacity = length + (prependStr?.Length ?? 0) + (appendStr?.Length ?? 0);
- this.Module = other.Members["Module"].Value as ShowCommandModuleInfo; + this.Module = other.Members["Module"]?.Value as ShowCommandModuleInfo;
- private static bool logInitialized = false; + private static volatile bool logInitialized = false;
Note
4 items intentionally left for maintainer guidance. Each requires a design decision beyond a mechanical fix.
| Item | File | Why Skipped |
|---|---|---|
| 3 | GetEventCommand.cs |
suppressOpener has no format placeholders — may be intentional design |
| 4 | New-Object.cs |
Missing FullLanguage switch case — may be deliberate fall-through |
| 8 | xmlSerializer.autogen.cs |
Auto-generated file — fix would be overwritten on next generation |
| 10 | Command.cs |
PipelineResultTypes Flags enum values — changing is a public API breaking change |
Addresses #25289 (items 1, 2, 5, 6, 7, 9). These are defensive null-checks and correctness fixes in code paths that would manifest as NullReferenceException crashes or silent miscalculation under specific conditions.
.h, .cpp, .cs, .ps1 and .psm1 files have the correct copyright header
Sorry, something went wrong.
There was an error while loading. Please reload this page.
Address items 1, 2, 5, 6, 7, 9 from the PVS-Studio report in PowerShell#25289: - Item 9: Fix ?? operator precedence in StringUtil.cs — appendStr length was silently ignored in capacity calculation - Item 2: Add volatile to logInitialized in CimCmdlets/Utils.cs for safe double-checked locking - Item 6: Move RunspaceRef null check before first use in ConsoleHost.cs to prevent NullReferenceException - Item 1: Add null guard for GetCimInstanceParameter return value in CimGetInstance.cs - Item 5: Use null-conditional on Members["Module"] access in ShowCommandCommandInfo.cs - Item 7: Split vd null check from type check in typeDataQuery.cs to prevent NullReferenceException in trace logging Items 3, 4, 8, 10 are intentionally left — they require design decisions, affect auto-generated code, or would be breaking changes. Addresses PowerShell#25289 Co-Authored-By: Claude Opus 4.6 <[email protected]>
514fb65 to
881d2bb
Compare
|
This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
This PR addresses a subset of the PVS-Studio findings from #25289 by applying targeted defensive fixes (null guards, corrected operator precedence, and a thread-safety adjustment) across several PowerShell components to prevent NullReferenceExceptions and subtle correctness issues.
Changes:
?? operator precedence in StringUtil.VtSubstring capacity calculation.volatile.Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/System.Management.Automation/utils/StringUtil.cs | Fixes ??/+ precedence to compute StringBuilder capacity correctly. |
| src/System.Management.Automation/FormatAndOutput/common/DisplayDatabase/typeDataQuery.cs | Splits null checks to avoid dereferencing vd when it is null and keeps tracing safe. |
| src/Microsoft.PowerShell.ConsoleHost/host/msh/ConsoleHost.cs | Reorders checks to avoid using RunspaceRef before verifying it is non-null. |
| src/Microsoft.PowerShell.Commands.Utility/commands/utility/ShowCommand/ShowCommandCommandInfo.cs | Uses null-conditional member access to avoid null dereference on missing "Module" member. |
| src/Microsoft.Management.Infrastructure.CimCmdlets/Utils.cs | Marks logInitialized as volatile to make double-checked locking safer. |
| src/Microsoft.Management.Infrastructure.CimCmdlets/CimGetInstance.cs | Guards CimInstance usage to prevent dereference when GetCimInstanceParameter returns null. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
At least 1 approving review is required to merge this pull request.
There was an error while loading. Please reload this page.
Successfully merging this pull request may close these issues.
There was an error while loading. Please reload this page.