dokan-dev / dokan-dotnet

Dokan DotNet Wrapper
http://dokan-dev.github.io
MIT License
462 stars 116 forks source link

Dokan 2.0.0 support #280

Closed Liryna closed 2 years ago

Liryna commented 2 years ago

Most dokan 2.0.0 API changes were implemented in dokan-dotnet and the test fixed in master but the new async mount function CreateFileSystem is still not working due to the garbage collector releasing the allocated DOKAN_OPTIONS DOKAN_OPERATIONS and maybe even DokanOperationProxy shortly after the mount which make the application crash since it is still used by the native code. https://github.com/dokan-dev/dokan-dotnet/blob/master/DokanNet/Dokan.cs#L382-L429 https://github.com/dokan-dev/dokan-dotnet/blob/master/sample/DokanNetMirror/Program.cs#L37-L40

I have been trying to add a reference by creating a DokanInstance class but it did not help unfortunately. If someone has an idea or a fix it would be highly appreciated ! Kind ping to @magol

LTRData commented 2 years ago

After a quick look there are a couple of issues with the approach as far as I can see. For instance, DOKAN_OPERATIONS is a managed structure (value type containing object references such as delegates), so when passing such a structure by ref to unmanaged code the runtime marshals the managed structure to a temporary native structure with function pointers instead, just like it would do for a class. This native structure only lives in memory during the native call, when it returns the runtime copies fields back to the managed structure and frees the native structure. The problem is not that the managed structure is freed, it is the temporary copy of it for the function call that is freed.

One way to solve this would be to create a value-only structure that can be directly passed to native code and pin it in memory with GCHandle.Alloc. Such a structure needs to contain only value fields such as IntPtr rather than delegate references and we need to be sure that these delegates that we marshal to unmanaged pointers survive too for at least the needed lifetime. I could take a look at the details here and exactly what needs pinning and manual marshalling and hopefully come up with a suggestion!

Liryna commented 2 years ago

Fully agree with what you just said !!! Let me know if I can be any help

LTRData commented 2 years ago

Just added a pull request with some changes that should fix this.

kyanha commented 2 years ago

After a quick look there are a couple of issues with the approach as far as I can see. For instance, DOKAN_OPERATIONS is a managed structure (value type containing object references such as delegates), so when passing such a structure by ref to unmanaged code the runtime marshals the managed structure to a temporary native structure with function pointers instead, just like it would do for a class. This native structure only lives in memory during the native call, when it returns the runtime copies fields back to the managed structure and frees the native structure. The problem is not that the managed structure is freed, it is the temporary copy of it for the function call that is freed.

One way to solve this would be to create a value-only structure that can be directly passed to native code and pin it in memory with GCHandle.Alloc. Such a structure needs to contain only value fields such as IntPtr rather than delegate references and we need to be sure that these delegates that we marshal to unmanaged pointers survive too for at least the needed lifetime. I could take a look at the details here and exactly what needs pinning and manual marshalling and hopefully come up with a suggestion!

You realize that that's why DOKAN_OPTIONS was a struct and shouldn't have been changed to a class, yes? #282 introduced that change. Even though it might have been copied, it was still effectively not going to be marshaled to a temporary struct that was immediately deallocated.

LTRData commented 2 years ago

No, DOKAN_OPTIONS has a byte array so it could not be directly passed by pinned pointer to unmanaged code, it had to be marshalled to an unmanaged structure when passed to unmanaged methods. This is because managed structs with byte arrays do not have the data for the byte array allocated directly in the struct, they only contain a managed reference to a managed byte array object and such cannot be sent to unmanaged code. It is typically inefficient to keep this kind of structs as "struct". Declaring them as "class" instead means that they do not have to be copied and allocated in as many places and it is easy to marshal them into unmanaged memory at one single point. (Otherwise, even the call to Marshal.StructureToPtr need to box the managed struct in a managed reference object anyway.)

Another way to solve this, since it is anyway a fixed length byte array in that struct would be to declare it as an unsafe struct and change the byte array field to a fixed byte array field. So instead of: [MarshalAs(UnmanagedType.ByValArray, SizeConst = 16384, ArraySubType = UnmanagedType.U1)] public byte[] VolumeSecurityDescriptor; Change to: fixed byte VolumeSecurityDescriptor[16384];

That would make it possible to directly pass this structure to unmanaged code without any kind of marshalling. Lifetime of that would also be easy to manage because we could send a ref to a field of this type in a managed object that we make sure to keep alive while the file system is mounted and the structure is used by unmanaged code.

LTRData commented 2 years ago

Sorry, answered to quickly, just realized there are string references in DOKAN_OPTIONS as well. Then the fixed byte array approach would not help anyway because the string refences need to be marshalled manually to unmanaged strings and replaced with IntPtr fields in the struct.

So, there are very good reasons to have DOKAN_OPTIONS as a class instead of struct.

Liryna commented 2 years ago

The version is now released with 2.0.0 support