dokan-dev / dokan-java

Dokan Java Wrapper
GNU Lesser General Public License v3.0
51 stars 28 forks source link

Mismatching DokanOptions #46

Closed JaniruTEC closed 4 years ago

JaniruTEC commented 4 years ago

When implementing DokanFileSystem#zwCreateFile one has two objects that wrap the same native DokanOptions-Object: DokanFileSystem#dokanOptions written on mount and DokanFileInfo#DokanOpts which is generated from the pointer every time the method is invoked.

Tests show that those two references do indeed point to the same native address, but are out of sync. From my perspective the native object is (wrongly!) changed after mount which results in differing values that shouldn't be different. My tests with this code (branch / https://github.com/JaniruTEC/dokan-java/commit/8b636ca8c05052162127753bead7204ff765c754) yield the following results: This: DeviceOptions(Version=110, ThreadCount=5, Options=138, mountOptions=EnumIntegerSet(elements=[STD_ERR_OUTPUT, WRITE_PROTECTION, CURRENT_SESSION]), GlobalContext=0, MountPoint=M:, UNCName=null, Timeout=3000, AllocationUnitSize=4096, SectorSize=512)

Parameter: DeviceOptions(Version=110, ThreadCount=5, Options=138, mountOptions=EnumIntegerSet(elements=[STD_ERR_OUTPUT, WRITE_PROTECTION, CURRENT_SESSION]), GlobalContext=0, MountPoint=M:, UNCName=null, Timeout=2199023258552, AllocationUnitSize=512, SectorSize=512)

It shows that only two fields are affected (Timeout and AllocationUnitSize). It seems that this error has it's origin in @dokan-dev/dokan-core. Maybe you guys know more.

Liryna commented 4 years ago

Hi @JaniruTEC ,

That's weird, I have no idea how does this could happen. DokanOptions provided at mount time is copied to our internal Dokan instance. https://github.com/dokan-dev/dokany/blob/6a4144f6ee723dc22ead433f835dccea75678f42/dokan/dokan.c#L239 then it is assign to all DOKAN_FILE_INFO before calling the user function https://github.com/dokan-dev/dokany/blob/6a4144f6ee723dc22ead433f835dccea75678f42/dokan/create.c#L133 Especially Timeout is never touched in the library 😮

JaniruTEC commented 4 years ago

Hi @Liryna,

thanks for looking into this. My own research into Dokany shows the same results. I see no reason why this could possibly happen.

Judging from the way Timeout behaves my idea would be an overflow?

Liryna commented 4 years ago

Yes! Indeed this could be the reason but as we do not touch it, I cannot say where does it comes from :( Maybe debugging with VS on a break point when value changed would help?

JaniruTEC commented 4 years ago

I'm not a C++ Developer (and I therefore have no idea how to debug dokany) but I guess, I found it: dokan.h defines Timeout as ULONG, which is a 32-bit unsigned integer, according to this and JNA. See: https://github.com/dokan-dev/dokany/blob/6a4144f6ee723dc22ead433f835dccea75678f42/dokan/dokan.h#L144

DokanOptions in dokan-java defines the matching field (Timeout) as a java long, which is a 64-bit signed integer. See: https://github.com/dokan-dev/dokan-java/blob/43b8d1344c5e62f24e9d78f48d1d16f1317455a3/src/main/java/dev/dokan/dokan_java/structure/DokanOptions.java#L56

I wrote https://github.com/JaniruTEC/dokan-java/commit/b0433acfbe68c1f360f678d64b63b127016f65db as a test and found those bitmasks: ....This: 0000 0000 0000 0000 0000 00‭*0*0 0000 0000 | 0000 0000 0000 0000 0000 1011 1011 1000‬ Paramter: 0000 0000 0000 0000 0000 00‭*1*0 0000 0000 | 0000 0000 0000 0000 0000 1011 1011 1000‬ These map to 3000 and 2199023258552, accordingly. (The pipe (|) is 32-bit-mark)

Changing DokanOptions to use JNA's ULONG (https://github.com/JaniruTEC/dokan-java/commit/adc12b9b205bd09e2e3e98bd313da6be0c554876) fixed the problem, making both return 0000 0000 0000 0000 0000 00‭*0*0 0000 0000 | 0000 0000 0000 0000 0000 1011 1011 1000‬ or 3000.

Obviously the conversion was causing this. But I still have not idea, why on earth a bit is set at index 41 of an unsigned 32-bit int.

EDIT I suppose something similiar is happening with all other longs in DokanOptions (especially AllocationUnitSize). The value is probably increased, leading it to fail this check (https://github.com/dokan-dev/dokany/blob/6a4144f6ee723dc22ead433f835dccea75678f42/dokan/dokan.c#L159-L171), causing Dokany to reset it to it's default value (512). I'm not a C++-Dev as i said, so I would need help with verifying this hypotheses and/or debugging Dokany on my own.

Liryna commented 4 years ago

Well found! This does looks to be the correct cause of the issue.

Does that mean Options was also a 64bits ? This would have slide all the data from the struct. We would need to look at the struct padding and fields position to know what could have set this field. Not sure that really needed here as the reason was found now.

JaniruTEC commented 4 years ago

Options is also a 64bit long, yes.

Is it possible that wrong padding can cause JNA to read data from another field? We're not talking about a Gameboy after all.

But if we are sure that this is, what's happening, then no further analysis on the issue will be needed. The dokan-java team will have to check all structs for similar problems tho.

@infeo Could you verify our solution, please?

infeo commented 4 years ago

Well this is interesting. On my computer everything works as expected without any fix, i.e. there is no magic bit "flip" in the fields of DokanOptions.

I mean, the error is already anaylzed and it is clear where the problem is, but it seems like it only occurs on specific setups.

@JaniruTEC What Dokan version is installed on your system?

JaniruTEC commented 4 years ago

This could lie in the nature of this error. We can't know for sure which bits are set on which addresses; this bit could possibly come from anywhere on my computer.

I use Dokan 1.3.1.1000.

infeo commented 4 years ago

Well, then i don't think I can verify here a lot.^^

Changing the type to JNA's ULONG should solve it. Of course it decreases performance because we wrap the primitive type with an object, but JNA was never built for it in the first way.