Skip to content
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

Jump to bottom

Start-Process: Fix -Credential error as non-admin #21393

Merged
merged 2 commits into from
Apr 15, 2024
Merged

Start-Process: Fix -Credential error as non-admin #21393

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7e46944
Start-Process: Fix -Credential error as non-admin
jborean93 Mar 30, 2024
c12a42f
Change warning to debug
jborean93 Apr 4, 2024
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
16 changes: 15 additions & 1 deletion src/Microsoft.PowerShell.Commands.Management/commands/management/Process.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2109,7 +2109,21 @@ protected override void BeginProcessing()
// "Process was not started by this object, so requested information cannot be determined."
// Fetching the process handle will trigger the `Process` object to update its internal state by calling `SetProcessHandle`,
// the result is discarded as it's not used later in this code.
_ = process.Handle;
try
{
_ = process.Handle;
}
catch (Win32Exception e)
Copy link
Member

Choose a reason for hiding this comment

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

Should we check the HRESULT for the specific error rather than treating them all the same?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

As getting the Handle was an attempt to try and set the internal state and not something that is being used in the code I didn't think it was worth setting a specific error code. I can change it but I didn't think it warranted catching a specific error because we aren't doing anything to work around the error.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@SteveL-MSFT Here we should rather move in the other direction. Since the only operation we do is getting a handle, we rather need to catch and ignore all exceptions, especially since if there is a question about behavior, we can always display the debug message.
Although I think Win32Exception covers everything that is needed.

{
// If the caller was not an admin and the process was started with another user's credentials .NET
// won't be able to retrieve the process handle. As this is not a critical failure we treat this as
// a warning.
if (PassThru)
{
string msg = StringUtil.Format(ProcessResources.FailedToCreateProcessObject, e.Message);
WriteDebug(msg);
}
}

// Resume the process now that is has been set up.
processInfo.Resume();
Expand Down
3 changes: 3 additions & 0 deletions src/Microsoft.PowerShell.Commands.Management/resources/ProcessResources.resx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -189,6 +189,9 @@
<data name="CannotStarttheProcess" xml:space="preserve">
<value>This command cannot be run completely because the system cannot find all the information required.</value>
</data>
<data name="FailedToCreateProcessObject" xml:space="preserve">
<value>Failed to retrieve the new process handle: "{0}". The Process object outputted may have some properties and methods that do not work properly.</value>
</data>
<data name="InvalidUserError" xml:space="preserve">
<value>This command cannot be run due to error 1783. The possible cause of this error can be using of a non-existing user "{0}". Please give a valid user and run your command again.</value>
</data>
Expand Down