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.

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

Process.Start() failure should include path #46417

Merged
merged 19 commits into from
Jul 14, 2021
Merged

Conversation

batzen
Copy link
Contributor

@batzen batzen commented Dec 27, 2020

This PR fixes #39928

@ghost
Copy link

ghost commented Dec 27, 2020

Tagging subscribers to this area: @eiriktsarpalis
See info in area-owners.md if you want to be subscribed.

Issue Details

This PR fixes #39928

Author: batzen
Assignees: -
Labels:

area-System.Diagnostics.Process

Milestone: -

Copy link
Member

@adamsitnik adamsitnik left a comment

Choose a reason for hiding this comment

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

@batzen big thanks for your PR! I've left some comments, please address them and take a look at the failing tests (let me know if you need some help in getting some information from CI logs).

Could you also remove the empty file (src/installer/snaps/dotnet-sdk/dotnet-runtime) ?

@@ -185,6 +185,9 @@
<data name="InvalidApplication" xml:space="preserve">
<value>The specified executable is not a valid application for this OS platform.</value>
</data>
<data name="FailedToStartFileDirectory" xml:space="preserve">
<value>Failed to start '{0}' with working directory '{1}'. {2}</value>
Copy link
Member

Choose a reason for hiding this comment

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

It's just an opinion (and I can see that it was a suggestion in the original issue #39928 (comment)), but I think that it would be better to append the missing information to the existing exception message. Especially if we want to add it for all throwing cases (not just when the file is not found)

Suggested change
<value>Failed to start '{0}' with working directory '{1}'. {2}</value>
<value>{2} (the file name was '{0}', the working directory '{1}').</value>

So we would have:

- The system cannot find the file specified
+ The system cannot find the file specified (the file name was 'abc.exe', the working directory 'C:\xyz').

Instead of:

- The system cannot find the file specified
+ Failed to start 'abc.exe' with working directory 'C:\xyz'. The system cannot find the file specified

cc @danmosemsft who proposed the initial version

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How about "{0} (file: '{1}', working directory: '{2}')." then?

Given that "file name" is not always appropriate as it could also be an absolute/relative path to some exe.
Even file is not always the right term/word as it could also be "ftp://something" or "https://something", but "file" sounds better than "file name" here.
Or should we stick with "file name" as the property is also called "FileName"?

Are there native errors that could produce an empty message?
In that case the original proposed message would still make sense whereas the new one would look awkward.

- Failed to start 'abc.exe' with working directory 'C:\xyz'. 
+  (the file name was 'abc.exe', the working directory 'C:\xyz').

Copy link
Member

Choose a reason for hiding this comment

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

How about "{0} (file: '{1}', working directory: '{2}')." then?

Sounds good to me!

Are there native errors that could produce an empty message?

This is a very good question. From what I can see in the source code, there is no such possibility for Windows:

return string.Format("Unknown error (0x{0:x})", errorCode);

And on Unix we end up calling strerror_r:

byte* message = StrErrorR(platformErrno, buffer, maxBufferLength);

return StrErrorR(platformErrno, buffer, bufferSize);

and the docs for it says that it also returns unknow error message for such cases:

The strerror(), strerror_l(), and the GNU-specific strerror_r()
functions return the appropriate error description string, or an
"Unknown error nnn" message if the error number is unknown.

So I think that we can assume that it's never empty.

Copy link
Member

Choose a reason for hiding this comment

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

With respect to the message, my only thought is whether exceptions might occur that wouldn't suggest we were trying to start a process. Eg., System is not in a good state (the file name was 'abc.exe', the working directory 'C:\xyz'). (I made this up - but OS sometimes give strange errors). If you don't have the callstack, only the message, it is not clear what is going on. This problem already exists of course - but that was my reasoning for the "failed to start" text.
But this is @adamsitnik call.

Copy link
Member

Choose a reason for hiding this comment

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

After some manual testing, such as where the filename provided is a directory, the message, "No such file or directory (file: '/home`, working directory: '/home')" seems confusing to me. I agree with the previous comments that having "failed to start" at the beginning is a better experience; the second part of the message is the underlying error.

In that spirit, I'll go with "An error occurred trying to start process '{0}' with working directory '{1}'. {2}".

@adamsitnik adamsitnik added this to the 6.0.0 milestone Jan 4, 2021
@batzen
Copy link
Contributor Author

batzen commented Jan 4, 2021

@adamsitnik I just had a look at the failing tests when running on unix.
They are caused by the fact that the Process class tries to resolve the path for fileName and returns null if nothing was found.
This is then special cased in ForkAndExecProcess. This means that the originally passed in file name is already lost.

