Dokan does not pass the full range of dwFlagsAndAttributes to the DOKAN_OPERATIONS.CreateFile callback

[Corillian]

User mode CreateFile() combines flags and attributes into a single 32-bit value called dwFlagsAndAttributes however it gets split between two parameters, FileAttributes and CreateOptions, when CreateFile() gets translated into its kernel mode equivalent ZwCreateFile(). Dokan.dll DispatchCreate() does not translate the kernel mode ZwCreateFile() CreateOptions back into their user mode CreateFile() dwFlagsAndAttributes equivalents which results in these values being lost.

[marinkobabic]

You are right. Driver passes all correct to usermode dll splittet into CreateOptions and FileAttributes. FileAttributes are used but CreateOptions not really. CreateOptions are just used to get the createDisposition and to set the delete on close.

Your expectations is that FileAttributes contains also all the flags set in the CreateOptions or a new parameter is need? What is the actual flag you are missing? In generally all flags should be passed to the call back method.

[Corillian]

My expectation is that in the user mode call to DOKAN_OPERATIONS.CreateFile dwFlagsAndAttributes should have all of the flags that were passed into the original call to CreateFile(). It's missing all flags with values above 0xffff. If you look at https://msdn.microsoft.com/en-us/library/windows/desktop/aa363858(v=vs.85).aspx that's quite a few flags - the most important of which, for my purposes, are those controlling caching behavior.

[Corillian]

Reading more about ZwCreateFile() I think that the best solution would be changing the current DOKAN_OPERATIONS.CreateFile argument list to mirror that of ZwCreateFile(). There's a lot of information that gets lost by attempting to convert the information supplied to ZwCreateFile() to the format accepted by CreateFile(). If maintaining backwards compatibility is a concern we could always create another callback called DOKAN_OPERATIONS.ZwCreateFile however I think in this case it would be best to simply change the existing DOKAN_OPERATIONS.CreateFile and force anyone who's upgrading to update their CreateFile callback.

[Liryna]

If too much informations are lost in the current CreateFile(), I see no problem to break the compatibility.
I would even accept to put it in the 0.8.0 since it already break the compatibility and still a pre-release.

[Corillian]

OK I can take care of this once I finish #79 as I would like both of these issues to be resolved for the next release.

[Liryna]

Hello @Corillian & @billziss-gh,

You have both proposed a pull request for the same purpose but in different implementation.

billziss-gh seems to be the same as Corillian but without the lpSecurityAttributes that he have prefer to keep empty.
To quote billziss-gh

I have reviewed this patch and have found that it presents a rather complex interface to the file system implementor (DOKAN_IO_SECURITY_CONTEXT).

The documentation has this to say about lpSecurityAttributes:

    The lpSecurityDescriptor member of the structure specifies a SECURITY_DESCRIPTOR for
    a file or device. If this member is NULL, the file or device associated with the returned
    handle is assigned a default security descriptor.
    CreateFile ignores the lpSecurityDescriptor member when opening an existing file or device.

So when creating a new file it is possible to attach a security descriptor to it. This capability does not seem to exist in Dokan currently. This is why I added the Reserved security descriptor parameter, so that we can pass it at a later time from the driver.

I agree that DOKAN_IO_SECURITY_CONTEXT is complex but, if we keep it, will it be usefull in the futur ? or would we need to rewrite it anyway ?

And also bill has not override the CreateFile, but propose a CreateFileEx, that's really something I like !

Corillian has created DokanMapKernelToUserCreateFileFlags that I would like to keep in any case to help the dev.

I am saying this because you have made both a great work and you have both good ideas. It would be great if you could both work together to have one proposition.

[Corillian]

@Liryna we had discussed the possibility of adding a separate function above. I suggested the name ZwCreateFile since its purpose is to mirror the kernel mode function of the same name. This tells the user what to google to look up the associated Windows documentation. In the end we had concluded that updating CreateFile was a better option as the next release breaks backward compatibility anyway. If you've changed your mind that's fine however I personally believe it's better to force an upgrade as it explicitly makes the user aware of new changes which I believe are important.

As for the SECURITY_DESCRIPTOR my understanding is that it's needed in the case where a file system supports the storage of security information such as NTFS. After doing some more research (https://msdn.microsoft.com/en-us/library/windows/hardware/ff538915(v=vs.85).aspx) I need to make some modifications to my SECURITY_DESCRIPTOR code. According to that article a new SECURITY_DESCRIPTOR should be created via SeAssignSecurity() and the result of that is what I should be passing through. Either way a SECURITY_DESCRIPTOR should be passed through to the user mode driver.

