fix: parallel pipe reads, WQL validation, tighter regex#381
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📥 CommitsReviewing files that changed from the base of the PR and between a5f7913d50f54a5ffc3dd66193a0dde142af1423 and f7195d8. 📒 Files selected for processing (5)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (3)
📜 Recent review details⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
📝 WalkthroughWalkthroughPR 381 implements security and stability hardening across four services: DiskHealthService now validates WMI objectId before WQL interpolation; SpeedTestService switches to parallel stream reading to prevent pipe deadlocks; UninstallerService tightens PackageIdPattern to disallow unintended whitespace; WindowsFeaturesService adds SECURITY-CRITICAL documentation emphasizing FeatureNamePattern as the sole injection defense. ChangesSecurity and Stability Hardening v0.48.16
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@SysManager/SysManager/Services/SpeedTestService.cs`:
- Around line 209-220: Wrap the parallel stream reads (the
StandardOutput.ReadToEndAsync calls assigned to stdoutTask and stderrTask) and
the await proc.WaitForExitAsync(linked) inside a single try/catch/finally so
that any OperationCanceledException or other failure during Task.WhenAll will
run your cleanup; on catch (or finally if preferred) call
proc.Kill(entireProcessTree: true) (targeting the speedtest.exe child) and then
await proc.WaitForExitAsync() to ensure the process is reaped, and only after
successful cleanup read stdoutTask.Result and stderrTask.Result or handle their
cancellation/exception states accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 0ed746d3-a921-4089-98d8-3ed7d37a1826
📥 Commits
Reviewing files that changed from the base of the PR and between d6bf10b and a5f7913d50f54a5ffc3dd66193a0dde142af1423.
📒 Files selected for processing (5)
CHANGELOG.mdSysManager/SysManager/Services/DiskHealthService.csSysManager/SysManager/Services/SpeedTestService.csSysManager/SysManager/Services/UninstallerService.csSysManager/SysManager/Services/WindowsFeaturesService.cs
📜 Review details
🔇 Additional comments (4)
CHANGELOG.md (1)
9-23: LGTM!SysManager/SysManager/Services/DiskHealthService.cs (1)
6-6: LGTM!Also applies to: 72-79
SysManager/SysManager/Services/UninstallerService.cs (1)
68-68: LGTM!SysManager/SysManager/Services/WindowsFeaturesService.cs (1)
158-160: LGTM!
Comment on lines
+209
to
220
| // Read stdout and stderr in parallel to prevent pipe buffer deadlock. | ||
| // If one pipe fills while the other is being read sequentially, the | ||
| // child process blocks indefinitely (classic Windows pipe deadlock). | ||
| var stdoutTask = proc.StandardOutput.ReadToEndAsync(linked); | ||
| var stderrTask = proc.StandardError.ReadToEndAsync(linked); | ||
| await Task.WhenAll(stdoutTask, stderrTask).ConfigureAwait(false); | ||
| var stdout = stdoutTask.Result; | ||
| var stderr = stderrTask.Result; | ||
|
|
||
| try | ||
| { | ||
| await proc.WaitForExitAsync(linked); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether Task.WhenAll cancellation is covered by the same cleanup catch that kills the process tree.
# Expected: Task.WhenAll + WaitForExitAsync both appear within one try/catch(OperationCanceledException) path.
rg -n -C5 'Task\.WhenAll\(stdoutTask,\s*stderrTask\)|WaitForExitAsync\(linked\)|OperationCanceledException|Kill\(entireProcessTree:\s*true\)' --type csRepository: laurentiu021/SystemManager
Length of output: 50382
Move parallel pipe reads into the cancellation cleanup path to prevent orphaned processes.
If linked is canceled during Task.WhenAll at line 214, the exception is thrown before reaching the try/catch block at line 218, so the process cleanup never executes and speedtest.exe may remain running. Wrap the stream reads and exit wait in a single try/catch to ensure proc.Kill(entireProcessTree: true) always runs on cancellation.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@SysManager/SysManager/Services/SpeedTestService.cs` around lines 209 - 220,
Wrap the parallel stream reads (the StandardOutput.ReadToEndAsync calls assigned
to stdoutTask and stderrTask) and the await proc.WaitForExitAsync(linked) inside
a single try/catch/finally so that any OperationCanceledException or other
failure during Task.WhenAll will run your cleanup; on catch (or finally if
preferred) call proc.Kill(entireProcessTree: true) (targeting the speedtest.exe
child) and then await proc.WaitForExitAsync() to ensure the process is reaped,
and only after successful cleanup read stdoutTask.Result and stderrTask.Result
or handle their cancellation/exception states accordingly.
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
- SpeedTestService: read stdout/stderr in parallel with Task.WhenAll to prevent pipe buffer deadlock on Ookla CLI - DiskHealthService: validate objectId with regex before WQL interpolation (defense-in-depth against injection) - WindowsFeaturesService: add SECURITY-CRITICAL comment on FeatureNamePattern - UninstallerService: replace \s with literal space in PackageIdPattern (reject tabs/newlines in package IDs) - Add audit JSON files to .gitignore
laurentiu021
force-pushed
the
fix/process-deadlock-security
branch
from
May 15, 2026 08:59
a5f7913 to
f7195d8
Compare
3 tasks
laurentiu021
added a commit
that referenced
this pull request
May 22, 2026
…afety indicators (#476) ## Summary Adds a built-in JSON database of 107 common Windows processes and popular applications with plain-language descriptions, categories, and safety indicators to the Process Manager tab. ## Changes - **New**: \ProcessDescriptions.json\ — embedded resource with entries for system processes, browsers, dev tools, communication apps, media players, games, graphics drivers, productivity apps, creative tools, cloud services, utilities, network tools, and security software - **New**: \ProcessDescriptionService\ — singleton with fast case-insensitive lookup, .exe stripping, category and safety queries - **Extended**: \ProcessEntry\ model with \PlainDescription\, \Category\, \SafetyLevel\ fields - **Enhanced**: Process Manager filter now searches description and category - **Tests**: 10 unit tests for ProcessDescriptionService ## Categories System, Security, Browser, Development, Communication, Media, Gaming, Graphics, Productivity, Creative, Cloud, Utility, Network ## Safety Levels - **System** — known Windows component (safe, do not kill) - **Trusted** — well-known third-party application - **Unknown** — not in database Closes #381 Co-authored-by: laurentiu021 <laurentiu021@users.noreply.github.com>
laurentiu021
added a commit
that referenced
this pull request
May 22, 2026
- SpeedTestService: read stdout/stderr in parallel with Task.WhenAll to prevent pipe buffer deadlock on Ookla CLI - DiskHealthService: validate objectId with regex before WQL interpolation (defense-in-depth against injection) - WindowsFeaturesService: add SECURITY-CRITICAL comment on FeatureNamePattern - UninstallerService: replace \s with literal space in PackageIdPattern (reject tabs/newlines in package IDs) - Add audit JSON files to .gitignore Co-authored-by: laurentiu021 <laurentiu021@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes process deadlock and security defense-in-depth findings from the full codebase audit.
Pipe Deadlock Fix (H-04)
Task.WhenAllbeforeWaitForExitAsync.WQL Injection Defense (H-07)
PowerShell Injection Documentation (H-10)
Package ID Regex Tightening (SVC-50)
Build
Summary by CodeRabbit
Bug Fixes
Documentation