System.IO.Directory.Delete should check for the name surrogate bit on reparse tags before deciding if it should enumerate directories with reparse points

[Petermarcu]

Copied from internal bug 402782

System.IO.Directory.Delete recursive delete will not recurse through directory placeholders as it should (directory placeholders are a feature of GvFlt and the
container filter).

Christian Allred [callred] found that the Delete code "will avoid trying to enumerate directories that are any kind of reparse point, rather than just name surrogates ... It should be fixed to check for the name surrogate bit on the reparse tag, same as rmdir /s does"

See: http://ddindex/?rightProject=mscorlib&file=system%5cio%5cdirectory.cs&line=1324)

rmdir code

//
//  We now enumerate and delete children iff:
//      directory and not reparse/namesurrogate
//
if (IsDirectory( find_data.dwFileAttributes )
    && !(IsReparse( find_data.dwFileAttributes ) && IsReparseTagNameSurrogate( find_data.dwReserved0 ))) {
    ...
    //  (Enumerate, deleting files and recursing into directories)
    ...
}

return RemoveDirectoryForce(pszDirectory);

...

Rmdir /s checks the name surrogate bit using IsReparseTagNameSurrogate, which is in ntifs.h. It is defined as:

#define IsReparseTagNameSurrogate(_tag) (
((_tag) & 0x20000000)
)

If you're doing a FindFirst/FindNext, the _tag value is the dwReserved0 field of the WIN32_FIND_DATA structure. That is how rmdir /s does its check:

//
//  We now enumerate and delete children iff:
//      directory and not reparse/namesurrogate
//
if (IsDirectory( find_data.dwFileAttributes )
    && !(IsReparse( find_data.dwFileAttributes ) && IsReparseTagNameSurrogate( find_data.dwReserved0 ))) {
    ...
    //  (Enumerate, deleting files and recursing into directories)
    ...
}
return RemoveDirectoryForce(pszDirectory);

...

Forgot to mention, the name surrogate bit is set on directory junctions, symlinks, etc. So having this check correctly identifies other types of reparse points that a recursive delete would not want to recurse in to.

[JeremyKuhne]

@danmosemsft Another fungible one that needs to be done.

[danmoseley]

existing code

                            // Reparse point, don't recurse, just remove. (dwReserved0 is documented for this flag)
                            if (findData.dwReserved0 == Interop.Kernel32.IOReparseOptions.IO_REPARSE_TAG_MOUNT_POINT)
                            {
                                // Mount point. Unmount using full path plus a trailing '\'.
                                // (Note: This doesn't remove the underlying directory)
...
                                if (!Interop.Kernel32.DeleteVolumeMountPoint(mountPoint) && exception == null)
                                {
...
                                }
                            }

This seems wrong in the first place because MSDN says we should first check for FILE_ATTRIBUTE_REPARSE_POINT and we're not.

If the dwFileAttributes member includes the FILE_ATTRIBUTE_REPARSE_POINT attribute, this member specifies the reparse point tag.

Otherwise, this value is undefined and should not be used.

It looks like the IsReparseTagNameSurrogate behavior is sufficiently documented for us to use.

So it seems we should add a check for FILE_ATTRIBUTE_REPARSE_POINT. If it is true then check for IsReparseTagNameSurrogate (the 29th bit, so just & 0x20000000). If that is false, treat as a regular directory. If that is true, check for IO_REPARSE_TAG_MOUNT_POINT and continue as per existing code.

Is that correct @JeremyKuhne ? If I understand correctly these directory placeholders are not name surrogates and we are expected to enumerate through them.

[danmoseley]

@maryamariyan another one for you I think.

[JeremyKuhne]

@danmosemsft We do check above the code you mentioned:

https://github.com/dotnet/corefx/blob/master/src/System.IO.FileSystem/src/System/IO/FileSystem.Windows.cs#L452

The answer is, yes, we need to check the flag on any reparse point and just delete (without recursing) if it is a name surrogate. Note that we do another check for reparse point status at the top level entry point for directory deletion.

[danmoseley]

@maryamariyan not sure how you'd test this, perhaps a little pinvoking to make a temporary reparse point of each flavor.

[JeremyKuhne]

I don't think we can create any of the non-surrogate type outside of OneDrive folders, which you'd have to set up and manually test. Hard and symbolic links are the only ones I know how to create programmatically and they are surrogates. You might want to contact Christian Allred (mentioned above) to see if they have any suggestions.

[JeremyKuhne]

@maryamariyan I spoke with @danmosemsft- I'm going to take this back as we want to get the change into the system soon.