I say we do the right thing now so that we don't have to worry about it in the future. I can fix up the

[billziss-gh]

Hello, Liryna and Corillian:

I wrote my PR a couple of days ago for a few reasons:

• I needed CreateFileEx (or an equivalent) to fix a few problems in my file system (related to missing the CreateOptions parameter).
• I believe that the split between CreateFile, CreateDirectory, OpenDirectory is not necessarily helpful, because the implementor has to implement opening directories in CreateFile anyway.
• At the time it appeared that Corillian's PR was not complete and I was not sure about its timeline for completion.

Furthermore I reviewed Corillian's PR and I found what IMO are some concerns:

• It breaks every file system implementation out there by fundamentally changing the CreateFile signature. I understand that we recently changed Win32 error codes to NTSTATUS, but this is a much bigger change.
• It maintains the split between CreateFile, CreateDirectory, OpenDirectory. This is problematic for the reason mentioned earlier but also because CreateDirectory and OpenDirectory do not get the extra parameters that they need in order to work. For example, what security descriptor is to be applied to a newly created directory in CreateDirectory?
• As a file system implementor I am not sure what to do with DOKAN_IO_SECURITY_CONTEXT. There seems to be a wealth of information in there, but most of it seems to be information that should be dealt with at the driver level.
• I am not a maintainer of this project, but I felt that a PR that touches so many files can be dangerous. I am in favor of smaller and simpler patches that are easily understood.

Regarding the Reserved security descriptor in my PR: this is currently reserved and will always be zero. The intent is to (minimally) modify the driver to pass the security descriptor, so that it can be attached to newly created files. This change will happen in the near future. [But not in the same PR.]

[Corillian]

Addressing @billziss-gh's concerns:

1. As mentioned above I don't consider this an issue. With the helper function I created this is a trivial change which is clearly documented in the Mirror example code.
2. I agree with you that we should look into removing those functions however I kept my PR within the scope of Dokan does not pass the full range of dwFlagsAndAttributes to the DOKAN_OPERATIONS.CreateFile callback #83. I have no problem with increasing the scope of the PR to cover those changes - particularly given that we're already instituting a breaking API change. We could also create a separate issue to cover those changes.
3. The user mode code is still technically the driver level. We should not remove information relevant to the implementation of a file system driver just because we think it's hard to understand. That's how we ended up in this mess with missing flags in the first place. It's better to provide all of the information that may be needed along with helper functions to cover the most common usage scenarios.
4. That's a pretty arbitrary measurement of danger

I don't see the point in having a reserved parameter when you know you're going to need to solve that problem anyway.

[billziss-gh]

@Corillian:

Re: 1. I understand that it is not an issue for you. But it may be for other file system implementors.
Re: 3. Can you please document for me what I am supposed to do with the information I get through DOKAN_IO_SECURITY_CONTEXT? What fields are important? How should I use them?

[Corillian]

1. I'm pretty confident that anybody with the competence to write a file system driver can figure out they need to change a function signature and, at the very least, call a single helper function to get the same parameters they were using before.

2. https://msdn.microsoft.com/en-us/library/windows/hardware/ff550613(v=vs.85).aspx
   https://msdn.microsoft.com/en-us/library/windows/hardware/ff540431(v=vs.85).aspx
   https://msdn.microsoft.com/en-us/library/windows/hardware/ff538915(v=vs.85).aspx

[billziss-gh]

Thanks for the documentation pointers. To clarify I am aware of the IO_SECURITY_CONTEXT and ACCESS_STATE documentation, but that does not make me much wiser on what to do with them. Perhaps I am in the minority.

In any case I have registered my concerns and my reasons for creating the alternative PR.

[xgid]

If it helps, let me say that I think ALL we know that Dokan was left in a state that was not production-ready which means that MAJOR changes have to be made to take it to the desired point. IMHO this means that breaking API changes are probably inevitable but, most important, it's better to make them now than later. And trying to simplify complex things in a way that gives a "handicapped" solution in my experience is not a good way. It's better to be able to address the full complexity of the problem, but provide "simplified paths" for the common use cases as you all are already trying to provide in your PRs.

It would be great to have all the power of your solutions together in Dokany as @Liryna is asking for. Keep the good work and thank you for your efforts!

PS: I'm sorry I don't have the skills nor the knowledge to help with the code as I would wish.

[Liryna]

Thank you @billziss-gh, @Corillian & @xgid for all your inputs !
Such talks are very important for being able to make the right moves.

So I think we all agree that:

• Since 0.8.0 already break compatibility with NtStatus and some flags removed. We can afford to continue the changes since 0.8.0 is in pre-release for now.
  If we replace/rewrite a DokanOperation, it have to keep the same complexity.
  If the new DokanOperation is too different and complex, it need to be separated and optional.
