Issue metadata
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 descriptionUserAgent: 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
,
Apr 14 2016
Mictosoft
,
Dec 2 2016
Duping into issue 571906, which I am fixing. |
|||||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||||
Comment 1 by ashej...@chromium.org
, Apr 11 2016