Bug in Generichid_cs (InvalidOverlappedToPinvoke)

[jdunne]

Hello,

I just uncovered a bug in generichid_cs.  I kept getting an exception InvalidOverlappedToPinvoke in my application similar to this guy: http://www.janaxelson.com/forum/index.php?topic=171.0.  I was not seeing the exception on your generichid_cs application, but I was seeing it on mine.  I have been slowly refactoring my code until it is virtually identical to generichid_cs in terms of the kernel32.dll calls.  I have been battling this for 2 days now and finally I just decided that this was just a warning generated by the MDA, so I figured I'd try disabling the warning and see if the thing actually works.  Well, it turns out that it does work just fine, so I went back to the generichid_cs project and found that generichid_cs disabled the exceptions just as I did.  Once they are re-enabled, the exception shows on generichid_cs just as it had on mine.

It appears that this bug has always been there, but its been covered up by the fact that the project file has the break on exceptions disabled.  Open GenericHID_CS and press CTRL+ALT+E.  There will be box that shows whether to break execution upon thrown exceptions or not.  They are disabled for both Common Language Runtime and Managed Debug Assistants.  Enable both.  In this case, the exception is generated by the Managed Debug Assistant.  Voila, exception upon ReadFile using Interrupt transfer.  This means that we've been getting heap corruption whenever this tool is used.  

I spent a little while looking into how to use a NativeOverlapped structure utilizing the Overlapped.Pack method, and everything I've tried causes ReadFile to return false.  I tried the things in http://www.beefycode.com/post/Using-Overlapped-IO-from-Managed-Code.aspx, and I think I'm still missing something as its still not working (ReadFile returns false).  (Though I get no exception using this method).

Code: [Select]

//Doesn't work.
//internal static extern Boolean ReadFile( SafeFileHandle hFile, IntPtr lpBuffer, Int32 nNumberOfBytesToRead, ref Int32 lpNumberOfBytesRead, NativeOverlapped lpOverlapped );
//success = FileIO.ReadFile(_ReadHandle, nonManagedBuffer, Capabilities.InputReportByteLength, ref bytesread, HIDOverlapped);           //Returns false

//Existing method in generichid_cs.  Works, but corrupts the heap.
internal static extern Boolean ReadFile(SafeFileHandle hFile, IntPtr lpBuffer, Int32 nNumberOfBytesToRead, ref Int32 lpNumberOfBytesRead, IntPtr lpOverlapped);        
success = FileIO.ReadFile(_ReadHandle, nonManagedBuffer, Capabilities.InputReportByteLength, ref bytesread, nonManagedOverlapped);      //Returns True, but generates Exception

//Doesn't work.. Should work, but I'm doing something wrong.  My attempt at following this: http://www.beefycode.com/post/Using-Overlapped-IO-from-Managed-Code.aspx.  
//unsafe internal static extern Boolean ReadFile(SafeFileHandle hFile, IntPtr lpBuffer, Int32 nNumberOfBytesToRead, ref Int32 lpNumberOfBytesRead, NativeOverlapped* lpOverlapped);        
//success = FileIO.ReadFile(_ReadHandle, nonManagedBuffer, Capabilities.InputReportByteLength, ref bytesread, nativeOverlapped);        //Returns false


Joe

[jdunne]

I have hacked up the generichid_cs v4.6 to fix the error.  I basically followed http://www.beefycode.com/post/Using-Overlapped-IO-from-Managed-Code.aspx.  Note the way I did this is a pretty bad hack.. I'm not using a proper method for waiting for the callback, and I'm basically not using the callback the way it was intended.  Anyway, this works in that it doesn't generate the exception about heap corruption, and returns valid data, but ReadFile always returns false, which concerns me a bit.  We need to come up with a proper solution to this problem.

PLEASE DO NOT USE THIS CODE AS IS.  IT IS A BAD HACK AND NOT A COMPLETE SOLUTION TO THE ISSUE I'VE POINTED OUT.

Joe

[attachment deleted by admin]

[jdunne]

DOH!  I figured out why ReadFile was returning false. 

Change the following:
                    NativeOverlapped* nativeOverlapped = overlapped.Pack(
                    DeviceWriteControlIOCompletionCallback,
                    nonManagedBuffer                                        //was null
                    );

Remove success = true:
//We're always getting success = false right now, but it actually works..
//success = true;

Now ReadFile returns true as it should.  I still don't like my method for waiting til the callback completes though..

Joe

[jdunne]

My last post was completely wrong.  Ignore it.

Anyway, I've come up with a final solution that works.  I've attached a modified version of Generichid_cs v4.6 with the minimum set of changes needed to fix the heap corruption issue.  The only problem with it, if you can consider it a problem is that it now requires the code be compiled with "unsafe."  I prefer that than code that doesn't need to be compiled with unsafe, but corrupts the heap.

By the way, I've renamed the subject of this thread to something that is a bit more descriptive.  (It used to be InvalidOverlappedToPinvoke).

Joe

[attachment deleted by admin]

[Jan Axelson]

Thanks for sharing your solution!

Jan

[jdunne]

I found an even better solution that doesn't need the unsafe code.  A person named wimar created a USB HID library at the following location: http://www.codeproject.com/KB/cs/USB_HID.aspx.
His library doesn't support interrupt transfer, but he did use a much better approach to dealing with the file stream object.  He used System.IO.FileStream to communicate with the HID handle.  This approach is MUCH better as it uses a native C# library instead of ReadFile/WriteFile.  I simply extended what he did to use an Interrupt transfer handle as the handle for the FileStream.  Basically the following is how the various handles are used:

