Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Mismatching DokanOptions #46

Closed
JaniruTEC opened this issue Jun 5, 2020 · 9 comments · Fixed by #48
Closed

Mismatching DokanOptions #46

JaniruTEC opened this issue Jun 5, 2020 · 9 comments · Fixed by #48
Assignees
Labels
Milestone

Comments

@JaniruTEC
Copy link
Member

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 / JaniruTEC@8b636ca) 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.

@JaniruTEC JaniruTEC added the bug label Jun 5, 2020
JaniruTEC added a commit to JaniruTEC/dokan-java that referenced this issue Jun 8, 2020
Updated DokanFileHandle to use ROMountInfo instead of DokanOptions
Added setFileInfo method

This commit led to discovery of dokan-dev#46 which leads to problems with this code.
@Liryna
Copy link
Member

Liryna commented Jun 9, 2020

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
Copy link
Member Author

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
Copy link
Member

Liryna commented Jun 9, 2020

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
Copy link
Member Author

JaniruTEC commented Jun 9, 2020

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:

I wrote JaniruTEC@b0433ac 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 (JaniruTEC@adc12b9) 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
Copy link
Member

Liryna commented Jun 10, 2020

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
Copy link
Member Author

JaniruTEC commented Jun 10, 2020

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
Copy link
Member

infeo commented Jun 13, 2020

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
Copy link
Member Author

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
Copy link
Member

infeo commented Jun 16, 2020

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.

@JaniruTEC JaniruTEC added this to the 2.0 milestone Jun 17, 2020
@JaniruTEC JaniruTEC mentioned this issue Jun 18, 2020
@JaniruTEC JaniruTEC linked a pull request Jun 21, 2020 that will close this issue
JaniruTEC added a commit that referenced this issue Jul 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants