Skip to content

fix: parallel pipe reads, WQL validation, tighter regex #381

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.

Sign up for GitHub

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

Merged
laurentiu021 merged 1 commit into from
May 15, 2026
Merged

fix: parallel pipe reads, WQL validation, tighter regex #381

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,21 @@ adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
handler from ViewModel on teardown. Previously 51 subscriptions leaked
permanently. `MainWindowViewModel.Dispose()` now disposes all NavItems.

## [0.48.16] - 2026-05-15

### Fixed
- **SpeedTestService** — stdout and stderr now read in parallel via
`Task.WhenAll` to prevent classic Windows pipe buffer deadlock when
Ookla CLI writes enough to stderr while stdout is being consumed.
- **DiskHealthService** — added regex validation (`^[\w{}\-\\.:/]+$`) on
WMI objectId before WQL interpolation (defense-in-depth against injection).
- **UninstallerService** — tightened `PackageIdPattern` regex: replaced `\s`
(which allows tabs/newlines) with a literal space character.

### Changed
- **WindowsFeaturesService** — added SECURITY-CRITICAL documentation comment
on `FeatureNamePattern()` regex explaining it is the sole injection defense.

## [0.48.14] - 2026-05-15

### Fixed
Expand Down
10 changes: 10 additions & 0 deletions SysManager/SysManager/Services/DiskHealthService.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
// License: MIT

using System.Management;
using Serilog;
using SysManager.Models;

namespace SysManager.Services;
Expand Down Expand Up @@ -68,6 +69,15 @@ private static void EnrichWithReliability(ManagementScope scope, string objectId
{
try
{
// Defense-in-depth: validate objectId format before WQL interpolation.
// ObjectId from MSFT_PhysicalDisk is typically like {guid}\\PhysicalDisk0.
// Reject anything that doesn't match expected characters.
if (!System.Text.RegularExpressions.Regex.IsMatch(objectId, @"^[\w{}\-\\.:/]+$"))
{
Log.Warning("DiskHealth: unexpected objectId format, skipping reliability query");
return;
}

// Escape quotes & backslashes for the WQL literal.
var safeId = objectId.Replace("\\", "\\\\").Replace("\"", "\\\"").Replace("'", "\\'");
var query = new ObjectQuery(
Expand Down
12 changes: 10 additions & 2 deletions SysManager/SysManager/Services/SpeedTestService.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -205,8 +205,16 @@ public async Task<SpeedTestResult> RunOoklaAsync(
using var proc = new Process();
proc.StartInfo = psi;
await Task.Run(() => proc.Start(), linked).ConfigureAwait(false);
var stdout = await proc.StandardOutput.ReadToEndAsync(linked);
var stderr = await proc.StandardError.ReadToEndAsync(linked);

// 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);
Comment on lines +209 to 220

@coderabbitai coderabbitai Bot May 15, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

🧩 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 cs

Repository: 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.

Expand Down
2 changes: 1 addition & 1 deletion SysManager/SysManager/Services/UninstallerService.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ public async Task<int> UninstallAsync(string packageId, CancellationToken ct = d
/// Matches valid winget package IDs: alphanumeric, dots, hyphens,
/// underscores, forward slashes, plus signs, and spaces. Max 256 chars.
/// </summary>
[System.Text.RegularExpressions.GeneratedRegex(@"^[\w.\-/+\s]{1,256}$")]
[System.Text.RegularExpressions.GeneratedRegex(@"^[\w.\-/+ ]{1,256}$")]
private static partial System.Text.RegularExpressions.Regex PackageIdPattern();

internal static List<InstalledApp> ParseListTable(List<string> lines)
Expand Down
3 changes: 3 additions & 0 deletions SysManager/SysManager/Services/WindowsFeaturesService.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,6 +155,9 @@ internal static string HumanizeName(string featureName)

/// <summary>
/// Validates feature names: alphanumeric, hyphens, underscores, dots. Max 128 chars.
/// SECURITY-CRITICAL: This regex is the sole defense against PowerShell injection
/// in Enable/DisableFeatureAsync where featureName is interpolated into a command.
/// Do NOT relax this pattern without reviewing injection implications.
/// </summary>
[GeneratedRegex(@"^[\w.\-]{1,128}$")]
private static partial Regex FeatureNamePattern();
Expand Down
Loading