Profile manager types and profile editor dialog - #26
Conversation
Introduce ProfileManager and FanProfile serialization in Core, a Windows Task Scheduler helper for future automation hooks, and WinForms pieces (DarkMenuRenderer, ProfileEditorDialog) compiled into the GUI project. Form wiring can follow in a later change. Made-with: Cursor
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements a profile management system, enabling custom fan curves based on active processes. It adds core logic for profile handling, a Windows Task Scheduler helper for auto-start, and a GUI editor. Key feedback includes addressing thread-safety concerns in the profile manager, fixing potential serialization bugs with delimiters, optimizing process enumeration performance, and improving the robustness of UI-based profile removal.
|
|
||
| public class ProfileManager | ||
| { | ||
| private readonly List<FanProfile> _profiles = new List<FanProfile>(); |
There was a problem hiding this comment.
The _profiles list is accessed and modified from potentially different threads (e.g., the UI thread for adding/removing profiles and a background timer thread for CheckActiveProfile). This can lead to an InvalidOperationException if the collection is modified while being enumerated. Consider using a lock (similar to how it's done in FanCurve.cs) or a thread-safe collection.
| return $"{Name}|{curveStr}|{procStr}"; | ||
| } | ||
|
|
||
| public static FanProfile FromString(string data) | ||
| { | ||
| if (string.IsNullOrWhiteSpace(data)) return null; | ||
| var parts = data.Split('|'); |
There was a problem hiding this comment.
The serialization logic uses | and || as delimiters but does not escape or validate the Name property. If a profile name contains these characters, the parsing in FromString and LoadProfiles will fail. Consider using a standard format like JSON or XML, or implementing proper escaping for the delimiters.
|
|
||
| public void AddProfile(FanProfile profile) | ||
| { | ||
| _profiles.RemoveAll(p => p.Name == profile.Name); |
There was a problem hiding this comment.
Profile name comparisons are currently case-sensitive. In Windows applications, users generally expect names to be treated case-insensitively. This could lead to duplicate profiles with the same name but different casing (e.g., 'Gaming' and 'gaming').
_profiles.RemoveAll(p => string.Equals(p.Name, profile.Name, StringComparison.OrdinalIgnoreCase));| _profiles.RemoveAll(p => p.Name == name); | ||
| } | ||
|
|
||
| public FanProfile CheckActiveProfile(FanCurve defaultCurve) |
There was a problem hiding this comment.
The defaultCurve parameter is defined but never used within the CheckActiveProfile method.
public FanProfile CheckActiveProfile()| Process.GetProcesses().Select(p => | ||
| { | ||
| try { return p.ProcessName.ToLowerInvariant(); } | ||
| catch { return null; } | ||
| }).Where(n => n != null), | ||
| StringComparer.OrdinalIgnoreCase | ||
| ); |
There was a problem hiding this comment.
Using Process.GetProcesses() to check for running applications is an expensive operation as it enumerates all system processes and creates a Process object for each. If this is called frequently (e.g., in a high-frequency timer loop), it will cause significant CPU overhead. Consider caching the list of running process names for a short duration (e.g., 5 seconds) to avoid redundant system calls.
| foreach (var profile in _profiles) | ||
| { | ||
| if (profile.TriggerProcesses.Any(tp => | ||
| runningProcesses.Contains(tp.Replace(".exe", "").ToLowerInvariant()))) |
There was a problem hiding this comment.
The .Replace(".exe", "") call is case-sensitive. If a trigger process is defined as 'App.EXE', the extension will not be removed, potentially causing a mismatch. Additionally, ToLowerInvariant() is redundant here because the HashSet is already using OrdinalIgnoreCase.
| if (listProfiles.SelectedIndex >= 0 && listProfiles.SelectedIndex < _profileManager.Profiles.Count) | ||
| { | ||
| var profile = _profileManager.Profiles[listProfiles.SelectedIndex]; | ||
| _profileManager.RemoveProfile(profile.Name); |
There was a problem hiding this comment.
Removing a profile based on the SelectedIndex of the ListBox is fragile. It assumes the UI list order perfectly matches the internal _profiles list. If the list is ever sorted or filtered, this will result in the wrong profile being deleted. It is safer to retrieve the profile object directly from the list item or look it up by a unique identifier.
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| var psi = new ProcessStartInfo | ||
| { | ||
| FileName = "schtasks.exe", | ||
| Arguments = $"/Create /TN \"{TaskName}\" /TR \"\\\"{exePath}\\\"\" /SC ONLOGON /RL HIGHEST /F", |
There was a problem hiding this comment.
CRITICAL: Unvalidated exePath in RegisterTask creates security risk
Creating a scheduled task with highest privileges using an unvalidated exePath could allow arbitrary code execution if the path contains malicious content or is controlled by an attacker. Validate the path or sanitize input before passing to schtasks.exe.
| } | ||
| } | ||
| catch | ||
| { |
There was a problem hiding this comment.
WARNING: Silent exception handling in CheckActiveProfile
Catching all exceptions and swallowing them makes it difficult to diagnose issues with process enumeration. At least log the error somewhere. This could hide permission problems or system changes.
| return new FanProfile | ||
| { | ||
| Name = parts[0], | ||
| Curve = FanCurve.FromString(parts[1]), |
There was a problem hiding this comment.
SUGGESTION: Potential null curve in FromString method
When calling FanCurve.FromString(parts[1]), if it returns null, the Curve property will be null. This could cause NullReferenceException later if the code assumes profile.Curve is non-null. Consider adding a null check or default curve.
| foreach (var profile in _profiles) | ||
| { | ||
| if (profile.TriggerProcesses.Any(tp => | ||
| runningProcesses.Contains(tp.Replace(".exe", "").ToLowerInvariant()))) |
There was a problem hiding this comment.
SUGGESTION: Potential null reference in process matching lambda
The lambda tp => runningProcesses.Contains(tp.Replace(".exe", "").ToLowerInvariant()) could throw NullReferenceException if tp is null. While TriggerProcesses is typically initialized, it's safer to guard against null values (e.g., tp?.Replace(...)).
Code Review SummaryStatus: 4 Issues Found (No New Changes) | Recommendation: Address before merge Overview
Issue Details (click to expand)CRITICAL
WARNING
SUGGESTION
Files Reviewed (4 files)
Note: This is an incremental review. No new code changes were made since the previous review. Reviewed by trinity-large-thinking · 147,329 tokens |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@AsusFanControl.Core/ProfileManager.cs`:
- Around line 54-57: LoadProfiles clears the _profiles collection but leaves
_activeProfileName pointing to a potentially removed profile; update
LoadProfiles (and/or ActiveProfileName setter) to reset _activeProfileName when
reloading: after calling _profiles.Clear() (and when serialized is
null/whitespace) set _activeProfileName to null or an empty string, and if you
deserialize profiles ensure you validate the existing _activeProfileName against
the new collection and clear it if no match exists so ActiveProfileName never
references a non-loaded profile.
- Around line 73-76: The AddProfile method currently dereferences profile.Name
without validating profile; update AddProfile to guard against null by checking
the profile parameter (in the AddProfile method) and throw an
ArgumentNullException (or return/handle appropriately per project conventions)
when profile is null before accessing profile.Name or modifying _profiles;
ensure the null check is performed at the start of AddProfile to prevent
NullReferenceException.
- Around line 23-43: The current ToString/FromString pair (FanProfile.ToString
and FanProfile.FromString) assumes Name, Curve.ToString, and TriggerProcesses
entries never contain '|' or ';', which breaks parsing; fix by encoding/escaping
those fields instead of raw concatenation: either switch to a robust
serialization (e.g., JSON with System.Text.Json for FanProfile) or update
ToString to encode Name, Curve (or the result of Curve.ToString) and each
TriggerProcesses entry (e.g., Base64 or an escape function) and update
FromString to decode accordingly, ensuring FanCurve.FromString is fed the
decoded curve string and TriggerProcesses is reconstructed by splitting then
decoding each token.
- Around line 99-100: The lambda that checks trigger processes uses
tp.Replace(".exe", "") which is case-sensitive and removes internal occurrences;
change normalization to strip only a trailing extension and do case-insensitive
comparison — e.g., normalize each tp by using
Path.GetFileNameWithoutExtension(tp) (or if not using Path, check
tp.EndsWith(".exe", StringComparison.OrdinalIgnoreCase) and remove the last 4
chars) then ToLowerInvariant() and compare against runningProcesses; update the
ProfileManager check where profile.TriggerProcesses.Any(...) to use this
normalized value.
In `@AsusFanControl.Core/TaskSchedulerHelper.cs`:
- Around line 35-43: RegisterTask currently embeds exePath directly into
ProcessStartInfo.Arguments which allows malformed paths or injection; fix by
validating and properly escaping exePath: use Path.GetFullPath to normalize
exePath, reject or throw if it contains double quotes, newline or command
metacharacters (e.g., ; & | > <) to avoid injection, and then build the /TR
argument with a safely quoted path. If your target framework supports
ProcessStartInfo.ArgumentList, prefer adding each token separately (FileName
"schtasks.exe", add "/Create", "/TN", TaskName, "/TR", $"\"{safePath}\"", etc.)
to avoid manual quoting. Update the RegisterTask method to perform these
checks/normalization and use ArgumentList or sanitized quoting when constructing
Arguments.
- Around line 25-26: The calls to proc.WaitForExit(timeout) in
TaskSchedulerHelper.cs (where proc.WaitForExit and proc.ExitCode are used)
ignore the boolean return and may read ExitCode while the process is still
running; update each occurrence to capture the bool result (e.g., bool exited =
proc.WaitForExit(5000)), and if exited is false then kill the process
(proc.Kill() or proc.Kill(true) as appropriate), optionally call
proc.WaitForExit() again to ensure termination, and return a safe value (false
or throw) instead of reading proc.ExitCode when the timeout occurred; apply this
change for every place using proc.WaitForExit(...) followed by proc.ExitCode to
avoid unsafe reads.
In `@AsusFanControlGUI/ProfileEditorDialog.cs`:
- Around line 19-23: The constructor ProfileEditorDialog should validate its
dependency: add an explicit null check for the profileManager parameter at the
start of the ProfileEditorDialog(ProfileManager profileManager, FanCurve
defaultCurve) constructor and throw an ArgumentNullException (or similar) if
it's null to ensure predictable failure; keep assignment to _profileManager and
_defaultCurve and then call InitializeComponents() as before.
- Around line 194-199: The modal FanCurveEditor created in the
buttonEditCurve.Click handler is not disposed, risking UI resource leaks; change
the handler to create the FanCurveEditor in a disposal scope (e.g., using or
try/finally) so the editor is disposed after ShowDialog completes and still
assign _curve = editor.ResultCurve only when the dialog returned
DialogResult.OK, referencing the existing variables and methods:
buttonEditCurve.Click, FanCurveEditor, ShowDialog(), ResultCurve and _curve.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: f3dae693-853e-4e60-ab5f-a8ef59bc6613
📒 Files selected for processing (5)
AsusFanControl.Core/ProfileManager.csAsusFanControl.Core/TaskSchedulerHelper.csAsusFanControlGUI/AsusFanControlGUI.csprojAsusFanControlGUI/DarkMenuRenderer.csAsusFanControlGUI/ProfileEditorDialog.cs
| public override string ToString() | ||
| { | ||
| // Format: name|curveData|proc1;proc2;proc3 | ||
| var curveStr = Curve?.ToString() ?? ""; | ||
| var procStr = string.Join(";", TriggerProcesses); | ||
| return $"{Name}|{curveStr}|{procStr}"; | ||
| } | ||
|
|
||
| public static FanProfile FromString(string data) | ||
| { | ||
| if (string.IsNullOrWhiteSpace(data)) return null; | ||
| var parts = data.Split('|'); | ||
| if (parts.Length < 3) return null; | ||
|
|
||
| return new FanProfile | ||
| { | ||
| Name = parts[0], | ||
| Curve = FanCurve.FromString(parts[1]), | ||
| TriggerProcesses = parts[2].Split(new[] { ';' }, StringSplitOptions.RemoveEmptyEntries).ToList() | ||
| }; | ||
| } |
There was a problem hiding this comment.
Serialization format is fragile with unescaped delimiters.
Line 28 and Line 41 serialize user-provided values with | and ; delimiters but never escape/encode them. A profile name or process containing these characters will break round-trip parsing.
Proposed fix
+private static string Escape(string value) => Uri.EscapeDataString(value ?? string.Empty);
+private static string Unescape(string value) => Uri.UnescapeDataString(value ?? string.Empty);
public override string ToString()
{
var curveStr = Curve?.ToString() ?? "";
- var procStr = string.Join(";", TriggerProcesses);
- return $"{Name}|{curveStr}|{procStr}";
+ var procStr = string.Join(";", TriggerProcesses.Select(Escape));
+ return $"{Escape(Name)}|{Escape(curveStr)}|{procStr}";
}
public static FanProfile FromString(string data)
{
...
return new FanProfile
{
- Name = parts[0],
- Curve = FanCurve.FromString(parts[1]),
- TriggerProcesses = parts[2].Split(new[] { ';' }, StringSplitOptions.RemoveEmptyEntries).ToList()
+ Name = Unescape(parts[0]),
+ Curve = FanCurve.FromString(Unescape(parts[1])),
+ TriggerProcesses = parts[2]
+ .Split(new[] { ';' }, StringSplitOptions.RemoveEmptyEntries)
+ .Select(Unescape)
+ .ToList()
};
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@AsusFanControl.Core/ProfileManager.cs` around lines 23 - 43, The current
ToString/FromString pair (FanProfile.ToString and FanProfile.FromString) assumes
Name, Curve.ToString, and TriggerProcesses entries never contain '|' or ';',
which breaks parsing; fix by encoding/escaping those fields instead of raw
concatenation: either switch to a robust serialization (e.g., JSON with
System.Text.Json for FanProfile) or update ToString to encode Name, Curve (or
the result of Curve.ToString) and each TriggerProcesses entry (e.g., Base64 or
an escape function) and update FromString to decode accordingly, ensuring
FanCurve.FromString is fed the decoded curve string and TriggerProcesses is
reconstructed by splitting then decoding each token.
| public void LoadProfiles(string serialized) | ||
| { | ||
| _profiles.Clear(); | ||
| if (string.IsNullOrWhiteSpace(serialized)) return; |
There was a problem hiding this comment.
Reset active profile state when reloading profiles.
Line 56 clears _profiles but leaves _activeProfileName untouched. After reload, ActiveProfileName can reference a no-longer-loaded profile.
Proposed fix
public void LoadProfiles(string serialized)
{
_profiles.Clear();
+ _activeProfileName = null;
if (string.IsNullOrWhiteSpace(serialized)) return;📝 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.
| public void LoadProfiles(string serialized) | |
| { | |
| _profiles.Clear(); | |
| if (string.IsNullOrWhiteSpace(serialized)) return; | |
| public void LoadProfiles(string serialized) | |
| { | |
| _profiles.Clear(); | |
| _activeProfileName = null; | |
| if (string.IsNullOrWhiteSpace(serialized)) return; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@AsusFanControl.Core/ProfileManager.cs` around lines 54 - 57, LoadProfiles
clears the _profiles collection but leaves _activeProfileName pointing to a
potentially removed profile; update LoadProfiles (and/or ActiveProfileName
setter) to reset _activeProfileName when reloading: after calling
_profiles.Clear() (and when serialized is null/whitespace) set
_activeProfileName to null or an empty string, and if you deserialize profiles
ensure you validate the existing _activeProfileName against the new collection
and clear it if no match exists so ActiveProfileName never references a
non-loaded profile.
| public void AddProfile(FanProfile profile) | ||
| { | ||
| _profiles.RemoveAll(p => p.Name == profile.Name); | ||
| _profiles.Add(profile); |
There was a problem hiding this comment.
Guard against null profile input in AddProfile.
Line 75 dereferences profile.Name without validating profile, which can throw unexpectedly from public API usage.
Proposed fix
public void AddProfile(FanProfile profile)
{
+ if (profile == null) throw new ArgumentNullException(nameof(profile));
+ if (string.IsNullOrWhiteSpace(profile.Name)) throw new ArgumentException("Profile name is required.", nameof(profile));
_profiles.RemoveAll(p => p.Name == profile.Name);
_profiles.Add(profile);
}📝 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.
| public void AddProfile(FanProfile profile) | |
| { | |
| _profiles.RemoveAll(p => p.Name == profile.Name); | |
| _profiles.Add(profile); | |
| public void AddProfile(FanProfile profile) | |
| { | |
| if (profile == null) throw new ArgumentNullException(nameof(profile)); | |
| if (string.IsNullOrWhiteSpace(profile.Name)) throw new ArgumentException("Profile name is required.", nameof(profile)); | |
| _profiles.RemoveAll(p => p.Name == profile.Name); | |
| _profiles.Add(profile); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@AsusFanControl.Core/ProfileManager.cs` around lines 73 - 76, The AddProfile
method currently dereferences profile.Name without validating profile; update
AddProfile to guard against null by checking the profile parameter (in the
AddProfile method) and throw an ArgumentNullException (or return/handle
appropriately per project conventions) when profile is null before accessing
profile.Name or modifying _profiles; ensure the null check is performed at the
start of AddProfile to prevent NullReferenceException.
| if (profile.TriggerProcesses.Any(tp => | ||
| runningProcesses.Contains(tp.Replace(".exe", "").ToLowerInvariant()))) |
There was a problem hiding this comment.
Process-name normalization is incorrect for .EXE and non-suffix matches.
Line 100 uses tp.Replace(".exe", ""), which is case-sensitive and removes internal occurrences, not just a file-extension suffix. This causes false negatives/positives in trigger matching.
Proposed fix
+private static string NormalizeProcessName(string name)
+{
+ if (string.IsNullOrWhiteSpace(name)) return null;
+ var value = name.Trim();
+ if (value.EndsWith(".exe", StringComparison.OrdinalIgnoreCase))
+ value = value.Substring(0, value.Length - 4);
+ return value.ToLowerInvariant();
+}
...
if (profile.TriggerProcesses.Any(tp =>
- runningProcesses.Contains(tp.Replace(".exe", "").ToLowerInvariant())))
+ runningProcesses.Contains(NormalizeProcessName(tp))))📝 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.
| if (profile.TriggerProcesses.Any(tp => | |
| runningProcesses.Contains(tp.Replace(".exe", "").ToLowerInvariant()))) | |
| private static string NormalizeProcessName(string name) | |
| { | |
| if (string.IsNullOrWhiteSpace(name)) return null; | |
| var value = name.Trim(); | |
| if (value.EndsWith(".exe", StringComparison.OrdinalIgnoreCase)) | |
| value = value.Substring(0, value.Length - 4); | |
| return value.ToLowerInvariant(); | |
| } | |
| ... | |
| if (profile.TriggerProcesses.Any(tp => | |
| runningProcesses.Contains(NormalizeProcessName(tp)))) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@AsusFanControl.Core/ProfileManager.cs` around lines 99 - 100, The lambda that
checks trigger processes uses tp.Replace(".exe", "") which is case-sensitive and
removes internal occurrences; change normalization to strip only a trailing
extension and do case-insensitive comparison — e.g., normalize each tp by using
Path.GetFileNameWithoutExtension(tp) (or if not using Path, check
tp.EndsWith(".exe", StringComparison.OrdinalIgnoreCase) and remove the last 4
chars) then ToLowerInvariant() and compare against runningProcesses; update the
ProfileManager check where profile.TriggerProcesses.Any(...) to use this
normalized value.
| proc.WaitForExit(5000); | ||
| return proc.ExitCode == 0; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify timeout waits are followed by timeout handling before ExitCode usage.
rg -nP --type=cs -C2 'WaitForExit\(\d+\);\s*\n\s*return\s+proc\.ExitCode\s*==\s*0;' AsusFanControl.Core/TaskSchedulerHelper.cs
# Expected after fix: no matches.Repository: beenycool/AsusFanControl
Length of output: 50
🏁 Script executed:
cat -n AsusFanControl.Core/TaskSchedulerHelper.cs | sed -n '1,100p'Repository: beenycool/AsusFanControl
Length of output: 3188
Handle WaitForExit timeouts before reading ExitCode.
The return value of WaitForExit(...) is ignored at lines 25, 50, and 75. When the timeout elapses, the process remains running and ExitCode is unsafe to read. Check the return value and kill the process on timeout:
Proposed fix
using (var proc = Process.Start(psi))
{
- proc.WaitForExit(5000);
- return proc.ExitCode == 0;
+ if (!proc.WaitForExit(5000))
+ {
+ try { proc.Kill(); } catch { }
+ return false;
+ }
+ return proc.ExitCode == 0;
}Also applies to: lines 50-51, 75-76
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@AsusFanControl.Core/TaskSchedulerHelper.cs` around lines 25 - 26, The calls
to proc.WaitForExit(timeout) in TaskSchedulerHelper.cs (where proc.WaitForExit
and proc.ExitCode are used) ignore the boolean return and may read ExitCode
while the process is still running; update each occurrence to capture the bool
result (e.g., bool exited = proc.WaitForExit(5000)), and if exited is false then
kill the process (proc.Kill() or proc.Kill(true) as appropriate), optionally
call proc.WaitForExit() again to ensure termination, and return a safe value
(false or throw) instead of reading proc.ExitCode when the timeout occurred;
apply this change for every place using proc.WaitForExit(...) followed by
proc.ExitCode to avoid unsafe reads.
| public static bool RegisterTask(string exePath) | ||
| { | ||
| try | ||
| { | ||
| var psi = new ProcessStartInfo | ||
| { | ||
| FileName = "schtasks.exe", | ||
| Arguments = $"/Create /TN \"{TaskName}\" /TR \"\\\"{exePath}\\\"\" /SC ONLOGON /RL HIGHEST /F", | ||
| UseShellExecute = false, |
There was a problem hiding this comment.
Harden /TR command construction against malformed exePath.
Line 42 embeds exePath directly into command arguments. A path containing quotes or invalid content can break task creation semantics and may allow argument injection into the scheduled task action.
Proposed fix
+using System.IO;
...
public static bool RegisterTask(string exePath)
{
+ if (string.IsNullOrWhiteSpace(exePath)) return false;
+ if (exePath.Contains("\"")) return false;
+ var fullExePath = Path.GetFullPath(exePath);
+ if (!File.Exists(fullExePath)) return false;
+
try
{
var psi = new ProcessStartInfo
{
FileName = "schtasks.exe",
- Arguments = $"/Create /TN \"{TaskName}\" /TR \"\\\"{exePath}\\\"\" /SC ONLOGON /RL HIGHEST /F",
+ Arguments = $"/Create /TN \"{TaskName}\" /TR \"\\\"{fullExePath}\\\"\" /SC ONLOGON /RL HIGHEST /F",
UseShellExecute = false,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@AsusFanControl.Core/TaskSchedulerHelper.cs` around lines 35 - 43,
RegisterTask currently embeds exePath directly into ProcessStartInfo.Arguments
which allows malformed paths or injection; fix by validating and properly
escaping exePath: use Path.GetFullPath to normalize exePath, reject or throw if
it contains double quotes, newline or command metacharacters (e.g., ; & | > <)
to avoid injection, and then build the /TR argument with a safely quoted path.
If your target framework supports ProcessStartInfo.ArgumentList, prefer adding
each token separately (FileName "schtasks.exe", add "/Create", "/TN", TaskName,
"/TR", $"\"{safePath}\"", etc.) to avoid manual quoting. Update the RegisterTask
method to perform these checks/normalization and use ArgumentList or sanitized
quoting when constructing Arguments.
| public ProfileEditorDialog(ProfileManager profileManager, FanCurve defaultCurve) | ||
| { | ||
| _profileManager = profileManager; | ||
| _defaultCurve = defaultCurve; | ||
| InitializeComponents(); |
There was a problem hiding this comment.
Validate constructor dependency early.
Line 21 assumes profileManager is non-null. Add an explicit guard for predictable failure semantics.
Proposed fix
public ProfileEditorDialog(ProfileManager profileManager, FanCurve defaultCurve)
{
+ if (profileManager == null) throw new ArgumentNullException(nameof(profileManager));
_profileManager = profileManager;
_defaultCurve = defaultCurve;📝 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.
| public ProfileEditorDialog(ProfileManager profileManager, FanCurve defaultCurve) | |
| { | |
| _profileManager = profileManager; | |
| _defaultCurve = defaultCurve; | |
| InitializeComponents(); | |
| public ProfileEditorDialog(ProfileManager profileManager, FanCurve defaultCurve) | |
| { | |
| if (profileManager == null) throw new ArgumentNullException(nameof(profileManager)); | |
| _profileManager = profileManager; | |
| _defaultCurve = defaultCurve; | |
| InitializeComponents(); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@AsusFanControlGUI/ProfileEditorDialog.cs` around lines 19 - 23, The
constructor ProfileEditorDialog should validate its dependency: add an explicit
null check for the profileManager parameter at the start of the
ProfileEditorDialog(ProfileManager profileManager, FanCurve defaultCurve)
constructor and throw an ArgumentNullException (or similar) if it's null to
ensure predictable failure; keep assignment to _profileManager and _defaultCurve
and then call InitializeComponents() as before.
| buttonEditCurve.Click += (s, e) => | ||
| { | ||
| var editor = new FanCurveEditor(_curve); | ||
| if (editor.ShowDialog() == DialogResult.OK) | ||
| _curve = editor.ResultCurve; | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify modal dialogs are created with disposal in this file.
rg -nP --type=cs -C2 'new\s+FanCurveEditor\s*\(' AsusFanControlGUI/ProfileEditorDialog.cs
# Expected after fix: creation exists inside a `using (...)` block.Repository: beenycool/AsusFanControl
Length of output: 314
🏁 Script executed:
#!/bin/bash
# Find FanCurveEditor class definition
find . -name "*.cs" -type f | xargs grep -l "class FanCurveEditor"Repository: beenycool/AsusFanControl
Length of output: 152
🏁 Script executed:
#!/bin/bash
# Check the FanCurveEditor class to see what it inherits from
rg -A 5 "class FanCurveEditor" --type=csRepository: beenycool/AsusFanControl
Length of output: 906
🏁 Script executed:
#!/bin/bash
# Check if there are other modal dialog usages in the codebase and their patterns
rg -n "ShowDialog\(\)" --type=cs | head -20Repository: beenycool/AsusFanControl
Length of output: 769
🏁 Script executed:
#!/bin/bash
# Check if the editor variable is used anywhere after ShowDialog() call
rg -A 10 "buttonEditCurve.Click" AsusFanControlGUI/ProfileEditorDialog.csRepository: beenycool/AsusFanControl
Length of output: 435
🏁 Script executed:
#!/bin/bash
# Check context around line 100 in ProfileEditorDialog
sed -n '95,105p' AsusFanControlGUI/ProfileEditorDialog.csRepository: beenycool/AsusFanControl
Length of output: 432
🏁 Script executed:
#!/bin/bash
# Check context around line 331 in Form1.cs
sed -n '328,335p' AsusFanControlGUI/Form1.csRepository: beenycool/AsusFanControl
Length of output: 469
Dispose FanCurveEditor after use.
Line 196 creates a modal form without disposal, which can leak UI resources on repeated opens. The correct pattern is already used at line 100 in the same file with AddProfileDialog.
Proposed fix
- var editor = new FanCurveEditor(_curve);
- if (editor.ShowDialog() == DialogResult.OK)
- _curve = editor.ResultCurve;
+ using (var editor = new FanCurveEditor(_curve))
+ {
+ if (editor.ShowDialog() == DialogResult.OK)
+ _curve = editor.ResultCurve;
+ }📝 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.
| buttonEditCurve.Click += (s, e) => | |
| { | |
| var editor = new FanCurveEditor(_curve); | |
| if (editor.ShowDialog() == DialogResult.OK) | |
| _curve = editor.ResultCurve; | |
| }; | |
| buttonEditCurve.Click += (s, e) => | |
| { | |
| using (var editor = new FanCurveEditor(_curve)) | |
| { | |
| if (editor.ShowDialog() == DialogResult.OK) | |
| _curve = editor.ResultCurve; | |
| } | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@AsusFanControlGUI/ProfileEditorDialog.cs` around lines 194 - 199, The modal
FanCurveEditor created in the buttonEditCurve.Click handler is not disposed,
risking UI resource leaks; change the handler to create the FanCurveEditor in a
disposal scope (e.g., using or try/finally) so the editor is disposed after
ShowDialog completes and still assign _curve = editor.ResultCurve only when the
dialog returned DialogResult.OK, referencing the existing variables and methods:
buttonEditCurve.Click, FanCurveEditor, ShowDialog(), ResultCurve and _curve.
* Ship single AsusFanControl.exe: GUI + CLI in one project Remove the legacy console project and build AsusFanControl.exe from the WinForms app (AssemblyName AsusFanControl). Add CliProgram for CLI mode with optional --debug-log, attach console when args are present, and Costura.Fody to embed managed dependencies. Update the solution to Core + GUI only, tune CI artifact staging for the new output layout, and document same-exe usage in the README. Made-with: Cursor * Profile manager types and profile editor dialog (#26) * Add profile manager core types and profile editor UI Introduce ProfileManager and FanProfile serialization in Core, a Windows Task Scheduler helper for future automation hooks, and WinForms pieces (DarkMenuRenderer, ProfileEditorDialog) compiled into the GUI project. Form wiring can follow in a later change. Made-with: Cursor * Apply reviewer suggestions for PR #26 * Apply reviewer suggestions for PR #25
1 participant
Stacked on the single-exe PR: adds
ProfileManager/FanProfileserialization in Core,TaskSchedulerHelperfor scheduled automation, and WinForms UI pieces (ProfileEditorDialog,DarkMenuRenderer) included in the GUI project. Menu integration and runtime wiring can be added in a follow-up so this stays a focused library + dialog change set.Made with Cursor
Summary by CodeRabbit