Repository: mesos Updated Branches: refs/heads/master 4b1a8a24c -> ffc45b346
Windows: Changed sharing semantics in `get_handle_follow`. In the internal Windows functions `get_handle(_no)_follow`, we erroneously only opened with `FILE_SHARE_READ` permissions. While we are only opening the files for reading (indicated by the `GENERIC_READ` flag), we need to open with write and delete sharing semantics too, in order to avoid obtaining a lock on the file. A regression test using `os::realpath` on a file that was already being written to was added to ensure the correct behavior. Review: https://reviews.apache.org/r/65936/ Project: http://git-wip-us.apache.org/repos/asf/mesos/repo Commit: http://git-wip-us.apache.org/repos/asf/mesos/commit/ffc45b34 Tree: http://git-wip-us.apache.org/repos/asf/mesos/tree/ffc45b34 Diff: http://git-wip-us.apache.org/repos/asf/mesos/diff/ffc45b34 Branch: refs/heads/master Commit: ffc45b3467a8c6ebc774d4825f48030006863306 Parents: 4b1a8a2 Author: John Kordich <[email protected]> Authored: Fri Mar 9 15:10:25 2018 -0800 Committer: Andrew Schwartzmeyer <[email protected]> Committed: Fri Mar 9 18:10:43 2018 -0800 ---------------------------------------------------------------------- .../stout/internal/windows/reparsepoint.hpp | 18 ++++++---- 3rdparty/stout/tests/os/filesystem_tests.cpp | 37 ++++++++++++++++++++ 2 files changed, 49 insertions(+), 6 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/mesos/blob/ffc45b34/3rdparty/stout/include/stout/internal/windows/reparsepoint.hpp ---------------------------------------------------------------------- diff --git a/3rdparty/stout/include/stout/internal/windows/reparsepoint.hpp b/3rdparty/stout/include/stout/internal/windows/reparsepoint.hpp index 858b3c7..280956f 100644 --- a/3rdparty/stout/include/stout/internal/windows/reparsepoint.hpp +++ b/3rdparty/stout/include/stout/internal/windows/reparsepoint.hpp @@ -179,8 +179,9 @@ inline Try<SharedHandle> get_handle_follow(const std::string& absolute_path) // can be found in MSDN[2]. // // The `GENERIC_READ` flag is being used because it's the most common way of - // opening a file for reading only. The `FILE_SHARE_READ` allows other - // processes to read the file at the same time. MSDN[1] provides a more + // opening a file for reading only. The `SHARE` flags allow other processes + // to read the file at the same time, as well as allow this process to read + // files that were also opened with these flags. MSDN[1] provides a more // detailed explanation of these flags. // // [1] https://msdn.microsoft.com/en-us/library/windows/desktop/aa363858(v=vs.85).aspx // NOLINT(whitespace/line_length) @@ -192,7 +193,9 @@ inline Try<SharedHandle> get_handle_follow(const std::string& absolute_path) const HANDLE handle = ::CreateFileW( longpath(absolute_path).data(), GENERIC_READ, // Open the file for reading only. - FILE_SHARE_READ, // Just reading this file, allow others to do the same. + // Must pass in all SHARE flags below, in case file is already open. + // Otherwise, we may get an access denied error. + FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE, nullptr, // Ignored. OPEN_EXISTING, // Open existing file. access_flags, // Open file, not the symlink itself. @@ -234,8 +237,9 @@ inline Try<SharedHandle> get_handle_no_follow(const std::string& absolute_path) // can be found in MSDN[2]. // // The `GENERIC_READ` flag is being used because it's the most common way of - // opening a file for reading only. The `FILE_SHARE_READ` allows other - // processes to read the file at the same time. MSDN[1] provides a more + // opening a file for reading only. The `SHARE` flags allow other processes + // to read the file at the same time, as well as allow this process to read + // files that were also opened with these flags. MSDN[1] provides a more // detailed explanation of these flags. // // [1] https://msdn.microsoft.com/en-us/library/windows/desktop/aa363858(v=vs.85).aspx // NOLINT(whitespace/line_length) @@ -247,7 +251,9 @@ inline Try<SharedHandle> get_handle_no_follow(const std::string& absolute_path) const HANDLE handle = ::CreateFileW( longpath(absolute_path).data(), GENERIC_READ, // Open the file for reading only. - FILE_SHARE_READ, // Just reading this file, allow others to do the same. + // Must pass in all SHARE flags below, in case file is already open. + // Otherwise, we may get an access denied error. + FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE, nullptr, // Ignored. OPEN_EXISTING, // Open existing symlink. access_flags, // Open symlink, not the file it points to. http://git-wip-us.apache.org/repos/asf/mesos/blob/ffc45b34/3rdparty/stout/tests/os/filesystem_tests.cpp ---------------------------------------------------------------------- diff --git a/3rdparty/stout/tests/os/filesystem_tests.cpp b/3rdparty/stout/tests/os/filesystem_tests.cpp index b84f1ae..c190baa 100644 --- a/3rdparty/stout/tests/os/filesystem_tests.cpp +++ b/3rdparty/stout/tests/os/filesystem_tests.cpp @@ -259,6 +259,43 @@ TEST_F(FsTest, CreateDirectoryLongerThanMaxPath) EXPECT_SOME_TRUE(os::access(testfile, R_OK | W_OK)); EXPECT_SOME_EQ(testfile, os::realpath(testfile)); } + + +// This test ensures that `os::realpath` will work on open files. +TEST_F(FsTest, RealpathValidationOnOpenFile) +{ + // Open a file to write, with "SHARE" read/write permissions, + // then call `os::realpath` on that file. + const string file = path::join(sandbox.get(), id::UUID::random().toString()); + + const string data = "data"; + + const SharedHandle handle( + ::CreateFileW( + wide_stringify(file).data(), + FILE_APPEND_DATA, + FILE_SHARE_READ | FILE_SHARE_WRITE, + nullptr, // No inheritance. + CREATE_ALWAYS, + FILE_ATTRIBUTE_NORMAL, + nullptr), // No template file. + ::CloseHandle); + EXPECT_NE(handle.get_handle(), INVALID_HANDLE_VALUE); + + DWORD bytes_written; + BOOL written = ::WriteFile( + handle.get_handle(), + data.c_str(), + static_cast<DWORD>(data.size()), + &bytes_written, + nullptr); // No overlapped I/O. + EXPECT_EQ(written, TRUE); + EXPECT_EQ(data.size(), bytes_written); + + // Verify that `os::realpath` (which calls `CreateFileW` on Windows) is + // successful even though the file is open elsewhere. + EXPECT_SOME_EQ(file, os::realpath(file)); +} #endif // __WINDOWS__
