Skip to content

fix: WQL escaping, atomic lock operations, safe CTS disposal (CQ-007, CQ-008, CQ-015) #374

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: WQL escaping, atomic lock operations, safe CTS disposal (CQ-007, CQ-008, CQ-015) #374

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
11 changes: 11 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,17 @@ adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).

## [Unreleased]

## [0.48.12] - 2026-05-15

### Fixed
- **DiskHealthService (CQ-007)** — WQL ASSOCIATORS OF query now escapes single
quotes in objectId, preventing potential WQL injection.
- **OperationLockService (CQ-008)** — removed redundant lock object; TryAcquire
and Release now use ConcurrentDictionary atomic TryAdd/TryRemove directly.
- **PingMonitorService (CQ-015)** — CancellationTokenSource only disposed if the
background loop actually completed, preventing ObjectDisposedException in
still-running pump tasks.

## [0.48.11] - 2026-05-15

### Fixed
Expand Down
2 changes: 1 addition & 1 deletion SysManager/SysManager/Services/DiskHealthService.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ private static void EnrichWithReliability(ManagementScope scope, string objectId
try
{
// Escape quotes & backslashes for the WQL literal.
var safeId = objectId.Replace("\\", "\\\\").Replace("\"", "\\\"");
var safeId = objectId.Replace("\\", "\\\\").Replace("\"", "\\\"").Replace("'", "\\'");
var query = new ObjectQuery(
$"ASSOCIATORS OF {{MSFT_PhysicalDisk.ObjectId=\"{safeId}\"}} WHERE AssocClass=MSFT_PhysicalDiskToStorageReliabilityCounter");
using var searcher = new ManagementObjectSearcher(scope, query);
Expand Down
28 changes: 10 additions & 18 deletions SysManager/SysManager/Services/OperationLockService.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,6 @@ public sealed partial class OperationLockService : ObservableObject
public static OperationLockService Instance => _instance.Value;

private readonly ConcurrentDictionary<OperationCategory, OperationInfo> _active = new();
private readonly object _lock = new();

private OperationLockService() { }

Expand All @@ -47,17 +46,13 @@ private OperationLockService() { }
/// <returns>A disposable lock handle, or null if the category is busy.</returns>
public OperationHandle? TryAcquire(OperationCategory category, string operationName)
{
lock (_lock)
{
if (_active.ContainsKey(category))
return null;

var info = new OperationInfo(operationName, DateTime.UtcNow);
_active[category] = info;
OnPropertyChanged(nameof(ActiveOperations));
OnPropertyChanged(nameof(HasActiveOperations));
return new OperationHandle(this, category);
}
var info = new OperationInfo(operationName, DateTime.UtcNow);
if (!_active.TryAdd(category, info))
return null;

OnPropertyChanged(nameof(ActiveOperations));
OnPropertyChanged(nameof(HasActiveOperations));
return new OperationHandle(this, category);
}

/// <summary>
Expand All @@ -84,12 +79,9 @@ private OperationLockService() { }

private void Release(OperationCategory category)
{
lock (_lock)
{
_active.TryRemove(category, out _);
OnPropertyChanged(nameof(ActiveOperations));
OnPropertyChanged(nameof(HasActiveOperations));
}
_active.TryRemove(category, out _);
OnPropertyChanged(nameof(ActiveOperations));
OnPropertyChanged(nameof(HasActiveOperations));
}

/// <summary>
Expand Down
6 changes: 5 additions & 1 deletion SysManager/SysManager/Services/PingMonitorService.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,11 @@ public void Stop()
try { _loop?.Wait(1500); }
catch (AggregateException) { /* task cancellation or faulted — expected during stop */ }
catch (ObjectDisposedException) { /* task already cleaned up */ }
_cts?.Dispose();
// Only dispose CTS if the loop actually completed; otherwise the
// background task still holds a reference to the token and would
// throw ObjectDisposedException on next cancellation check.
if (_loop is { IsCompleted: true })
_cts?.Dispose();
_cts = null;
Comment on lines +59 to 64

@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 Stop() has no deferred CTS disposal path in the current branch.
sed -n '51,90p' SysManager/SysManager/Services/PingMonitorService.cs
rg -n --type=cs -C2 'ContinueWith|_cts\?\.Dispose\(' SysManager/SysManager/Services/PingMonitorService.cs

Repository: laurentiu021/SystemManager

Length of output: 1913

Defer CTS disposal until the background task actually completes.

The conditional disposal at line 62 only executes when _loop is already completed. However, if Wait(1500) times out, _loop is still running and the CTS is not disposed—the reference is then lost to null at line 64. This creates a resource leak on repeated Start/Stop cycles.

The background task still holds a reference to the cancellation token, so immediate disposal would cause an ObjectDisposedException on its next cancellation check. Instead, chain the disposal to execute after the loop truly completes:

Proposed fix 
-            if (_loop is { IsCompleted: true })
-                _cts?.Dispose();
+            if (_loop is { IsCompleted: true })
+            {
+                _cts?.Dispose();
+            }
+            else if (_loop != null && _cts != null)
+            {
+                var ctsToDispose = _cts;
+                _ = _loop.ContinueWith(
+                    _ => ctsToDispose.Dispose(),
+                    CancellationToken.None,
+                    TaskContinuationOptions.ExecuteSynchronously,
+                    TaskScheduler.Default);
+            }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
// Only dispose CTS if the loop actually completed; otherwise the
// background task still holds a reference to the token and would
// throw ObjectDisposedException on next cancellation check.
if (_loop is { IsCompleted: true })
_cts?.Dispose();
_cts = null;
// Only dispose CTS if the loop actually completed; otherwise the
// background task still holds a reference to the token and would
// throw ObjectDisposedException on next cancellation check.
if (_loop is { IsCompleted: true })
{
_cts?.Dispose();
}
else if (_loop != null && _cts != null)
{
var ctsToDispose = _cts;
_ = _loop.ContinueWith(
_ => ctsToDispose.Dispose(),
CancellationToken.None,
TaskContinuationOptions.ExecuteSynchronously,
TaskScheduler.Default);
}
_cts = null;
🤖 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/PingMonitorService.cs` around lines 59 - 64,
The current Stop logic disposes and nulls _cts prematurely when _loop is still
running, leaking the CTS on timeout; instead attach a continuation to the
background task (_loop) that disposes and nulls _cts only after the task
completes. Modify the Stop/cleanup code around symbols _loop and _cts so that if
Wait(1500) times out you do NOT drop _cts to null immediately but call
_loop?.ContinueWith(...) (or await/async completion) to perform _cts?.Dispose()
and set _cts = null once the task finishes, ensuring disposal runs exactly when
the background task has completed.

_loop = null;
}
Expand Down
Loading