DokanCreateGlobalDiskDevice() object initialization appears incorrect

[d-hoke]

Feature request can skip this form. Bug report must complete it. Check List must be 100% match or it will be automatically closed without further discussion. Please remove this line.

Environment

• Windows version:
• Processor architecture:
• Dokany version:
• Library type (Dokany/FUSE):

Check List

• [x] I checked my issue doesn't exist yet
• [ ] My issue is valid with mirror default sample and not specific to my user-mode driver implementation
• [x] I can always reproduce the issue with the provided description below.
• [ ] I have updated Dokany to the latest version and have reboot my computer after.
• [ ] I tested one of the last snapshot from appveyor CI

Description

Note: I am fairly new to device drivers, so please be gentle if you find the following seems inaccurate - but I thought it would be worth mentioning for examination.

Exploration of code observes that DokanCreateGlobalDiskDevice() does not seem to be correctly initializing the dokanGlobal deviceExtension entity.

Concerns observed: 1)assignments are made to dokanGlobal ->DeviceObject ->FsDiskDeviceObject ->FsCdDeviceObject and then following that the entire dokanGlobal object is ZEROed with RtlZeroMemory, thus presumably making those previous assignments useless. 2) The dokanGlobal->Resource entity is not initialized with ExInitializeResourceLite(), anywhere that I have been able to find. (It appears that this may be OK, due, as it might be that 'ZERO'd memory is an acceptable initialization - but not sure you can count on that.)

[ 3)(not initialization, but may not get further examination nor another issue written) A separate issue may be that the dokanGlobal->Resosurce, acquisition and release may not be handled properly elsewhere - I mention it here because don't know how much further I'll go trying to determine that. From observation so far, it is not quickly evident that it is always being allocated/held at appropriate IRQL level. If so, this could be cause of some 'thread'-related issues that have been reported. ]

Logs

Please attach in separate files: mirror output, library logs and kernel logs. In case of BSOD, please attach minidump or dump analyze output.

[Liryna]

Hi @d-hoke ,

1. Wow, this is totally unexpected 😮 you are right ! I checked and this comes from dokan 0.6.0 (legacy)! (way before the dokany fork) Amazing that we have not seen it before. I fixed it here: https://github.com/dokan-dev/dokany/commit/a0d81b09a3335607546d2a4bd93fe919d1822e93

2. You are also right here 👍 I added ExInitializeResourceLite (previous commit) and ExDeleteResourceLite here: https://github.com/dokan-dev/dokany/commit/a56a217d39edcdd5ca1bf61f839a1ac0caa0c5d1

3. It would be very nice if you could review it ❤️

Thanks a lot for the report ! It is very helpful having other eyes reviewing the project !

[d-hoke]

code review On https://github.com/dokan-dev/dokany/commit/a0d81b09a3335607546d2a4bd93fe919d1822e93, are all members of the DeviceExtension being set by some other means now? (I see you added an assignment to ->MountId.)
Did you verify that all DOKAN_GLOBAL members are now initialized with that addition (and having removed the RtlZeroMemory()) ?

Regarding the the delete (https://github.com/dokan-dev/dokany/commit/a56a217d39edcdd5ca1bf61f839a1ac0caa0c5d1), the one you added looks Ok, but there are other areas in DriverEntry() code that may err where it should probably also be done as well (in DriverEntry, other setup after return from call to DokanCreateGlobalDiskDevice() ). I see at least: potential failure of fastIoDispatch = ...; FsRtlRegisterFileSystem() DokanLookasideCreate() x 3 And, is there any overhead to any of the other init'd DOKAN_GLOBAL items that should have cleanup added as well, before the deviceObject (and thus deviceExtension) is deleted?

This should probably be another issue, but I noticed while reviewing that DokanCreateGlobalDiskDevice() does not properly cleanup extraneous items (fsDiskDeviceObject, fsCdDeviceObject) if failure occurs there on IoCreateSymbolicLink(), where it deletes the deviceObject only...

[Liryna]

Did you verify that all DOKAN_GLOBAL members are now initialized with that addition (and having removed the RtlZeroMemory()) ?

Yes 👍 all are initialized without the RtlZeroMemory

there are other areas in DriverEntry() code that may err where it should probably also be done as well (in DriverEntry, other setup after return from call to DokanCreateGlobalDiskDevice() ).

I have added a function to cleanup all disk device data https://github.com/dokan-dev/dokany/commit/de7d1d8fda83546a8bd60a1cb8d299874d25c19b Some data was not cleaned in some place 👍

I noticed while reviewing that DokanCreateGlobalDiskDevice() does not properly cleanup extraneous items (fsDiskDeviceObject, fsCdDeviceObject) if failure occurs there on IoCreateSymbolicLink(), where it deletes the deviceObject only...

Done ! https://github.com/dokan-dev/dokany/commit/2851823a56610fde6df06d4eab2f6f24a3349fb6

Thank a lot ❤️ !

[d-hoke]

Don't immediately see anything wrong with what you've done, but I'm tired at the moment so may review again later. That said...

One more thing I see (saw before, suspicious, but forgot), is the use of ObReferenceObject(deviceObject) in DokanCreateGlobalDiskObject().

It appears that the deviceObject is never unreferenced, which I think means it won't ever actually be deleted, based on information @ "https://msdn.microsoft.com/en-us/library/windows/hardware/ff549083%28v=vs.85%29.aspx", "When a driver calls IoDeleteDevice, the I/O manager deletes the target device object if there are no outstanding references to it.

So, at any of the IoDeleteDevice(deviceObject) after ObReferenceObject(), I think ObDereferenceObject() has to be called beforehand. (Maybe afterwards would work too, don't know.)

[Liryna]

Looks like there is a some missing 😢

Find all "ObReferenceObject", Find Results 1, Entire Solution, ""
  E:\dokan\dokany\sys\fscontrol.c(617):  ObReferenceObject(volDeviceObject);
  E:\dokan\dokany\sys\init.c(607):  ObReferenceObjectByHandle(thread, THREAD_ALL_ACCESS, NULL, KernelMode,
  E:\dokan\dokany\sys\init.c(891):  ObReferenceObject(deviceObject);
  E:\dokan\dokany\sys\init.c(937):  ObReferenceObject(fsDiskDeviceObject);
  E:\dokan\dokany\sys\init.c(938):  ObReferenceObject(fsCdDeviceObject);
  E:\dokan\dokany\sys\init.c(997):    ObReferenceObjectByHandle(handle, THREAD_ALL_ACCESS, NULL, KernelMode,
  E:\dokan\dokany\sys\init.c(1056):          ObReferenceObjectByHandle(handle, THREAD_ALL_ACCESS, NULL, KernelMode,
  E:\dokan\dokany\sys\init.c(1102):          ObReferenceObjectByHandle(handle, THREAD_ALL_ACCESS, NULL, KernelMode,
  E:\dokan\dokany\sys\init.c(1306):    ObReferenceObject(diskDeviceObject);
  E:\dokan\dokany\sys\init.c(1375):        ObReferenceObjectByHandle(handle, THREAD_ALL_ACCESS, NULL, KernelMode,
  E:\dokan\dokany\sys\notification.c(374):  ObReferenceObjectByHandle(thread, THREAD_ALL_ACCESS, NULL, KernelMode,
  E:\dokan\dokany\sys\pnp.c(98):    ObReferenceObject(DeviceObject);
  E:\dokan\dokany\sys\timeout.c(333):  ObReferenceObjectByHandle(thread, THREAD_ALL_ACCESS, NULL, KernelMode,
  Matching lines: 13    Matching files: 5    Total files searched: 76

  
  Find all "ObDereferenceObject", Find Results 1, Entire Solution, ""
  E:\dokan\dokany\sys\init.c(112):  ObDereferenceObject(mountFileObject);
  E:\dokan\dokany\sys\init.c(1001):    ObDereferenceObject(thread);
  E:\dokan\dokany\sys\init.c(1060):          ObDereferenceObject(thread);
  E:\dokan\dokany\sys\init.c(1106):          ObDereferenceObject(thread);
  E:\dokan\dokany\sys\init.c(1379):        ObDereferenceObject(thread);
  E:\dokan\dokany\sys\notification.c(395):      ObDereferenceObject(Dcb->EventNotificationThread);
  E:\dokan\dokany\sys\timeout.c(360):    ObDereferenceObject(Dcb->TimeoutThread);
  Matching lines: 7    Matching files: 3    Total files searched: 76