• @Corillian (with the helpers) & @billziss-gh have the same level of complexity. From what we see in both PR, mirror.c has not really moved so anybody can easily make the migration.
  But we probably should (like @Corillian said at first 😋 ) rename CreateFile to ZwCreateFile for not missleading the dev and inform guide him to the correct msdn documentations.
• @Corillian implementation is more advanced since he have already begin to manage security context.
  But I agree with @billziss-gh, I think a lots of users will not care about security context so it have to be optional to manage it some how.
• Since we add the security context parameter, it have to be implemented right now to prevent to make changes in the futur.
• We should right now merge CreateFile, CreateDirectory, OpenDirectory to the single fonction ZwCreateFile. Does everyone agree with it ?

If what I wrote is ok for everyone:
@Corillian , Could you rename CreateFile to ZwCreateFile ?
Also, do you think you can implement IO_SECURITY_CONTEXT totally ? (and give a simple exemple of test 😃 )
@billziss-gh , does winfstest have already a stable release ? I really would like to add it to Appveyor CI !

[billziss-gh]

I believe the name ZwCreateFile is misleading. Here is what the real ZwCreateFile prototype looks like:

NTSTATUS ZwCreateFile(
  _Out_    PHANDLE            FileHandle,
  _In_     ACCESS_MASK        DesiredAccess,
  _In_     POBJECT_ATTRIBUTES ObjectAttributes,
  _Out_    PIO_STATUS_BLOCK   IoStatusBlock,
  _In_opt_ PLARGE_INTEGER     AllocationSize,
  _In_     ULONG              FileAttributes,
  _In_     ULONG              ShareAccess,
  _In_     ULONG              CreateDisposition,
  _In_     ULONG              CreateOptions,
  _In_opt_ PVOID              EaBuffer,
  _In_     ULONG              EaLength
);

My opinion is that if we are changing the API, we should also purge CreateDirectory and OpenDirectory.

[EDIT: just reread @Liryna's message and saw she is already saying that CreateDirectory, OpenDirectory should be purged. Apologies for missing that.]

The NTSTATUS checker should be cleaned up. It is currently overcomplicated. It should be a simple mapping between the CreateDisposition and the corresponding NTSTATUS to return. Along the lines of:

    switch (disposition) {
    case FILE_CREATE:
        eventInfo.Operation.Create.Information = FILE_CREATED;
        break;
    case FILE_OPEN_IF:
    case FILE_OPEN:
        eventInfo.Operation.Create.Information = FILE_OPENED;
        break;
    case FILE_OVERWRITE:
    case FILE_OVERWRITE_IF:
        eventInfo.Operation.Create.Information = FILE_OVERWRITTEN;
        break;
    case FILE_SUPERSEDE:
        eventInfo.Operation.Create.Information = FILE_SUPERSEDED;
        break;
    default:
        break;
    }

STATUS_OBJECT_NAME_COLLISION should not be handled specially. I note that ERROR_ALREADY_EXISTS is added by the Win32 CreateFileW using a test along the lines of:

if (disposition == CREATE_ALWAYS && iosb.Information == FILE_OVERWRITTEN) ||
    disposition == OPEN_ALWAYS && iosb.Information == FILE_OPENED)) {
    SetLastError(ERROR_ALREADY_EXISTS);
}

SL_OPEN_TARGET_DIRECTORY is not handled properly. For one thing, we should return FILE_EXISTS, FILE_DOES_NOT_EXIST based on whether the original pointed file exists (not the target directory). In my tests with MoveFileExW everything worked when setting the information code to FILE_OPENED though.

The information codes FILE_EXISTS and FILE_DOES_NOT_EXIST are not used except in the case of SL_OPEN_TARGET_DIRECTORY. Confirmed on Fastfat and NTFS.

Also for consideration: eventInfo need not be a malloc allocation. This seems superfluous (unless I am missing something)

Regarding winfstest: It is located at https://bitbucket.org/billziss/secfs.test, subdirectory winfstest. I have it under a BSD license, i.e. up for grabs. It requires Python (2.7) to run. It can run on both Windows and Cygwin Python. Change to the drive/directory you wish to test and use run-winfstest.bat on Windows and run-winfstest (shell script) on Cygwin.

There are only a few number of tests currently, but they exercise a fair amount of the Dokan API. All tests pass on NTFS. Many tests fail on mirror.exe and most pass on my own file system. Handling ShareAccess is a particular sticky point (I believe @Liryna you have said that these should and will be handled at the driver level and I agree with you).

