All CLA requirements met.

This seems wrong to me because we map FILE_ATTRIBUTE_READONLY to the writiable bit, so if someone uses permisions() to remove the write bit, they could expect that remove() would fail, but after this PR it succeeds.

(That is to say, this manifestation of remove() seems inconsistent with how we implement permissions() in addition to making the call no longer atomic)

Additionally this creates a problem if you get ERROR_ACCESS_DENIED for another reason, such as NTFS permissions. This will end up removing the FILE_ATTRIBUTE_READONLY bit, then still get ERROR_ACCESS_DENIED, reporting a failure, but the filesystem is damaged because the attribute was removed.

Here's an example repro of the problem: https://gist.github.com/BillyONeal/dbd60eda8620d17772caebd9b17b0df6

You might be able to resolve this correctly over in __std_fs_remove (

    _STD _Fs_file _Handle(_Target, __std_access_rights::_Delete |  __std_access_rights::_File_read_attributes |  __std_access_rights::_File_write_attributes, _Flags, &_Last_error);
    if (_Last_error == __std_win_error::_Success) {
        // remove the readonly bit using _Handle the same way __std_fs_change_permissions does
    } else if (_Last_error == __std_win_error::_Access_denied) {
        // can't edit the readonly bit, try to open with just delete
        _Handle = _Fs_file(_Target, __std_access_rights::_Delete, &_Last_error)
    }

    if (_Last_error != __std_win_error::_Success) { return; }

    // set the deletion disposition on _Handle as before

Billy O'Neal (@BillyONeal) Thanks for pointing this out. In addition to your proposed change, I think it is also important to re-set the FILE_ATTRIBUTE_READONLY bit , if we fail to delete the file even after clearing it to maintain the integrity of the file system. This is currently not possible with the current implementation of __std_fs_remove alone.
As noted by Casey Carter (@CaseyCarter), in the #1511 discussion, we should avoid doing more work than necessary to delete a normal file(which is created by 'New Text Document'), which are much more in number compared to these special cases.

The change I propose is

auto _Result = __std_fs_remove(_Target.c_str());
        if (_Result._Error == __std_win_error::_Access_denied) {
            // check if read-only bit is set
            // Clear it if it is set, otherwise return
            const auto _Perm_change_err = __std_fs_change_permissions(_Target.c_str(), false, false);
            if (_Perm_change_err != __std_win_error::_Success) {
                _Ec = _Make_ec(_Perm_change_err);
                return false;
            }
            _Result = __std_fs_remove(_Target.c_str());
           // If we fail again re-set the read-only bit.
         
        }

The change I propose is [...]

The problem with that change is that it CreateFiles again; you want to reuse the same handle.

The problem with that change is that it CreateFiles again; you want to reuse the same handle.

Yeah, 5 calls in the worst case. Is there way to pass the HANDLEs from function to function without modifying xfilesystem_abi.h

Then, how do we solve the issue of re-setting the FILE_ATTRIBUTE_READONLY bit, if we fail to delete the file after clearing the same?

I really think that using DeleteFile and RemoveDirectory would be better here. It should avoid the CreateFile calls. (Please correct me if I am missing something here.)

Is there way to pass the HANDLEs from function to function without modifying xfilesystem_abi.h

No. You need to make this change in __std_fs_remove itself.

I really think that using DeleteFile and RemoveDirectory would be better here.

DeleteFile and RemoveDirectory internally call CreateFile but enforce additional restrictions (that the target is a file/directory) that we don't want.

All deletions on NT work like:

1. Open the file.
2. Set the DELETE_ON_CLOSE flag.
3. Close the file.

Then, how do we solve the issue of re-setting the FILE_ATTRIBUTE_READONLY bit, if we fail to delete the file after clearing the same?

Put it back on with SetFileInformationByHandle(Ex), the same way you remove it.

This turned out to be quite big. I am reusing the same HANDLE for the file operations.
Have reset inc\filesystem to the previous commit.
Waiting for a review.

I really think that using DeleteFile and RemoveDirectory would be better here.

DeleteFile and RemoveDirectory internally call CreateFile but enforce additional restrictions (that the target is a file/directory) that we don't want.

All deletions on NT work like:

1. Open the file.
2. Set the DELETE_ON_CLOSE flag.
3. Close the file.

Thanks for the information 😄

Hmmm this is interesting: https://docs.microsoft.com/en-us/windows-hardware/drivers/ddi/ntddk/ns-ntddk-_file_disposition_information_ex:

FILE_DISPOSITION_IGNORE_READONLY_ATTRIBUTE

Maybe you can just supply that along with our existing FILE_FLAG_POSIX_SEMANTICS and it'll just work on current OSes?

This looks really neat.
I can confirm it works on 19H2 (the version I am using).
But are you sure it works for previous versions of Windows?

One more thing why do we do this #undef _SetFileInformationByHandle . I found only one occurrence of this in the entire source tree

It's a leftover from #1194 (where the #define _SetFileInformationByHandle got deleted).

I'll remove it in the next push

Have removed the #undef _SetFileInformationByHandle

Hi there,
Billy O'Neal (@BillyONeal), Stephan T. Lavavej (@StephanTLavavej) any updates regarding this pull request.

Hi there,
Billy O'Neal (@BillyONeal), Stephan T. Lavavej (@StephanTLavavej) any updates regarding this pull request.

Sorry for the delay: we're in a fairly heavy crunch to get C++20 finished, and having trouble finding time to look at much of anything that isn't C++20 feature work. I've moved this back into a Review status so folks will know it's ready for another look, but I can't guarantee when exactly that will happen. Thanks for understanding, I assure you we haven't forgotten this PR ;)

Thanks for the update 😃

Stephan T. Lavavej (@StephanTLavavej) I have made the suggested changes. Please take a look.

Note for the future - GitHub makes it difficult to incrementally review a PR when the branch is force-pushed, so we recommend avoiding force-pushes unless they're really necessary. (This may change in the future as GitHub's UI improves.)

Don't worry about this case, I'll be able to handle the changes 😸 - just thought I'd mention it.

Sibi Siddharthan (@SibiSiddharthan) Looks great! I pushed a merge with main and a one-line change to simplify _Last_error's initialization.

Miya Natsuhara (@mnatsuhara) FYI, this PR has changed since you approved it back in January. It's still accomplishing the same goal, just in a different way. I know you're busy with NT validation at the moment - no action requested, just wanted to highlight this.

Thanks for fixing this significant correctness bug - and congratulations on your first microsoft/STL commit! 😺 🚀 🎉 This will ship in VS 2022 17.0 Preview 3.

Stephan T. Lavavej (@StephanTLavavej), Billy O'Neal (@BillyONeal) Thanks a lot for your help and support. 😃