Fix filesystem::remove to remove readonly files also

[SibiSiddharthan]

Fixes #1511
Remove Readonly Files also.

[ghost]

All CLA requirements met.

[StephanTLavavej]

Thanks! I noticed that a couple of lines could be simplified, and verified that the tests continue to pass, so I'll push a change.

[BillyONeal]

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.

[BillyONeal]

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

[BillyONeal]

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 (

STL/stl/src/filesystem.cpp

Line 493 in ac4fde7

const _STD _Fs_file _Handle(_Target, __std_access_rights::_Delete, _Flags, &_Last_error);

) by doing something like:

    _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

[StephanTLavavej]

@BillyONeal (who originally worked on <filesystem>) explained this issue to me, and (after I explained that POSIX indeed allows non-writeable files to be removed), I agree that a different approach appears to be necessary here. Although it seems obscure/difficult to have parent directory permissions that would allow a file's readonly bit to be cleared while file deletion still fails, in that scenario we're left with a permanent change to the file's permissions. (This is separate from any question of "filesystem races", which the STL technically does not need to worry about.)

Billy's proposed change to filesystem.cpp instead of <filesystem> sounds like it would work, and would not require an invasive overhaul. I think that manually verifying that his unusual repro is fixed would be sufficient; we don't need to add automated coverage for those weird directory permissions.

Please let us know if you'd like us to investigate the proposed change; I realize that asking for a total rewrite of the product code fix is a lot for a first-time contribution. 😸

[SibiSiddharthan]

@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 @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.
         
        }

[BillyONeal]

The change I propose is [...]

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

[SibiSiddharthan]

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.)

[BillyONeal]

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.

[SibiSiddharthan]

Hi there,
@BillyONeal, @StephanTLavavej any updates regarding this pull request.

[CaseyCarter]

Hi there,
@BillyONeal, @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 ;)

[SibiSiddharthan]

Thanks for the update 😃

[StephanTLavavej]

Thanks! My apologies for how long it took to review this. I think the logic looks good; all that I found were fairly superficial issues and one concern about GetLastError state.

[SibiSiddharthan]

@StephanTLavavej I have made the suggested changes. Please take a look.

[StephanTLavavej]

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.

[StephanTLavavej]

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

@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.

[StephanTLavavej]

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.

[SibiSiddharthan]

@StephanTLavavej, @BillyONeal Thanks a lot for your help and support. 😃