(I didn't take the generichid_cs source quite this far, but this COULD be done)
hidHandle = FileIO.CreateFile(devicePathName[memberIndex], 0, FileIO.FILE_SHARE_READ | FileIO.FILE_SHARE_WRITE, IntPtr.Zero, FileIO.OPEN_EXISTING, 0, 0);
hidFileStream = new FileStream(hidHandle, FileAccess.Read | FileAccess.Write, MyHid.Capabilities.FeatureReportByteLength, false);

readHandle = FileIO.CreateFile(myDevicePathName, FileIO.GENERIC_READ, FileIO.FILE_SHARE_READ | FileIO.FILE_SHARE_WRITE, IntPtr.Zero, FileIO.OPEN_EXISTING, 0, 0);
readFileStream = new FileStream(readHandle, FileAccess.Read, MyHid.Capabilities.InputReportByteLength, false);      //true

writeHandle = FileIO.CreateFile(myDevicePathName, FileIO.GENERIC_WRITE, FileIO.FILE_SHARE_READ | FileIO.FILE_SHARE_WRITE, IntPtr.Zero, FileIO.OPEN_EXISTING, 0, 0);
writeFileStream = new FileStream(writeHandle, FileAccess.Write, MyHid.Capabilities.OutputReportByteLength, false);

You simply then use readFileStream.Read, or .BeginRead depending on whether you want to use Synchronous or Asynchronous I/O.  I haven't fully investigated it, but I believe either way, when calling CreateFile, don't specify FileIO.FILE_FLAG_OVERLAPPED as was done previously.

Anyway, this now allows the code to be compiled WITHOUT the "unsafe" option which I didn't like.  (It also allows a solution for VB.NET for those interested).  I've attached the updated genericHID_CS with the changes mentioned.  Note that I opted for synchronous I/O since I find the code easier to follow.  I simply wrap it in a thread in my own applications to prevent the main thread from blocking.  (Either way, the original application was using overlapped I/O, but then blocking the thread til it completed, which basically is the same as using synchronous I/O from the perspective of the application)

Joe

[attachment deleted by admin]

[jcddcjjcd]

Joe.
First of all thanks for all the effort you put into finding a solution to the problem mentioned.
I have spent a couple of days playing around with various mods to the Generic solution using ideas you drew my attention to.
In the end I have suck with the Generic solution originated by Jan Axelson as it now stands and here is why.
A problem with synchronous reads is that once started they don't return until data is available and there is no satisfactory way of shutting them down. If you abort a thread they are running on they that can also present problems as bad as what we are trying to solve. Even if you use asynchronous reads using a FileStream you are still left with a pending read awaiting data before its callback is raised.
The FileStream way of doing things does not support a timeout which provides the elegant way of getting out of an incompleted read.
The best thing about the Generic solution is that it uses the asynchronous read with a timeout. This means that in a backgroundworker you can initiate a read with say a 100mS timeout and then check for a thread abort flag and leave if require by the application. Also a check can be made every second or so to see if the device is still there and if it is not action can be taken.
Also the Generic solution has proved  to be very robust and that counts for something too.
Finally the problem of the InvalidOverlappedToPinvoke is only a message telling of consequences that will arise only if the read is aborted before it finishes and this will rarely happen because the the precautions I just mentioned and in any case other methods would not help us entirely anyway.

The changes to the Generic solution you have proposed and provided code for seem to me to be the best way at present and I thank you again for the excellent work there.

Of course I may be wrong in what I am saying here and nothing would please me more to have it pointed.

Best regards,
John.

[jdunne]

John,

Thank you for your time on this issue.  In response to your post I quickly put together a version which uses Asynchronous I/O, but after thinking about it some more I'm not 100% sure if it actually addresses your concern or not. 

The way I have implemented the timeout uses a ManualResetEvent, which is a .NET class.  My x2 version above also uses the ManualResetEvent to implement the timeout.  The timeout parameter isn't built into the read function, it is built into the WaitForSingleObject function call which is separate from the Read call.  My new version basically does the same thing, but instead of using WaitForSingleObject, it uses a ManualResetEvent.  What does this mean when the application or thread is aborted?  I'll take a guess that the WaitForSingleObject timeout might still occur as it is a PInvoke to a System dll, which I don't think will terminate with the application (even though the application which requested it has shut down), so I could see why one might think that maybe this provides some protection if the application dies..  Realistically though, all this does is signal that the timeout has occurred.  It is up to the application to close the handle.  Once the .NET application / thread has terminated, CancelIo won't be called, and neither will readHandle.Close to properly release the resources of the Read...  Honestly, I really just don't know how this stuff works well enough to comment with any certainty.

One thing I do know is that the warning we get from the Managed Debug Assistant says to use a NativeOverlapped structure similar to what I did in x2.  My guess is that the reason they are saying to do this is so the .NET library can manage the handle by itself, so if the application or thread dies, the .NET runtime will take care to properly release the resources of the file handle.  I am assuming that using a FileStream object would put us under the same .NET umbrella, similar to the NativeOverlapped object. 

I may be putting too much faith in .NET, but I think the x3 solution I provided should be safe, and the x4 solution I attached here should address your concerns about Synchronous I/O.

Again, thanks for spending the time.  Its nice to know I'm not talking to myself here.

Joe

[attachment deleted by admin]

[Jan Axelson]

Joe,

I very much appreciate your efforts to improve my code also!

Jan

[jcddcjjcd]

Thanks for pointing these things out to me Joe.
How do you feel about taking it all the way and applying the same FileStream methodology to Writing as well.
You have a good handle on this now and it would seem to be the logical step forward. If you get it started and post a version then I will apply it to my projects and see if there are any wrinkles. Finally if Jan is on board with this it could become the next release. Like the recent changes to make it 64bit capable, this addresses an issue that is worth improving.
Best regards,
John.

[jdunne]

John,

Now you're getting greedy...   

I'll try and put something together in the next few days.  Honestly though, I'm kinda out of time on this project as I need to get back to work on other projects.  I'll see what I can do in my free time to finish it up.

Joe

[Jan Axelson]

When I get some time, I'll look into modifying my examples at Lvr.com

Jan

[jcddcjjcd]

Joe.
I have spent some more time implementing the FileStream approach to reading rather than FileRead. I find the observations I made earlier still hold. While you have implemented a ManualResetEvent to provide a way to get out of an incomplete read, it still does not help when you need to dispose of the FileStream you are using. The FileStream must provide it's own underlying connection with the HID device using the provided readHandle but I begin to understand why it does not provide for a timeout as it does in other scenarios. I don't think MS were able to implement it in a satisfactory way.
The FileStream does not tolerate being Closed, Disposed or re instantiated if a read has not completed.

In contrast, when using FileIO.GetOverlappedResult and waiting for a FileIO.WAIT_TIMEOUT you can simply call FileIO.CancelIo(readHandle); and the underlying mechanism is correctly terminated and your readHandle is still intact.
If your abandon a FileStream the readHandle becomes invalid.

The FileStream method works as long as you don't need to terminate the read, and if you only need to do that at the end of the Applications life then I guess it is OK.

I don't rely on DeviceNotifications to tell me when there are problems with the HID device, disconnections etc, because your code will often have run into trouble performing an action before the notification has arrived. I find it better to perform reads with a 100mS timeout and call FileIO.CancelIo(readHandle); to end the read before the next read is performed. This way the availability of the HID device can be tested between reads and there is an elegant way out of trouble.

Using this approach I have many devices running for months at a time without any issues. The thread that does the continuous reading will run FindTheHid every second if the device has been removed and reading will commence when it is there again.

To sum it up. I have not been able to find a way to end a FileStream read that has not completed. There a lots of things that can be called to try to tidy up the the code MS have used to support the FileStream connected to an HID.  However they will give you exceptions that all relate in some way to calls across the Managed/Unmanaged boundry.

Perhaps you can straighten my thinking out if I am wrong here.

Meanwhile your version 2 which uses the Managed overleaped structure is still the best way to my way of thinking and as I have mentioned earlier even this is not neccessary if you exit the read correctly.

Regards,
John.

I

[jdunne]

John,

It looks like you've done your homework.  I am mainly a firmware developer, so PC software is not my main focus.  At this point I am going to trust your judgment on the issue. I think you are probably right in your summary of the issue.  At this point we are again left with no proper solution (in my opinion).  I guess that means the only valid solutions are my version 2 posted above, which requires unsafe code, and Jan's original code as-is with the warning message disabled.

Quite honestly I have moved on to other issues.  The thing that started this was that I found my bootloader wasn't working on Windows 7 machines.  I incorrectly assumed this was related to the overlapped I/O issue we have been chasing in this thread.  As it turns out, my real problem was that Windows Xp and Windows 2000 allowed low speed USB devices to enumerate with an interrupt pipe larger than 8 bytes.  This was incorrect all along, and Windows Vista and Windows 7 fixed this bug.  The new versions of Windows now disallow a USB low speed device from enumerating in this incorrect configuration.  It wasn't til recently that I tried Windows 7 with my bootloader and found that it did not work.  My solution was to switch the application and firmware to utilize control transfer (HidD_GetFeature) to transmit the data instead of interrupt transfer (ReadFile).  Control transfer supports transactions larger than 8 bytes although it supports them over multiple 8 byte packets.  Since I am no longer using Interrupt transfer, the entire overlapped I/O issue is no longer a problem for me. 

Despite this development I have continued to work on the issue out of my curiosity and desire to contribute my solution for others.  Since you seem to have taken the initiative I will leave it to you to decide how to deal with this issue from this point forward.  (which you pretty much already have)

Joe

[jcddcjjcd]

Thanks for your reply Joe and I guess until someone can shed more light on the issue we will leave it. It will be interesting to get a final comment from Jan before this thread goes to sleep.
My email is jdekker@xtra.co.nz
Why don't you drop me a line with a return address and I will send you my bootloader as you may see something in it that will be of interest?
Anyway thanks for you original post as the solution 2 you provided is very interesting and potentially very useful and is something I was completely unaware of.
Like you, my main interest is in firmware programing so I am always open to correction in these matters as are you.
Regards,
John.