FileSystem.AccessControl tests not cleaning files properly

[carlossanlop]

Fixes #44766

The TempDirectory helper class is disposable. The finalizer calls a function that deletes the folders left behind, without throwing exceptions on fail.

This protective try catch hid some additional errors that were not caught when these unit tests were first written, so I'm fixing them:

• I created a class that inherits from TempDirectory to override the folder deleting method
• In the finalizer, I iterate through all the files and folders created by the test, ensure their ACLs give me full control (in case the tests prevent deletion), then attempt to delete the tree.
• To verify my changes, I did not put the finalizer code in a try catch, so I could see the errors thrown when attempting to delete the tree.
• The deletion exceptions helped me find that GetAccessControl needs to be called with the AccessControlSections.Access argument, otherwise they throw "PrivilegeNotHeldException: The process does not possess the 'SeSecurityPrivilege' privilege which is required for this operation."
• I avoided creating files and directories using a FileSecurity or DirectorySecurity that did not have its access rules defined. Files and folders created this way could not be deleted.
• I avoided testing AccessControlType.Deny. This would also cause deletion exceptions.
• Moved some repeated code into common methods.
• Before commiting, I made sure to put the file/folder deletion code inside a try catch to prevent finalizer exceptions.

[carlossanlop]

Thank you, @danmosemsft for your suggestion to look inside the TempDirectory dispose/finalizer code.

[carlossanlop]

The last commit has the try catch commented in the DeleteDirectory method: ed55700

When the CI finishes successfully, I'll uncomment the try catch and merge.

[carlossanlop]

All checks passed for commit ed55700 with the try catch commented.
I'm submitted another commit bringing back the try catch so that I can merge.

[jozkee]

Can you please run the code coverage report and verify that it is not being regressed by this change? You can do so by running dotnet build /t:test /p:coverage=true.

[carlossanlop]

Can you please run the code coverage report and verify that it is not being regressed by this change?

@jozkee @adamsitnik

I compared before and after my changes (making sure I had the same commit as the baseline for both runs).

There is a ~1% reduction in code coverage in the System.IO.FileSystem.AccessControl unit tests. I needed to remove some FileSystemRights combinations (and make sure they all had a Read) to ensure the files would still be deletable.

@danmosemsft is a ~1% code coverage reduction tolerated? Would we consider it a blocker for merging?

Before:

+------------------------------------+--------+--------+--------+
| Module                             | Line   | Branch | Method |
+------------------------------------+--------+--------+--------+
| System.IO.FileSystem.AccessControl | 76.64% | 60.4%  | 77.69% |
+------------------------------------+--------+--------+--------+

+---------+--------+--------+--------+
|         | Line   | Branch | Method |
+---------+--------+--------+--------+
| Total   | 76.64% | 60.4%  | 77.69% |
+---------+--------+--------+--------+
| Average | 76.64% | 60.4%  | 77.69% |
+---------+--------+--------+--------+

After:

+------------------------------------+-------+--------+--------+
| Module                             | Line  | Branch | Method |
+------------------------------------+-------+--------+--------+
| System.IO.FileSystem.AccessControl | 75.6% | 59.04% | 76.97% |
+------------------------------------+-------+--------+--------+

+---------+--------+--------+--------+
|         | Line   | Branch | Method |
+---------+--------+--------+--------+
| Total   | 75.6%  | 59.04% | 76.97% |
+---------+--------+--------+--------+
| Average | 75.59% | 59.04% | 76.97% |
+---------+--------+--------+--------+

AFTER_coverage.opencover.xml.txt
BEFORE_coverage.opencover.xml.txt

[danmoseley]

@danmosemsft is a ~1% code coverage reduction tolerated? Would we consider it a blocker for merging?

We trust your judgement as the engineering owner of this area. Whether or not we should, we've not (except maybe in JSON) applied a hard rule - or even been consistent about measuring it. My inclination would be: the most important thing is whether something significant has been left uncovered, than reaching a particular number.

[carlossanlop]

The unit test failures are unrelated to my change:

• System.Net.Tests.HttpWebRequestTest_Sync.ReadWriteTimeout_CancelsResponse

  • Libraries Test Run release mono Linux x64 Debug
  • Libraries Test Run release coreclr Linux x64 Debug

• ThreadPoolBoundHandleTests.MultipleOperationsOverSingleHandle_CompletedWorkItemCountTest

  • Libraries Test Run release coreclr windows x86 Debug

[carlossanlop]

@jozkee @adamsitnik I submitted a new commit where I address your suggestions:

• I brought back the unit tests that consumed the parameterless FileSecurity and DirectorySecurity constructors. The files and dirs were being created in a way that DeleteDirectory was unable to enumerate so that I could reset their permissions before deleting them.
• I made sure to avoid enumerating files and folders in DeleteDirectory because that action expects access permission. Instead, any files or folders that a unit test creates, are passed to lists in the TempAclDirectory class. If non empty, they get traversed so the files and dirs they contain get their permissions restored (no need to enumerate if I already know what to modify).
• I brought back all the FileSystemRights in the inline data of some tests. I added comments describing why some of them cannot be tested.