I would expect that we would need about 10 times as many tests to have a comprehensive test suite though.

[Corillian]

@billziss-gh it's required for handling access rights, security, and auditing for the ZwCreateFile operation. The links I provided give additional information on what that means and why you would need it. I don't see how using ZwCreateFile is any more misleading than CreateFile or CreateFileEx. The IRP handler more closely resembles ZwCreateFile than anything else we could call it.

I don't think Dokan should handle ShareAccess unless it's something that can optionally be disabled. My driver needs to handle ShareAccess itself though I can imagine that's uncommon. Any modifications to the NTSTATUS handling and ShareAccess should probably be performed under a separate Issue/PR.

@Liryna Ok I shall rename CreateFile to ZwCreateFile. I hadn't included the auditing information in my implementation of DOKAN_ACCESS_STATE since the MSDN docs currently says it's unused but I'll add it just to make sure we're as future proof as possible. It should be noted that according to the MSDN docs (https://msdn.microsoft.com/en-us/library/windows/hardware/ff538878(v=vs.85).aspx) all of the actual system auditing functions are kernel mode so even though I'll be passing the auditing info to the user mode driver a proper implementation may require Dokan to optionally implement it itself.

I'll also update my SECURITY_DESCRIPTOR code to call SeAssignSecurity() (https://msdn.microsoft.com/en-us/library/windows/hardware/ff538915(v=vs.85).aspx) and pass the result of that to the user mode driver within our DOKAN_ACCESS_STATE. Lastly I'll go ahead and merge CreateDirectory and OpenDirectory.

[billziss-gh]

Understood. I do not believe I have anything else to add as I do not believe any input is taken into consideration anyway.

[Liryna]

@billziss-gh , I consider everything proposition. Removing CreateDirectory & OpenDirectory is one of your.
Could you create different issues for the different points that you have talk in your last post ?
I just would like to finish the CreateFile talk first.

But quickly: I am totally open to discuss about NtStatus. I agree that NtStatus is a big mess for now in dispatchcreate and it need to be changed. It is such part of the code that hard to change without breaking everything. But now that there is winfstest, we can rewrite this part from 0 and test it !
(About Standardize code format, I seen your message and I will do it after this PR also)

About ZwCreateFile name, it is right that it is not going to be exactly ZwCreateFile but since NtCreateFile documentation say : This function is the user-mode equivalent to the ZwCreateFile and we keep the same name of some parameters. The dev will be able to understand from msdn what is the purpose of them 😃 no ?

I also think eventInfo should not be dynamically allocated. And I am sure there is a lot of such in all dokan 😢

About winfstest, it is a great news! Appveyor have normally python installed. So I will add it in the futur.

About ShareAccess, @Corillian , I also think that it have to be optional when it gonna be made.
@billziss-gh , does every of you ShareAccess test fail with the mirror ? do you think the mirror could be fix without big changes ? (I prefer to keep it simple)

[billziss-gh]

@Liryna to clarify: my disagreement is with @Corillian and not you.

Re: winfstest. Perhaps we should open a new issue to discuss that as to not pollute this thread?

The mirror passes all ShareAccess tests, but only by accident. It simply forwards them back to Win32, which of course handles ShareAccess properly. There are a few other tests, but most have to do with wrong status codes returned, so easily fixable. There is also a more important failure about DeleteFile.

C:\Users\billziss\Projects\secfs\secfs\test\winfstest\t\base\00.t  ok
C:\Users\billziss\Projects\secfs\secfs\test\winfstest\t\base\01.t  ok
C:\Users\billziss\Projects\secfs\secfs\test\winfstest\t\base\02.t
not ok 2 - expect "CreateDirectory 318b3816 0" ERROR_ALREADY_EXISTS - got ERROR_ACCESS_DENIED
not ok 3 - expect "RemoveDirectory 318b3816" ERROR_DIRECTORY - got ERROR_INVALID_FUNCTION
not ok 6 - expect "CreateDirectory 318b3816 0" ERROR_ALREADY_EXISTS - got ERROR_ACCESS_DENIED
not ok 8 - expect "DeleteFile 318b3816" ERROR_ACCESS_DENIED - got 0
C:\Users\billziss\Projects\secfs\secfs\test\winfstest\t\base\02.t  not ok 4/10
C:\Users\billziss\Projects\secfs\secfs\test\winfstest\t\base\03.t
not ok 4 - expect "RemoveDirectory f4cc1675\foo" ERROR_DIRECTORY - got ERROR_INVALID_FUNCTION
C:\Users\billziss\Projects\secfs\secfs\test\winfstest\t\base\03.t  not ok 1/18
C:\Users\billziss\Projects\secfs\secfs\test\winfstest\t\base\04.t
not ok 7 - expect "MoveFileEx bb9253c6\bar bb9253c6\baz 0" ERROR_ALREADY_EXISTS - got ERROR_ACCESS_DENIED
not ok 21 - expect "MoveFileEx bb9253c6\bar bb9253c6\baz 0" ERROR_ALREADY_EXISTS - got ERROR_ACCESS_DENIED
C:\Users\billziss\Projects\secfs\secfs\test\winfstest\t\base\04.t  not ok 2/43
C:\Users\billziss\Projects\secfs\secfs\test\winfstest\t\base\05.t  ok
C:\Users\billziss\Projects\secfs\secfs\test\winfstest\t\base\06.t  ok
C:\Users\billziss\Projects\secfs\secfs\test\winfstest\t\base\07.t  ok
C:\Users\billziss\Projects\secfs\secfs\test\winfstest\t\base\08.t  ok
C:\Users\billziss\Projects\secfs\secfs\test\winfstest\t\base\09.t  ok
C:\Users\billziss\Projects\secfs\secfs\test\winfstest\t\streams\00.t  ok
C:\Users\billziss\Projects\secfs\secfs\test\winfstest\t\streams\01.t
not ok 1 - expect "CreateDirectory 1f004e38:foo 0" ERROR_DIRECTORY - got ERROR_ACCESS_DENIED
not ok 3 - expect "CreateDirectory 1f004e38 0" ERROR_ALREADY_EXISTS - got ERROR_ACCESS_DENIED
not ok 9 - expect "RemoveDirectory 1f004e38:foo" ERROR_DIRECTORY - got ERROR_INVALID_FUNCTION
C:\Users\billziss\Projects\secfs\secfs\test\winfstest\t\streams\01.t  not ok 3/19
C:\Users\billziss\Projects\secfs\secfs\test\winfstest\t\streams\02.t
not ok 26 - expect "FindStreams c6cd42ac" ERROR_HANDLE_EOF - got ERROR_ACCESS_DENIED
C:\Users\billziss\Projects\secfs\secfs\test\winfstest\t\streams\02.t  not ok 1/250

total ................................. ok 460/471 - not ok 11/471

For what it's worth the proposed NTSTATUS checking does not break any of the winfstest tests.

[Liryna]

@billziss-gh Thank for the feed back !
Could you just create a issue for each issue you see ? (you NTSTATUS implementation, DeleteFile, the lucky mirror with ShareAccess and add winfstest to appveyor)
I will look into them one by one 😄

[JohnOberschelp]

Hi Liryna. I agree with all 4 points.

On 10/29/2015 7:03 AM, Liryna wrote:

Thank you @billziss-gh https://github.com/billziss-gh, @Corillian
https://github.com/Corillian & @xgid https://github.com/xgid for
all your inputs !
Such talks are very important for being able to make the right moves.

So I think we all agree that:

Since 0.8.0 already break compatibility with NtStatus and some
flags removed. We can afford to continue the changes since 0.8.0
is in pre-release for now.
If we replace/rewrite a DokanOperation, it have to keep the same
complexity.
If the new DokanOperation is too different and complex, it need to
be separated and optional.

@Corillian <https://github.com/Corillian> (with the helpers) &
@billziss-gh <https://github.com/billziss-gh> have the same level
of complexity. From what we see in both PR, mirror.c has not
really moved so anybody can easily make the migration.
But we probably should (like @Corillian
<https://github.com/Corillian> said at first :yum: ) rename
CreateFile to ZwCreateFile for not missleading the dev and inform
guide him to the correct msdn documentations.

@Corillian <https://github.com/Corillian> implementation is more
advanced since he have already begin to manage security context.
But I agree with @billziss-gh <https://github.com/billziss-gh>, I
think a lots of users will not care about security context so it
have to be optional to manage it some how.

-Since we add the security context parameter, it have to be
implemented right now to prevent to make changes in the futur.

We should right now merge CreateFile, CreateDirectory,
OpenDirectory to the single fonction ZwCreateFile. *Does everyone
agree with it ?*

If what I wrote is ok for everyone:
@Corillian https://github.com/Corillian , Could you rename
|CreateFile| to |ZwCreateFile| ?
Also, do you think you can implement IO_SECURITY_CONTEXT totally ?
(and give a simple exemple of test 😃 )
@billziss-gh https://github.com/billziss-gh , does winfstest have
already a stable release ? I really would like to add it to Appveyor CI !

—
Reply to this email directly or view it on GitHub
#83 (comment).

[Liryna]

#91 has been merged