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

Open read-only file with FILE_FLAG_DELETE_ON_CLOSE flag #1255

Closed
5 tasks done
denisovpi opened this issue Dec 22, 2024 · 3 comments
Closed
5 tasks done

Open read-only file with FILE_FLAG_DELETE_ON_CLOSE flag #1255

denisovpi opened this issue Dec 22, 2024 · 3 comments

Comments

@denisovpi
Copy link

Environment

  • Windows version: Win11 Pro 23H2
  • Processor architecture: x86-64
  • Dokany version: v2.2.0.1000
  • Library type (Dokany/FUSE): Dokany

Check List

  • 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
  • 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

I'm working on implementing our own file system. I took dokan_memfs as an example. The file creation function has two checks for specific conditions:

  • Cannot delete a file with readonly attributes.

    // Cannot delete a file with readonly attributes.
    if ((f && (f->attributes & FILE_ATTRIBUTE_READONLY) ||
    (file_attributes_and_flags & FILE_ATTRIBUTE_READONLY)) &&
    (file_attributes_and_flags & FILE_FLAG_DELETE_ON_CLOSE))
    return STATUS_CANNOT_DELETE;

  • Cannot open a readonly file for writing.

    // Cannot open a readonly file for writing.
    if ((creation_disposition == OPEN_ALWAYS ||
    creation_disposition == OPEN_EXISTING) &&
    f && (f->attributes & FILE_ATTRIBUTE_READONLY) &&
    desiredaccess & FILE_WRITE_DATA)
    return STATUS_ACCESS_DENIED;

I noticed that if I change the order of these two conditions, the code will no longer pass the tests. This also happened with the implementation of the dokan-rust-memfs. I created an issue.
I think that the implementation of external file systems should not affect the logic of the Dokany itself. This is a bug or this case should be described in the documentation or in the code example itself.

Tests for winfstest

expect("CreateDirectory %s 0" % name, 0)
expect("CreateFile %s\\f GENERIC_WRITE 0 0 CREATE_ALWAYS FILE_ATTRIBUTE_READONLY 0" % name, 0)
expect("CreateFile %s\\f GENERIC_WRITE 0 0 OPEN_EXISTING FILE_FLAG_DELETE_ON_CLOSE 0" % name, "ERROR_ACCESS_DENIED")

Output:
not ok 3 - expect "CreateFile M:\81981b9e\f GENERIC_WRITE 0 0 OPEN_EXISTING FILE_FLAG_DELETE_ON_CLOSE 0" ERROR_ACCESS_DENIED - got 0

@Liryna
Copy link
Member

Liryna commented Dec 22, 2024

Hi @denisovpi ,

Thanks for pointing that out!
The test is failing because STATUS_ACCESS_DENIED triggers the library to check if the parent allows the deletion (normal Windows behavior).

dokany/dokan/create.c

Lines 250 to 254 in 2165b60

if (STATUS_ACCESS_DENIED == status &&
IoEvent->DokanInstance->DokanOperations->ZwCreateFile &&
(IoEvent->EventContext->Operation.Create.SecurityContext.DesiredAccess &
DELETE)) {
DbgPrint("Delete failed, ask parent folder if we have the right\n");

I agree that the order shouldn't affect the behavior but the rule is actually to return the most precise and correct status for the operation. STATUS_CANNOT_DELETE is more precise type of error than STATUS_ACCESS_DENIED. The documentation isn't clear about that but does say to return the proper value.
How do you think we should improve it ?

@denisovpi
Copy link
Author

Hi, @Liryna,
I can't tell if something needs to be changed, but I agree that the code should return the most correct status.
I'll create a PR to fix the dokan-rust-memfs and add the test to the winfstest.
Thanks for your help.

Liryna added a commit that referenced this issue Dec 24, 2024

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
@Liryna
Copy link
Member

Liryna commented Dec 24, 2024

Thanks @denisovpi ! Please ping me on te rust PR 👍 I will be glad to review it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants