pinvoke icon indicating copy to clipboard operation
pinvoke copied to clipboard

Last error needs to be cleared for functions where NULL return is not necessarily an error

Open gabbyzhat opened this issue 8 years ago • 5 comments

Example of a NULL that may be returned on success: GetWindowTextLength

It seems that the User32.GetWindowText can cause an erroneous exception by the following:

Steps to reproduce:

  1. Call something that may potentially set an error, such as Console.WriteLine (Example)
  2. Call User32.GetWindowText on a Window with an empty string title.
  3. The GetWindowTextLength returns 0, which can either mean an error or empty string
  4. User32.GetWindowText interprets this as a potential error and checks the last error
  5. User32.GetWindowText sees the error code that was set by calling Console.WriteLine prior and misinterprets this as an error of GetWindowTextLength, throwing an exception. This is because successful calls don't set the error value.

Solution:

Whenever it is possible that the last error may be misinterpreted, call SetLastError and clear it, so an empty string can be distinguished from an error. Example

gabbyzhat avatar Jun 02 '17 02:06 gabbyzhat

Thanks for the report and suggested fix, @AkariAkaori. That's very insightful, and I agree we should fix up all our helper methods to follow this pattern when the ambiguity exists.

AArnott avatar Jun 02 '17 14:06 AArnott

@AkariAkaori I couldn't repro the bug you referred to. I added tests to verify the appropriate behavior even if GetLastError != 0 going into a GetWindowTextLength call and in those tests, the last error is always reset to 0 when the resulting length is 0. What version of Windows and .NET Framework did you have installed when you saw this fail? Or did you not see this fail but theorized the problem may exist?

AArnott avatar Jul 12 '17 15:07 AArnott

I was getting exceptions when I iterated over all windows searching for a window whose text matched a regex with PInvoke.User32 v4.0.30319 targetting NET Framework 4.7 on Windows 10 64-bit enterprise

When I made an implementation that reset the error when appropriate, the exceptions stopped. The thing I noticed is that it only crashed when I called certain IO operations in .NET like Console.WriteLine (cant remember the specific functions I called but this is the general idea)

Crash after the first iteration:

foreach(var window in enumerateWindows)
   Console.WriteLine(window.GetWindowText())

No crash because texts are evaluated before calling Console:

foreach(var windowText in enumerateWindows.Select(x => x.GetWindowText()).ToArray())
    Console.WriteLine(windowText);

gabbyzhat avatar Jul 14 '17 00:07 gabbyzhat

Reopening per @AkariAkaori's repro.

AArnott avatar Jul 20 '17 00:07 AArnott

BTW, using CER/RuntimeHelpers.PrepareContrainedRegions referenced in #338 seems to work well for the GetWindowText helper (I just tried it out). It's certainly a big improvement over the native-module approach outlined by me in #388.

ghost avatar May 13 '18 17:05 ghost