Bug 13541 – std.windows.syserror.sysErrorString() should be nothrow

It should be nothrow because it is called by constructors like FileException, which are themselves exceptions. Exceptions should be buildable without having "double fault" exceptions happening. Digging into sysErrorString() there's this: if(length == 0) { throw new Exception( "failed getting error string for WinAPI error code: " ~ sysErrorString(GetLastError())); } This should be an assert because: 1. sysErrorString()'s argument should ONLY be values returned by Windows APIs like GetLastError(), and so this should always succeed. 2. recursively calling sysErrorString() with the SAME value will cause a stack overflow crash, not any usable exception. Following this fix, sysErrorString() should be marked as 'nothrow'.

That recursion is really bad. :p > 1. sysErrorString()'s argument should ONLY be values returned by Windows APIs like GetLastError(), and so this should always succeed. It should, yes. And that's surely Microsofts stance as well. Yet they state that 0 is returned on error, so if this was my code I would at least handle that case and print some generic English message like "Failed to retrieve WinAPI error message for code 0x123456" instead of an empty string.

sysErrorString should be replaced with the newly-added wenforce wherever applicable. Some code in Phobos (std.mmfile at least) also incorrectly uses/used errnoEnforce when the error code was stored in GetLastError, not errno.

When I was writing platform specific code, I just put POSIX and WinAPI enforcement into a single `osApiEnforce`, throwing an OSApiException. I found I used it very often, because it was so easy to remember. Still we would want to catch that Exception inside sysErrorString() to be able to make it nothrow.

(In reply to Walter Bright from comment #0) > This should be an assert because: > > 1. sysErrorString()'s argument should ONLY be values returned by Windows > APIs like GetLastError(), and so this should always succeed. No, it may fail. Windows exposes a SetLastError API, which allows setting error codes which may not be valid. Third-party libraries may use SetLastError to set the error code to a private one, which sysErrorString may fail to parse. > 2. recursively calling sysErrorString() with the SAME value will cause a > stack overflow crash, not any usable exception. No, sysErrorString is not recursing "with the SAME value". It is recursing with the value from FormatMessageW, which we can expect to be valid.

(In reply to Walter Bright from comment #0) > 1. sysErrorString()'s argument should ONLY be values returned by Windows > APIs like GetLastError(), and so this should always succeed. > 2. recursively calling sysErrorString() with the SAME value will cause a > stack overflow crash, not any usable exception. As pointed out, it can fail, and it does not recurse with the same value. I should have left a comment when I initially wrote it. However, I agree with the desire for `nothrow`. I think the practical compromise would be to replace the runtime error check for `FormatMessageW` with an assert (that also uses assert(false) in the release path), then document that the function only accepts valid Windows error codes.

(In reply to Marco Leise from comment #3) > When I was writing platform specific code, I just put POSIX and WinAPI > enforcement into a single `osApiEnforce`, throwing an OSApiException. I > found I used it very often, because it was so easy to remember. I think it would be nice to make WindowsException and ErrnoException a descendant of a new OSException class. FileException should be an alias of OSException - there really isn't anything specific to file operations (when compared with other syscalls) to warrant an exception class of their own.

(In reply to Jakob Ovrum from comment #5) > As pointed out, it can fail, and it does not recurse with the same value. I > should have left a comment when I initially wrote it. > > However, I agree with the desire for `nothrow`. I think the practical > compromise would be to replace the runtime error check for `FormatMessageW` > with an assert (that also uses assert(false) in the release path), then > document that the function only accepts valid Windows error codes. The cleanest would still be to always check the error code from FormatMessage, even in case of correct input, just because it gives no guarantees about success in the documentation.

Point taken about sysErrorString() and GetLastError() not being infinite recursion. But still, FileException should be fixed so it does not throw in the constructor, i.e. rewrite: if(length == 0) { throw new Exception( "failed getting error string for WinAPI error code: " ~ sysErrorString(GetLastError())); } as: if(length == 0) { auto newErrCode = GetLastError(); assert(errCode != newErrCode); // no infinite recursion return "failed getting error string for WinAPI error code: " ~ sysErrorString(newErrCode); } or something like that.

Only that won't make sysErrorString nothrow because of UTF conversions. (In reply to Walter Bright from comment #8) > return > "failed getting error string for WinAPI error code: " ~ > sysErrorString(newErrCode); For the record, this is a horrible solution. Imagine that an end-user tries using a D app, and they get an error popup that just says "failed getting error string for WinAPI error code: Resource not found". Not even an error code they can Google for. A better solution, and what wenforce already does, is to silently omit the error message and just return the error code as a string.

(In reply to Vladimir Panteleev from comment #9) > A better solution, and what wenforce already does, is to silently omit the > error message and just return the error code as a string. "Resource not found" can be added as a chained OSException.

@MoonlightSentinel created dlang/phobos pull request #8420 "Fix 13541 - Make sysErrorString and WindowsException ctor nothrow" fixing this issue: - Fix 13541 - Make sysErrorString and WindowsException ctor nothrow Both functions are used mostly when creating an exception for another error. Throwing another exception because the error message could not be retrieved suppresses the actually useful information (the error code). Hence this patch changes the aforementioned functions to yield `Error <code>` when the message lookup fails. https://github.com/dlang/phobos/pull/8420

dlang/phobos pull request #8420 "Fix 13541 - Replace plain usages of sysErrorString for errors" was merged into master: - 304160f5af979e56b667440520915fede5bfdd4d by MoonlightSentinel: Issue 13541 - Use `wenforce` instead of `enforce` + `sysErrorString` This ensures that the actual error message won't be suppressed when the lookup error code => message fails. The exception will also be more informative because `WindowsException` is explicitly intended for Windows API errors. - 362e81830c2cab9ed1508330d9886c0f4e1ebc12 by MoonlightSentinel: Fix 13541 - Replace usages of sysErrorString` with a helper function `sysErrorString` throws an exception for unknown error codes (e.g. from libraries using `SetLastError`) and hence could hide the actual error that caused the call to `sysErrorString`. The new helper function wraps the error code lookup and returns `Error X` on failure. https://github.com/dlang/phobos/pull/8420