I ran all these tests with the try and catch in DeleteDirectory commented, to make sure nothing was throwing in the finalizer.

[jozkee]

Seems like these two tests are still missing.

[carlossanlop]

I improved this test and renamed it to DirectoryInfo_Create_MultipleAddAccessRules.

[jozkee]

What about DirectorySecurity_CreateDirectory_DefaultDirectorySecurity?

[jozkee]

Seems like DirectorySecurity_CreateDirectory_DefaultDirectorySecurity is testing the same as DirectoryInfo_Create_DefaultDirectorySecurity. How odd.

[carlossanlop]

Code coverage with the latest commits:

 +------------------------------------+-------+--------+--------+
  | Module                             | Line  | Branch | Method |
  +------------------------------------+-------+--------+--------+
  | System.IO.FileSystem.AccessControl | 76.8% | 60.58% | 77.69% |
  +------------------------------------+-------+--------+--------+

  +---------+-------+--------+--------+
  |         | Line  | Branch | Method |
  +---------+-------+--------+--------+
  | Total   | 76.8% | 60.58% | 77.69% |
  +---------+-------+--------+--------+
  | Average | 76.8% | 60.58% | 77.69% |
  +---------+-------+--------+--------+

[adamsitnik]

LGTM, thank you @carlossanlop

also big thanks to @jozkee for a great review!

[carlossanlop]

I'm investigating this failure in mono x64 windows release:

System.IO.FileSystemAclExtensionsTests.FileInfo_Create_DefaultFileSecurity [FAIL]
      System.ArgumentException : Handle does not support asynchronous operations. The parameters to the FileStream constructor may need to be changed to indicate that the handle was opened synchronously (that is, it was not opened for overlapped I/O). (Parameter 'handle')
      ---- System.PlatformNotSupportedException : This API is specific to the way in which Windows handles asynchronous I/O, and is not supported on this platform.
      Stack Trace:
        /_/src/libraries/System.Private.CoreLib/src/System/IO/FileStream.Windows.cs(170,0): at System.IO.FileStream.InitFromHandleImpl(SafeFileHandle handle, Boolean useAsyncIO)
        /_/src/libraries/System.Private.CoreLib/src/System/IO/FileStream.Windows.cs(129,0): at System.IO.FileStream.InitFromHandle(SafeFileHandle handle, FileAccess access, Boolean useAsyncIO)
        /_/src/libraries/System.Private.CoreLib/src/System/IO/FileStream.cs(148,0): at System.IO.FileStream.ValidateAndInitFromHandle(SafeFileHandle handle, FileAccess access, Int32 bufferSize, Boolean isAsync)
        /_/src/libraries/System.Private.CoreLib/src/System/IO/FileStream.cs(153,0): at System.IO.FileStream..ctor(SafeFileHandle handle, FileAccess access, Int32 bufferSize, Boolean isAsync)
        /_/src/libraries/System.IO.FileSystem.AccessControl/src/System/IO/FileSystemAclExtensions.cs(220,0): at System.IO.FileSystemAclExtensions.Create(FileInfo fileInfo, FileMode mode, FileSystemRights rights, FileShare share, Int32 bufferSize, FileOptions options, FileSecurity fileSecurity)
        /_/src/libraries/System.IO.FileSystem.AccessControl/tests/FileSystemAclExtensionsTests.cs(564,0): at System.IO.FileSystemAclExtensionsTests.Verify_FileSecurity_CreateFile(FileMode mode, FileSystemRights rights, FileShare share, Int32 bufferSize, FileOptions options, FileSecurity expectedSecurity)
        /_/src/libraries/System.IO.FileSystem.AccessControl/tests/FileSystemAclExtensionsTests.cs(555,0): at System.IO.FileSystemAclExtensionsTests.Verify_FileSecurity_CreateFile(FileSecurity expectedSecurity)
        /_/src/libraries/System.IO.FileSystem.AccessControl/tests/FileSystemAclExtensionsTests.cs(372,0): at System.IO.FileSystemAclExtensionsTests.FileInfo_Create_DefaultFileSecurity()
        /_/src/mono/netcore/System.Private.CoreLib/src/System/Reflection/RuntimeMethodInfo.cs(378,0): at System.Reflection.RuntimeMethodInfo.Invoke(Object obj, BindingFlags invokeAttr, Binder binder, Object[] parameters, CultureInfo culture)
        ----- Inner Stack Trace -----
        /_/src/libraries/System.Private.CoreLib/src/System/Threading/ThreadPoolBoundHandle.PlatformNotSupported.cs(24,0): at System.Threading.ThreadPoolBoundHandle.BindHandle(SafeHandle handle)
        /_/src/libraries/System.Private.CoreLib/src/System/IO/FileStream.Windows.cs(164,0): at System.IO.FileStream.InitFromHandleImpl(SafeFileHandle handle, Boolean useAsyncIO)

[carlossanlop]

Not sure why all the Linux CI legs failed to install the SDK during the build step. I'm restarting them.
This PR should only affect Windows, though.