New issue
Advanced search Search tips

Issue 602055 link

Starred by 2 users

Issue metadata

Status: Duplicate
Merged: issue 571906
Owner:
Closed: Dec 2016
Components:
EstimatedDays: ----
NextAction: ----
OS: Windows
Pri: 2
Type: Bug



Sign in to add a comment

Chromium's Windows file deletion algorithm is (very subtly) incorrect

Reported by test35...@gmail.com, Apr 9 2016

Issue description

UserAgent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/50.0.2661.66 Safari/537.36

Steps to reproduce the problem:
1. Run system under maximal IO load, extreme torture testing
2. Attempt to delete a directory tree
3. Time to *actually* remove the file may take longer than expected, and will not be done by the time Chromium calls ::RemoveDirectory

What is the expected behavior?
base::DeleteFile, base::DeleteFileRecursive, and base::MoveUnsafe, should always work.

What went wrong?
Chromium's Windows file deletion code will work 99.99% of the time, and deleting directory trees will sometimes fail

See "CppCon 2015: Niall Douglas “Racing The File System"
 (https://youtu.be/uhRWMGBjlO8?t=7m35s) for *specifically* why the code is wrong.

Niall Douglas is the developer of Boost AFIO, and has become a Windows filesystem expert because of it.

From his presentation (linked above, for the correct algorithm skip to ~8:50), it looks like Chrome's file deletion code is incorrect.

...this could be also why  Issue #179576  exists - the deletion operation will "randomly" fail. Douglas suggests (when deleting a directory tree) moving the files one level up, and simultaneously renaming to some cryptographically random name, THEN actually deleting the files, and lastly deleting the directory. I'm going to mention this on that issue.

The documentation for RemoveDirectory (https://msdn.microsoft.com/en-us/library/windows/desktop/aa365488.aspx) is kinda crappy, because it says "RemoveDirectory removes a directory junction, even if the contents of the target are not empty; the function removes directory junctions regardless of the state of the target object.", but "Creating and Deleting Directories" (https://msdn.microsoft.com/en-us/library/windows/desktop/aa363872.aspx) says: "To delete an existing directory, use the RemoveDirectory or RemoveDirectoryTransacted function. Before removing a directory, you must ensure that the directory is empty and that you have the delete access privilege for the directory. To do the latter, call the GetSecurityInfo function."

Did this work before? N/A 

Chrome version: 50.0.2661.66  Channel: beta
OS Version: 10.0
Flash Version: Shockwave Flash 21.0 r0

Some related nits, that moreso bug me than actually are problematic:

- Nobody DCHECKs the return value of ::FindClose FileEnumerator::~FileEnumerator() (file_enumerator_win.cc)
- ::FindFirstFileEx in FileEnumerator::Next (file_enumerator_win.cc) uses FIND_FIRST_EX_LARGE_FETCH, which I've personally found to be slower.
- Nobody compares the return value of ::FindNextFile in FileEnumerator::Next (file_enumerator_win.cc) to ERROR_NO_MORE_FILES, because ::FindNextFile returns FALSE on failure AND success
- Nobody DCHECKs the return value of ::SetFileAttributes in DeleteFileRecursive
- Throughout file_util_win.cc, on failure of ::RemoveDirectory, ::DeleteFile, ::MoveFileEx, ::MoveFile, ::GetTempFileName, ::GetLongPathName, ::GetLogicalDriveStrings, ::QueryDosDevice, ::CreateFile, ::CreateFileMapping, ::MapViewOfFile, ::GetMappedFileName, ::GetFileAttributesEx, ::ReadFile, ::WriteFile, etc... nobody DLOGs the ::GetLastError value, which could've helped diagnose this issue
- Nobody actually checks to see why ::CreateDirectory (in CreateTemporaryDirInDir) fails
- Nobody checks the return value of ::UnmapViewOfFile
 
Labels: Te-NeedsFurtherTriage
Mictosoft

Comment 3 by grt@chromium.org, Dec 2 2016

Components: Internals>PlatformIntegration
Mergedinto: 571906
Owner: grt@chromium.org
Status: Duplicate (was: Unconfirmed)
Duping into issue 571906, which I am fixing.

Sign in to add a comment