Throw when Unix has no shell opener - #132219
Conversation
Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
|
Azure Pipelines: Successfully started running 3 pipeline(s). 13 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
adamsitnik
left a comment
adamsitnik
left a comment
There was a problem hiding this comment.
@copilot address my feedback
| } | ||
|
|
||
| [ConditionalFact(typeof(RemoteExecutor), nameof(RemoteExecutor.IsSupported))] | ||
| [PlatformSpecific(TestPlatforms.Linux)] |
There was a problem hiding this comment.
@copilot why is it specific to Linux? I would expect it to work on every Unix? Or even every platform?
There was a problem hiding this comment.
Updated in 3d8797e: the test now runs on Linux and FreeBSD. I did not extend it to all Unix platforms because this specific failure mode depends on opener lookup via PATH; on platforms with fixed opener paths (for example macOS/OpenBSD), clearing PATH does not produce the same no-opener condition.
Note
This reply was generated by AI/Copilot.
|
|
||
| private const int O_RDONLY = 0; | ||
| private const int O_WRONLY = 1; | ||
| private const int ERROR_NO_ASSOCIATION = 0x483; |
There was a problem hiding this comment.
@copilot what stops us from using Interop.Errors.ERROR_NO_ASSOCIATION? Why do we need to create a copy of this value?
There was a problem hiding this comment.
Updated in 3d8797e to use Interop.Errors.ERROR_NO_ASSOCIATION instead of a duplicated local constant. To make that available for the unix test target, I also included Common/Interop/Windows/Interop.Errors.cs in the unix test project item group.
Note
This reply was generated by AI/Copilot.
|
Tagging subscribers to this area: @dotnet/area-system-diagnostics-process |
There was a problem hiding this comment.
Pull request overview
Updates Unix UseShellExecute fallback behavior in System.Diagnostics.Process so that when no shell “open” utility is discoverable, it throws a consistent Win32Exception(ERROR_NO_ASSOCIATION) instead of failing later due to a null opener path. Adds a Linux regression test that makes the “no opener” condition deterministic by clearing PATH in a RemoteExecutor child process.
Changes:
- Throw
Win32Exception(Interop.Errors.ERROR_NO_ASSOCIATION)whenProcess.GetPathToOpenFile()returns null in the Unix shell-execute fallback. - Add a Linux
RemoteExecutortest that clearsPATHand asserts the native error code isERROR_NO_ASSOCIATION. - Introduce a local test constant for
ERROR_NO_ASSOCIATIONto validate the thrown error code.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/libraries/System.Diagnostics.Process/tests/ProcessTests.Unix.cs | Adds deterministic Linux regression coverage for the “no shell opener on PATH” scenario. |
| src/libraries/System.Diagnostics.Process/src/Microsoft/Win32/SafeHandles/SafeProcessHandle.Unix.cs | Throws the intended ERROR_NO_ASSOCIATION when no opener utility is found, preventing downstream null-related failures. |
| // use default program to open file/url | ||
| filename = Process.GetPathToOpenFile(); | ||
| filename = Process.GetPathToOpenFile() ?? throw new Win32Exception(Interop.Errors.ERROR_NO_ASSOCIATION); | ||
| string[] openFileArgv = ProcessUtils.ParseArgv(startInfo, filename, ignoreArguments: true); |
Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Suppressed comments (1)
src/libraries/System.Diagnostics.Process/tests/ProcessTests.Unix.cs:84
- This test relies on clearing PATH to force Process.GetPathToOpenFile() to return null. On FreeBSD, GetPathToOpenFile() checks a fixed path (/usr/local/bin/open) rather than searching PATH, so the test can become environment-dependent/flaky (it will start the opener if it happens to be installed). Consider limiting the test to Linux (as described in the PR) or otherwise making the FreeBSD behavior deterministic.
[PlatformSpecific(TestPlatforms.Linux | TestPlatforms.FreeBSD)]
public void ProcessStart_UseShellExecute_OnUnix_ThrowsWhenNoOpenerOnPath()
main PR #128416
Description
Win32Exception(ERROR_NO_ASSOCIATION)when no Unix shell opener is available.RemoteExecutorregression test that clearsPATH, ensuring the failure is deterministic.Customer Impact
Callers receive the intended Win32 error instead of a downstream null-related failure.
Regression
No.
Testing
Adds focused coverage for the no-opener path.
Risk
Low. Limits behavior to the existing shell-execute fallback failure path.
Package authoring no longer needed in .NET 9
IMPORTANT: Starting with .NET 9, you no longer need to edit a NuGet package's csproj to enable building and bump the version.
Keep in mind that we still need package authoring in .NET 8 and older versions.
Note
This PR description was generated by AI/Copilot.