dotnet / pinvoke

A library containing all P/Invoke code so you don't have to import it every time. Maintained and updated to support the latest Windows OS.
MIT License
2.12k stars 222 forks source link

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

Closed gabbyzhat closed 1 year ago

gabbyzhat commented 7 years ago

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

AArnott commented 7 years ago

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 commented 7 years ago

@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?

gabbyzhat commented 7 years ago

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);
AArnott commented 7 years ago

Reopening per @AkariAkaori's repro.

ghost commented 6 years ago

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.