I don't know why it's done this way and what happens in Interop.Sys.ForkAndExecProcess if the passed file name does not exist.
According to the code of SystemNative_ForkAndExecProcess the method tries to access the passed filename and if that fails the method also fails. But it just fails with -1 instead of something meaningful.

Do you have any idea on how i should proceed here?
I guess i have to change ForkAndExecProcess and it's callers to not pass null and change SystemNative_ForkAndExecProcess so that it returns ENOENT if access fails.

@batzen
Copy link
Contributor Author

batzen commented Feb 6, 2021

@adamsitnik I guess you missed my results and the question about the failing Linux tests. Would be nice if you could have a look. ;-)

Base automatically changed from master to main March 1, 2021 09:07
@batzen
Copy link
Contributor Author

batzen commented Mar 27, 2021

@adamsitnik Tests are now working on unix and i just rebased again.
I am still not sure about the final error message.
Currently it's "{0} (file: '{1}', working directory: '{2}').".

@jeffhandley
Copy link
Member

@adamsitnik Tests are now working on unix and i just rebased again.
I am still not sure about the final error message.
Currently it's "{0} (file: '{1}', working directory: '{2}').".

That message format seems good to me.

@adamsitnik -- could you take a look at the previous comment when you get a chance please?

@jeffhandley
Copy link
Member

@adamsitnik -- could you take a look at the previous comment when you get a chance please?

Ping on this, @adamsitnik. 😄

Comment on lines 429 to 432
ForkAndExecProcess(originalFilename, filename, argv, envp, cwd,
startInfo.RedirectStandardInput, startInfo.RedirectStandardOutput, startInfo.RedirectStandardError,
setCredentials, userId, groupId, groups,
out stdinFd, out stdoutFd, out stderrFd, usesTerminal);
Copy link
Contributor

Choose a reason for hiding this comment

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

Too many parameters! Maybe pass startInfo?

                    ForkAndExecProcess(startInfo, filename, argv, envp, cwd,
                        setCredentials, userId, groupId, groups,
                        out stdinFd, out stdoutFd, out stderrFd, usesTerminal);

@jeffhandley
Copy link
Member

@adamsitnik It looks like this PR is still awaiting your re-review. Note this comment too.

@jeffhandley
Copy link
Member

@batzen You're right; we let you down here. We failed to prioritize this and also failed to set expectations that we wouldn't be able to prioritize it until other work was completed. This isn't how we want to treat contributors and I'm glad you called us out on it. Thank you for letting us know how this was affecting you and I'm sorry.

I guess i have to change ForkAndExecProcess and it's callers to not pass null and change SystemNative_ForkAndExecProcess so that it returns ENOENT if access fails.

That sounds like the right approach to me. I hope it's OK with you, but I'd like to update your branch with the latest from main, resolve the conflicts, and take a closer look at this. I'll comment back within the next couple of days with the intention of getting this merged by Monday or Tuesday of next week so that this fix can make .NET 6 Preview 7.

@jeffhandley jeffhandley self-assigned this Jul 8, 2021
@batzen
Copy link
Contributor Author

batzen commented Jul 8, 2021

@jeffhandley Thanks for your kind response.
It would be very nice if my changes could be merged for .NET 6 preview 7.

According to the changes that could/should be made in ForkAndExecProcess and SystemNative_ForkAndExecProcess:
I don't feel very confident in doing those as my experience with Linux is near to zero and i barely managed to get the tests running locally with WSL to fix the failing tests. The changes in this PR also work without those other changes, but the native message presented to the user might not be accurate, as far as i understood by following the code flow.

@jeffhandley
Copy link
Member

Thank you, @batzen. I'll push some changes to your branch to get it caught up and then I'll check in again once I look more closely into ForkAndExecProcess and SystemNative_ForkAndExecProcess.

@jeffhandley
Copy link
Member

I guess I have to change ForkAndExecProcess and its callers to not pass null and change SystemNative_ForkAndExecProcess so that it returns ENOENT if access fails.

I'm going to split the difference on this. I'm changing ForkAndExecProcess within Process.Unix.cs to accept the ProcessStartInfo startInfo, string? resolvedFilename..., but I'm leaving Interop.ForkAndExecProcess alone.

I'm doing some manual testing on both Ubuntu and Windows with these changes now; I expect to have this ready for re-review tomorrow.

@jeffhandley
Copy link
Member

jeffhandley commented Jul 14, 2021

