Add ICommandRunner for safe cross-platform process execution - #289
Merged
Conversation
… execution - ICommandRunner: interface with RunAsync and RunOrThrowAsync - CommandRunner: safe Process wrapper using ArgumentList (no shell parsing) - CommandResult: structured result with exit code, stdout, stderr - Migrated Windows: PnPUtil install/verify from Process.Start to ICommandRunner - Migrated Linux: all insmod/modprobe/dpkg/rpm/dpkg-deb calls to ICommandRunner - Removed Linux ExecuteCommandAsync helper (obsoleted by CommandRunner) - Removed shell escaping workarounds (debPath.Replace, rpmPath.Replace) - Both platform updaters now accept ICommandRunner via constructor injection - DrivelutionFactory creates CommandRunner and passes it to platform constructors Closes #288
Contributor
There was a problem hiding this comment.
Pull request overview
This PR introduces a cross-platform process execution abstraction (ICommandRunner) for GeneralUpdate.Drivelution to replace direct Process.Start usage, aiming to reduce command-injection risk by passing arguments via ProcessStartInfo.ArgumentList. It also migrates Windows (PnPUtil) and Linux driver operations to use the new abstraction and wires the default implementation through the factory.
Changes:
- Added
ICommandRunner,CommandRunner, andCommandResultunderCore/Executionfor structured, argument-list-based command execution. - Migrated Windows PnPUtil verification/installation and Linux module/package commands to
ICommandRunner. - Updated
DrivelutionFactoryto create and inject aCommandRunnerinto platform implementations.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| src/c#/GeneralUpdate.Drivelution/Windows/Implementation/WindowsGeneralDrivelution.cs | Replaces direct PnPUtil Process.Start with injected ICommandRunner. |
| src/c#/GeneralUpdate.Drivelution/Linux/Implementation/LinuxGeneralDrivelution.cs | Replaces Linux command execution helper with ICommandRunner calls. |
| src/c#/GeneralUpdate.Drivelution/Core/Execution/ICommandRunner.cs | New interface for safe command execution with argument arrays. |
| src/c#/GeneralUpdate.Drivelution/Core/Execution/CommandRunner.cs | New default implementation wrapping ProcessStartInfo.ArgumentList and capturing stdout/stderr. |
| src/c#/GeneralUpdate.Drivelution/Core/Execution/CommandResult.cs | New structured result type (exit code, stdout, stderr, success). |
| src/c#/GeneralUpdate.Drivelution/Core/DriverUpdaterFactory.cs | Factory now constructs and injects CommandRunner for Windows/Linux updaters. |
Comments suppressed due to low confidence (1)
src/c#/GeneralUpdate.Drivelution/Windows/Implementation/WindowsGeneralDrivelution.cs:85
VerifyInstallationAsynctreats exceptions as non-fatal (returns true), but ifpnputil.exe /enum-driversexits non-zero,RunAsyncwill returnSuccess=falsewithout throwing and this method will return false, causing the verify pipeline step to fail and potentially trigger rollback. Handle!result.Successsimilarly to the catch path (log stderr and return true, or throw if you intend verification to be fatal).
var result = await _commandRunner.RunAsync(
"pnputil.exe",
new[] { "/enum-drivers" },
cancellationToken);
var driverFileName = Path.GetFileName(driverInfo.FilePath);
var driverName = Path.GetFileNameWithoutExtension(driverInfo.FilePath);
bool isInstalled = result.StandardOutput.Contains(driverFileName, StringComparison.OrdinalIgnoreCase)
|| result.StandardOutput.Contains(driverName, StringComparison.OrdinalIgnoreCase);
GeneralTracer.Info($"Driver verification result: {isInstalled}");
return isInstalled;
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
31
to
35
| var validator = new WindowsDriverValidator(); | ||
| var backup = new WindowsDriverBackup(); | ||
| return new WindowsGeneralDrivelution(validator, backup); | ||
| var commandRunner = new CommandRunner(); | ||
| return new WindowsGeneralDrivelution(validator, backup, commandRunner, options); | ||
| } |
| { | ||
| var output = await ExecuteCommandAsync("modinfo", koPath, cancellationToken); | ||
| var result = await _commandRunner.RunAsync("modinfo", new[] { koPath }, cancellationToken); | ||
| var output = result.Success ? result.StandardOutput : string.Empty; |
Comment on lines
+374
to
375
| var output = result.Success ? result.StandardOutput : string.Empty; | ||
|
|
Comment on lines
+405
to
406
| var output = result.Success ? result.StandardOutput : string.Empty; | ||
|
|
Comment on lines
+51
to
+58
| if (!process.Start()) | ||
| throw new InvalidOperationException($"Failed to start process: {command}"); | ||
|
|
||
| process.BeginOutputReadLine(); | ||
| process.BeginErrorReadLine(); | ||
|
|
||
| await process.WaitForExitAsync(cancellationToken); | ||
|
|
Comment on lines
+54
to
+64
| process.BeginOutputReadLine(); | ||
| process.BeginErrorReadLine(); | ||
|
|
||
| await process.WaitForExitAsync(cancellationToken); | ||
|
|
||
| return new CommandResult | ||
| { | ||
| ExitCode = process.ExitCode, | ||
| StandardOutput = stdout.ToString(), | ||
| StandardError = stderr.ToString() | ||
| }; |
| foreach (var arg in arguments) | ||
| psi.ArgumentList.Add(arg); | ||
|
|
||
| GeneralTracer.Debug($"Running: {command} {string.Join(" ", arguments)}"); |
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
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
Add ICommandRunner to eliminate direct Process.Start calls and command injection risks.
New files
Migrated
Removed
Closes #288