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

Inappropriate use of Win32Exception #26227

Open
Tracked by #64603
tdinucci opened this issue May 20, 2018 · 24 comments
Open
Tracked by #64603

Inappropriate use of Win32Exception #26227

tdinucci opened this issue May 20, 2018 · 24 comments
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Runtime os-linux Linux OS (any supported distro) os-mac-os-x macOS aka OSX
Milestone

Comments

@tdinucci
Copy link
Member

If you look at:

https://github.com/dotnet/corefx/blob/master/src/System.Diagnostics.Process/src/System/Diagnostics/Process.Unix.cs

You'll see that there are many places where Win32Exceptions are thrown and this doesn't feel right on non-Win32 platforms.

There probably is no great way to address this since moving away from Win32Exception would break existing applications. I wonder though if it'd be an improvement if a new ProcessException type was introduced which descended from Win32Exception? This way existing applications could continue to handle Win32Exceptions but developers could choose to handle ProcessExceptions when developing on non-Win32 platforms.

@stephentoub
Copy link
Member

It's an unfortunate name, but it works cross-platform, and is really just a PlatformException with a less-than-ideal name. Changing it would be a huge breaking change, and when we previously discussed it, we decided there wasn't enough value in introducing a new exception.

@tdinucci
Copy link
Member Author

tdinucci commented May 20, 2018

The value I see here is primarily from a non-developers perspective, e.g. admins looking at logs, end users who see raw exception messages in some default exception handler, etc. Also, there'll be some value in the perception that non-Windows platforms are considered first class citizens.