Test failures:

@jeffhandley
Copy link
Member

@batzen @stephentoub This is ready for re-review.

@batzen
Copy link
Contributor Author

batzen commented Jul 14, 2021

LGTM

@ghost
Copy link

ghost commented Jul 14, 2021

Hello @jeffhandley!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

@danmoseley
Copy link
Member

Failures aren't showing up here for some reason (but are likely all unrelated)
opened #55689

@danmoseley
Copy link
Member

System.IO.FileSystem.Net5Compat.Tests failed on Android with:

07-14 20:40:48.840  1565  1565 E lowmemorykiller: Kill 'net.dot.System.IO.FileSystem.Net5Compat.Tests' (7383), uid 10115, oom_adj 0 to free 2107432kB

Isolated storage tests failed on Android with

      <test name="System.IO.IsolatedStorage.Tests.HelperTests.GetDataDirectory(scope: Machine)" type="System.IO.IsolatedStorage.Tests.HelperTests" method="GetDataDirectory" time="0.0380145" result="Fail">
        <failure exception-type="System.IO.IOException">
          <message><![CDATA[System.IO.IOException : Read-only file system]]></message>
          <stack-trace><![CDATA[   at System.IO.FileSystem.CreateDirectory(String fullPath) in System.Private.CoreLib.dll:token 0x600553a+0x1a2
   at System.IO.Directory.CreateDirectory(String path) in System.Private.CoreLib.dll:token 0x60053e6+0x2d
   at System.Environment.GetFolderPathCore(SpecialFolder folder, SpecialFolderOption option) in System.Private.CoreLib.dll:token 0x60002b0+0x65
   at System.Environment.GetFolderPath(SpecialFolder folder, SpecialFolderOption option) in System.Private.CoreLib.dll:token 0x600029a+0x73
   at System.IO.IsolatedStorage.Helper.GetDataDirectory(IsolatedStorageScope scope) in /_/src/libraries/System.IO.IsolatedStorage/src/System/IO/IsolatedStorage/Helper.Win32Unix.cs:line 25
   at System.IO.IsolatedStorage.Tests.HelperTests.GetDataDirectory(IsolatedStorageScope scope) in /_/src/libraries/System.IO.IsolatedStorage/tests/System/IO/IsolatedStorage/HelperTests.cs:line 47
   at System.Reflection.RuntimeMethodInfo.Invoke(Object obj, BindingFlags invokeAttr, Binder binder, Object[] parameters, CultureInfo culture) in System.Private.CoreLib.dll:token 0x6004b15+0x6a]]></stack-trace>
        </failure>

System.Security.Cryptography.Xml tests failed with

      <test name="System.Security.Cryptography.Xml.Tests.SigningVerifyingX509Cert.SignedXmlHasDSACertificateVerifiableSignature" type="System.Security.Cryptography.Xml.Tests.SigningVerifyingX509Cert" method="SignedXmlHasDSACertificateVerifiableSignature" time="0.0440539" result="Fail">
        <failure exception-type="System.Security.Cryptography.CryptographicException">
          <message><![CDATA[System.Security.Cryptography.CryptographicException : The certificate data cannot be read with the provided password, the password may be incorrect.\n---- System.PlatformNotSupportedException : Algorithm 'RC2' is not supported on this platform.]]></message>
          <stack-trace><![CDATA[   at Internal.Cryptography.Pal.UnixPkcs12Reader.Decrypt(SafePasswordHandle password, Boolean ephemeralSpecified) in System.Security.Cryptography.X509Certificates.dll:token 0x600020e+0xb3

Oh, now I see that this is staging and can be ignored. But I'll leave this here for reference. cc @steveisok

@danmoseley
Copy link
Member

I realize now (1) failures in azdo are in staging and can be ignored (2) failure above should be ignored as it's staging also, but since it's a timeout it's not due to limitations.
So this is mergeable.

@danmoseley danmoseley merged commit 720279c into dotnet:main Jul 14, 2021
@danmoseley
Copy link
Member

@batzen thank you for the contribution here. It should go out in .NET 6 preview 7 in about a month. I second @jeffhandley comments that we aim to be more responsive on PR's and generally we are I think. I do hope you will continue to contribute and either Jeff or myself will pay special attention to any future PR you offer to make sure we do better.

@jeffhandley
Copy link
Member

Thanks for checking the tests and merging this, @danmoseley.

@batzen, this will be included in Preview 7. Thanks again for the contribution and for the feedback and help!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Process.Start() failure should include path
7 participants