I totally get that resolving this by doing something like renaming Win32Exception or creating a brand new UnixException (which didn't descend from Win32Exception would be a massive breaking change).

With my suggestion though (i.e. subclass Win32Exception) the potential to break existing code is significantly reduced (at the expensive of having a slightly confused type hierarchy). We're really talking about breaking places where types are compared explicitly, e.g.

catch(Exception ex)
{
	if(ex.GetType() == typeof(Win32Exception))
		...
}

I'd hope code like the above is rare but it would obviously break. However, I'd imagine this could be mitigated by the compiler spitting out a warning on non-Windows machines. There's obviously a question over what would happen with assemblies compiled and tested against an older version of these libraries compared to what it's actually executing on.

Anyway, I don't want to re-open a debate that's already been had. I just want to make sure that my original suggestion's been interpreted as intended.

@danmoseley
Copy link
Member

Im inclined to agree with @tdinucci that it would make code clearer (it always catches my eye on cross platform code). We historically consider replacing an exception with a derived type as very low risk.

It wouldn't help with the HResult property though. But that's less commonly seen in cross plat code.

@jnm2
Copy link
Contributor

jnm2 commented May 28, 2018

Plus, HResult is defined on the base Exception class.

@asyncawaitmvvm
Copy link

Oh you guys.

if (string.IsNullOrEmpty(filename))
{
    throw new Win32Exception(Interop.Error.ENOENT.Info().RawErrno);
}

What exactly IS this garbage?

P.S. ENOENT = no file or directory found.

Yet under System.IO we have:

FileNotFoundException
DirectoryNotFoundException

Don't justify this source file, fix it.

@danmoseley
Copy link
Member

danmoseley commented Jul 7, 2018

@asyncawaitmvvm is catching it making your code less readable, or you just noticed this in corefx code?

Would ProcessException derived from Win32Exception be a substantial improvement for you?

@danmoseley danmoseley reopened this Jul 7, 2018
@bgribaudo
Copy link
Contributor

A similar idea occurred to me when working on adding dotnet/corefx#34147. Would be nice to see something like this. :-)

@msftgits msftgits transferred this issue from dotnet/corefx Jan 31, 2020
@msftgits msftgits added this to the 5.0 milestone Jan 31, 2020
@maryamariyan maryamariyan added the untriaged New issue has not been triaged by the area owner label Feb 23, 2020
@joperezr joperezr removed the untriaged New issue has not been triaged by the area owner label Jul 6, 2020
@joperezr joperezr modified the milestones: 5.0.0, Future Jul 6, 2020
@danmoseley danmoseley added os-linux Linux OS (any supported distro) os-mac-os-x macOS aka OSX labels Dec 18, 2020
@danmoseley
Copy link
Member

danmoseley commented Dec 18, 2020

I looked through our Unix facing assemblies for public use of "Win32", in summary:

Win32Exception

  • used by Process for many errors. Could be replaced by a hypothetical ProcessException.
  • thrown from Console when it fails to initialize - that use does not appear to be documented, FWIW.
  • thrown from one minor place in Drawing (constructing Icon)
  • used numerous places in Networking. Networking code already defines four subclasses of Win32Exception visible in Unix (HttpListenerException, SocketException, WebSocketException, and NetworkInformationException) perhaps one of those would be a better fit in these places anyway.

Marshal.GetLastWin32Error() (and GetHRForLastWin32Error() which wraps it)

// we now have Marshal.GetLastPinvokeError() which resolves this
* essentially returns errno from last pinvoke - arguably that's non obvious, ie., the name is confusing - although most callers probably just use Win32Exception itself
* Would have to hypothetically be deprecated in favor of Marshal.GetLastError() that did the same thing

Microsoft.Win32.SafeHandles namespace

  • SafeProcessHandle and several other SafeXXHandle and CriticalHandleXX types.
  • Would need a hypothetical namespace forwarding feature to do anything about these.

Of the above, the networking work seems probably reasonable, but probably low priority unless these are often caught (don't know). It would need a separate issue opened for the networking team to consider. I also think the Process change is potentially worthwhile, as it's a fairly prominent (often caught) Windowsism that's out of place in a cross platform framework where Windows is no longer first, but I am not sure there is consensus achieved to change it. Neither would be considered a breaking change probably (per rules)

@danmoseley
Copy link
Member

cc @terrajobst for thoughts on this one.

@KZedan
Copy link
Contributor

KZedan commented May 17, 2021

@danmoseley can take a look at this one if needed.

@danmoseley
Copy link
Member

@KZedan I think the next work item would be this one:

Win32Exception is used numerous places in Networking. Networking code already defines four subclasses of Win32Exception visible in Unix (HttpListenerException, SocketException, WebSocketException, and NetworkInformationException) perhaps one of those would be a better fit in these places anyway.

@dotnet/ncl are you OK with replacing the use of Win32Exception in System.Net* with the existing derived exception types you already define? The goal would be to reduce the use of Win32Exception when there is no relationship with Win32 (eg., on Unix). We generally consider throwing a more derived type to be an acceptable change. There would be an element of judgement here to pick the most appropriate derived type in each case.

REDMOND+danmose@LAPTOP-P6UJDVTA MINGW64 /c/git/runtime/src/libraries (main)
$ grep -R 'Win32Exception' --include=*.cs System.Net*/*
System.Net.Http.WinHttpHandler/src/System/Net/Http/WinHttpException.cs:    internal sealed class WinHttpException : Win32Exception
System.Net.Http.WinHttpHandler/tests/FunctionalTests/ServerCertificateTest.cs:                var innerEx = (Win32Exception)ex.InnerException;
System.Net.HttpListener/ref/System.Net.HttpListener.cs:    public partial class HttpListenerException : System.ComponentModel.Win32Exception
System.Net.HttpListener/src/System/Net/HttpListenerException.cs:    public class HttpListenerException : Win32Exception
System.Net.HttpListener/src/System/Net/Windows/HttpListener.Windows.cs:            catch (Win32Exception exception)
System.Net.HttpListener/src/System/Net/Windows/HttpListenerResponse.Windows.cs:                catch (Win32Exception)
System.Net.HttpListener/src/System/Net/Windows/HttpListenerResponse.Windows.cs:            catch (Win32Exception)
System.Net.Mail/src/System/Net/Mail/SmtpNegotiateAuthenticationModule.cs:            catch (Win32Exception)
System.Net.Mail/src/System/Net/Mail/SmtpNegotiateAuthenticationModule.cs:            catch (Win32Exception)
System.Net.NetworkInformation/ref/System.Net.NetworkInformation.cs:    public partial class NetworkInformationException : System.ComponentModel.Win32Exception
System.Net.NetworkInformation/src/System/Net/NetworkInformation/TeredoHelper.cs:                    throw new Win32Exception((int)err);
System.Net.Ping/src/System/Net/NetworkInformation/Ping.Windows.cs:                    throw new Win32Exception(); // Gets last error.
System.Net.Ping/src/System/Net/NetworkInformation/Ping.Windows.cs:                    throw new Win32Exception(); // Gets last error.
System.Net.Ping/src/System/Net/NetworkInformation/Ping.Windows.cs:                throw new Win32Exception(statusCode);
System.Net.Primitives/ref/System.Net.Primitives.cs:    public partial class SocketException : System.ComponentModel.Win32Exception
System.Net.Primitives/src/System/Net/SocketException.cs:    public partial class SocketException : Win32Exception
System.Net.Primitives/src/System/Net/SocketException.Unix.cs:    public partial class SocketException : Win32Exception
System.Net.Primitives/src/System/Net/SocketException.Windows.cs:    public partial class SocketException : Win32Exception
System.Net.Security/src/System/Net/NTAuthentication.cs:                    throw new Win32Exception((int)SecurityStatusPalErrorCode.InvalidHandle);
System.Net.Security/src/System/Net/Security/NegotiateStream.cs:            catch (Win32Exception e)
System.Net.Security/src/System/Net/Security/NegotiateStream.cs:            var e = new Win32Exception((int)error);
System.Net.Security/src/System/Net/Security/NegotiateStream.cs:            exception is Win32Exception win32exception &&
System.Net.Security/src/System/Net/Security/NegotiateStreamPal.Unix.cs:                    throw new Win32Exception((int)status.ErrorCode);
System.Net.Security/src/System/Net/Security/NegotiateStreamPal.Windows.cs:                        throw new Win32Exception((int)SecurityStatusAdapterPal.GetInteropFromSecurityStatusPal(status));
System.Net.Security/src/System/Net/Security/NegotiateStreamPal.Windows.cs:                        throw new Win32Exception((int)winStatus);
System.Net.Security/src/System/Net/Security/NegotiateStreamPal.Windows.cs:                Exception e = new Win32Exception(errorCode);
System.Net.Security/src/System/Net/Security/NegotiateStreamPal.Windows.cs:                Exception e = new Win32Exception(errorCode);
System.Net.Security/src/System/Net/Security/NegotiateStreamPal.Windows.cs:                Exception e = new Win32Exception(errorCode);
System.Net.Security/src/System/Net/Security/NegotiateStreamPal.Windows.cs:                throw new Win32Exception(errorCode);
System.Net.Security/src/System/Net/Security/SslStream.Implementation.cs:            catch (Win32Exception e)
System.Net.Security/src/System/Net/Security/SslStream.Implementation.cs:            catch (Win32Exception e)
System.Net.Security/src/System/Net/Security/SslStreamPal.OSX.cs:            return status.Exception ?? new Win32Exception((int)status.ErrorCode);
System.Net.Security/src/System/Net/Security/SslStreamPal.Windows.cs:            return new Win32Exception(win32Code);
System.Net.Security/tests/FunctionalTests/SslStreamAlertsTest.cs:                Assert.IsType<Win32Exception>(exception.InnerException);
System.Net.Security/tests/FunctionalTests/SslStreamAlertsTest.cs:                var win32ex = (Win32Exception)exception.InnerException;
System.Net.Security/tests/FunctionalTests/SslStreamNegotiatedCipherSuiteTest.cs:                if (!(e is AuthenticationException || e is Win32Exception))
System.Net.Security/tests/FunctionalTests/SslStreamNegotiatedCipherSuiteTest.cs:                    catch (Win32Exception) { }
System.Net.Security/tests/FunctionalTests/SslStreamNegotiatedCipherSuiteTest.cs:            catch (Exception e) when (e is AuthenticationException || e is Win32Exception)
System.Net.Security/tests/FunctionalTests/SslStreamNegotiatedCipherSuiteTest.cs:            catch (Exception e) when (e is AuthenticationException || e is Win32Exception || e is IOException)
System.Net.Sockets/tests/FunctionalTests/CreateSocketTests.cs:                    throw new Win32Exception();
System.Net.WebSockets/ref/System.Net.WebSockets.cs:    public sealed partial class WebSocketException : System.ComponentModel.Win32Exception
System.Net.WebSockets/src/System/Net/WebSockets/WebSocketException.cs:    public sealed class WebSocketException : Win32Exception

@scalablecory
Copy link
Contributor

scalablecory commented May 17, 2021

@dotnet/ncl are you OK with replacing the use of Win32Exception in System.Net* with the existing derived exception types you already define? The goal would be to reduce the use of Win32Exception when there is no relationship with Win32 (eg., on Unix). We generally consider throwing a more derived type to be an acceptable change. There would be an element of judgement here to pick the most appropriate derived type in each case.

Is the proposal to still derive from Win32Exception, but only throw more derived types rather than Win32Exception directly? That seems fine to me.

@geoffkizer
Copy link
Contributor

Is the proposal to still derive from Win32Exception, but only throw more derived types rather than Win32Exception directly? That seems fine to me.

+1

A lot of these issues seem to be in Windows auth; NegotiateStream etc. Not sure if/how these get surfaced to the user -- @wfurt may know. We might need a new derived exception here, not sure.

The rest seem like smallish issues we could clean up fairly easily.

@danmoseley
Copy link
Member

@scalablecory yes exactly. After this there should be no hits in System.Net.* excepting the derivations of your exception types themselves.

@danmoseley
Copy link
Member

@KZedan I assigned you. Please let us know if you have questions.

@geoffkizer
Copy link
Contributor

After this there should be no hits in System.Net.* excepting the derivations of your exception types themselves.

A couple of the hits above are in windows-specific code. These likely won't go away.

@danmoseley
Copy link
Member

A couple of the hits above are in windows-specific code. These likely won't go away.

Ah, yes, if they're catches from some other library. I'm guessing that those are thrown out of API that are not Windows specific though so that's something maybe to think about on a future occasion.

@terrajobst
Copy link
Member

terrajobst commented May 17, 2021

cc @terrajobst for thoughts on this one.

I'd love to see a proposal that avoids throwing Win32Exception in Unix/Linux environments. However, keep in mind that changing the thrown exception is a breaking change (unless the new exception derives from Win32Exception, which, to be honest, would still be a wart, but a much less visible one and might actually give us the best bang for the buck).

@danmoseley
Copy link
Member

unless the new exception derives from Win32Exception

@terrajobst that's what the work is here: System.Net.* already has several such exceptions, they're just not using them everywhere. Possibly, they might find that another one is needed, which would need API review of course.

avoids throwing Win32Exception in Unix/Linux environments.

As an aside, I don't think it's a great exception on Windows much of the time, unless your API is clearly a thin wrapper around a Windows API.

@terrajobst
Copy link
Member

Agreed. For WinRT marshalling, we changed our interop layer to throw Exception directly if the we had no mapping for the given HRESULT (which is why we made the HResult property public on Exception). The idea is that in the future we could add a more specialized exception. If we were to do P/Invoke layers all over again, we'd probably do the same there, instead of creating weird types like Win32Exception that don't describe the error condition sufficiently anyway.

@geoffkizer
Copy link
Contributor

geoffkizer commented May 20, 2021

Clearly we have some improvement we could make in SslStream/SSPI: #52361

@KZedan
Copy link
Contributor

KZedan commented May 22, 2021

Sorry for the late reply guys, been a crazy week! 😅

In some cases, it wouldn't make sense for me to use one of the derived types as some of them are not descriptive of usage. Shall we leave those as is or create new derived types for them?

cc @danmoseley

@danmoseley
Copy link
Member

@KZedan i suggest to make the changes where the correct type is clear. We can merge that. Then you can make an API proposal if you wish for new exception type(s) as needed.

@ghost ghost added in-pr There is an active PR which will close this issue when it is merged and removed in-pr There is an active PR which will close this issue when it is merged labels May 23, 2021
@stephentoub
Copy link
Member

#58529 moves Win32Exception to corelib to enable brand new APIs to throw it. I think we should reconsider that. This thread primarily looks at compatibility for existing uses, with proposals for throwing derived exceptions, in order to hide the Win32 naming in the type's ancestry rather than having it be prominent in code and logs. We should consider also introducing a new base class for Win32Exception, moving Win32Exception's logic into that base class, having new APIs that want similar behavior to throw the new exception type, and updating all of our code that catches Win32Exception to instead catch the new base.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Runtime os-linux Linux OS (any supported distro) os-mac-os-x macOS aka OSX
Projects
None yet
Development

Successfully merging a pull request may close this issue.