From b9cc835bbdedd4898080ef955d51a269d08a1708 Mon Sep 17 00:00:00 2001 From: erg Date: Fri, 29 May 2015 12:22:34 -0700 Subject: [PATCH] mandoline filesystem: Pass raw file descriptors over mojo message pipes. This adds an AsHandle() method to filesystem::File so we can do things like pass file descriptors to sqlite. BUG=490237 Review URL: https://codereview.chromium.org/1152913003 Cr-Commit-Position: refs/heads/master@{#332024} --- components/filesystem/BUILD.gn | 2 + components/filesystem/DEPS | 1 + components/filesystem/directory_impl.cc | 14 +++++ components/filesystem/directory_impl_unittest.cc | 29 +++++++++++ components/filesystem/file_impl.cc | 40 +++++++++++++++ components/filesystem/file_impl.h | 1 + components/filesystem/file_impl_unittest.cc | 59 ++++++++++++++++++++++ components/filesystem/public/interfaces/file.mojom | 5 +- 8 files changed, 149 insertions(+), 2 deletions(-) diff --git a/components/filesystem/BUILD.gn b/components/filesystem/BUILD.gn index f022412503e1..f0580257f643 100644 --- a/components/filesystem/BUILD.gn +++ b/components/filesystem/BUILD.gn @@ -23,6 +23,7 @@ mojo_native_application("filesystem") { "//mojo/application/public/cpp", "//mojo/common", "//mojo/environment:chromium", + "//mojo/platform_handle", "//third_party/mojo/src/mojo/public/cpp/bindings", "//third_party/mojo/src/mojo/public/cpp/system", ] @@ -44,6 +45,7 @@ mojo_native_application("apptests") { "//base", "//components/filesystem/public/interfaces", "//mojo/application/public/cpp:test_support", + "//mojo/platform_handle", "//third_party/mojo/src/mojo/public/cpp/bindings", ] } diff --git a/components/filesystem/DEPS b/components/filesystem/DEPS index 12fb4fe3dbdb..40d80758f57b 100644 --- a/components/filesystem/DEPS +++ b/components/filesystem/DEPS @@ -1,4 +1,5 @@ include_rules = [ "+mojo/application", + "+mojo/platform_handle", "+mojo/public", ] diff --git a/components/filesystem/directory_impl.cc b/components/filesystem/directory_impl.cc index 7d3ae04d10ac..487f75550380 100644 --- a/components/filesystem/directory_impl.cc +++ b/components/filesystem/directory_impl.cc @@ -66,6 +66,20 @@ void DirectoryImpl::OpenFile(const mojo::String& raw_path, return; } + base::File::Info info; + if (!base_file.GetInfo(&info)) { + callback.Run(ERROR_FAILED); + return; + } + + if (info.is_directory) { + // We must not return directories as files. In the file abstraction, we can + // fetch raw file descriptors over mojo pipes, and passing a file + // descriptor to a directory is a sandbox escape on Windows. + callback.Run(ERROR_NOT_A_FILE); + return; + } + if (file.is_pending()) { new FileImpl(file.Pass(), base_file.Pass()); } diff --git a/components/filesystem/directory_impl_unittest.cc b/components/filesystem/directory_impl_unittest.cc index eb6f81c1140e..34cda903d400 100644 --- a/components/filesystem/directory_impl_unittest.cc +++ b/components/filesystem/directory_impl_unittest.cc @@ -120,6 +120,35 @@ TEST_F(DirectoryImplTest, BasicRenameDelete) { EXPECT_EQ(ERROR_FAILED, error); } +TEST_F(DirectoryImplTest, CantOpenDirectoriesAsFiles) { + DirectoryPtr directory; + GetTemporaryRoot(&directory); + Error error; + + { + // Create a directory called 'my_file' + DirectoryPtr my_file_directory; + error = ERROR_FAILED; + directory->OpenDirectory( + "my_file", GetProxy(&my_file_directory), + kFlagRead | kFlagWrite | kFlagCreate, + Capture(&error)); + ASSERT_TRUE(directory.WaitForIncomingResponse()); + EXPECT_EQ(ERROR_OK, error); + } + + { + // Attempt to open that directory as a file. This must fail! + FilePtr file; + error = ERROR_FAILED; + directory->OpenFile("my_file", GetProxy(&file), kFlagRead | kFlagOpen, + Capture(&error)); + ASSERT_TRUE(directory.WaitForIncomingResponse()); + EXPECT_EQ(ERROR_NOT_A_FILE, error); + } +} + + // TODO(vtl): Test delete flags. } // namespace diff --git a/components/filesystem/file_impl.cc b/components/filesystem/file_impl.cc index f76b0e81257d..c9d2637179b5 100644 --- a/components/filesystem/file_impl.cc +++ b/components/filesystem/file_impl.cc @@ -10,11 +10,13 @@ #include "base/files/scoped_file.h" #include "base/logging.h" #include "components/filesystem/util.h" +#include "mojo/platform_handle/platform_handle_functions.h" static_assert(sizeof(off_t) <= sizeof(int64_t), "off_t too big"); static_assert(sizeof(size_t) >= sizeof(uint32_t), "size_t too small"); using base::Time; +using mojo::ScopedHandle; namespace filesystem { @@ -252,4 +254,42 @@ void FileImpl::Dup(mojo::InterfaceRequest file, callback.Run(ERROR_OK); } +void FileImpl::AsHandle(const AsHandleCallback& callback) { + if (!file_.IsValid()) { + callback.Run(GetError(file_), ScopedHandle()); + return; + } + + base::File new_file = file_.Duplicate(); + if (!new_file.IsValid()) { + callback.Run(GetError(new_file), ScopedHandle()); + return; + } + + base::File::Info info; + if (!new_file.GetInfo(&info)) { + callback.Run(ERROR_FAILED, ScopedHandle()); + return; + } + + // Perform one additional check right before we send the file's file + // descriptor over mojo. This is theoretically redundant, but given that + // passing a file descriptor to a directory is a sandbox escape on Windows, + // we should be absolutely paranoid. + if (info.is_directory) { + callback.Run(ERROR_NOT_A_FILE, ScopedHandle()); + return; + } + + MojoHandle mojo_handle; + MojoResult create_result = MojoCreatePlatformHandleWrapper( + new_file.TakePlatformFile(), &mojo_handle); + if (create_result != MOJO_RESULT_OK) { + callback.Run(ERROR_FAILED, ScopedHandle()); + return; + } + + callback.Run(ERROR_OK, ScopedHandle(mojo::Handle(mojo_handle)).Pass()); +} + } // namespace filesystem diff --git a/components/filesystem/file_impl.h b/components/filesystem/file_impl.h index 58d5d3dfe2f2..469e89d8db55 100644 --- a/components/filesystem/file_impl.h +++ b/components/filesystem/file_impl.h @@ -47,6 +47,7 @@ class FileImpl : public File { const TouchCallback& callback) override; void Dup(mojo::InterfaceRequest file, const DupCallback& callback) override; + void AsHandle(const AsHandleCallback& callback) override; private: mojo::StrongBinding binding_; diff --git a/components/filesystem/file_impl_unittest.cc b/components/filesystem/file_impl_unittest.cc index d20cecc4075e..9041bee55fd5 100644 --- a/components/filesystem/file_impl_unittest.cc +++ b/components/filesystem/file_impl_unittest.cc @@ -4,7 +4,9 @@ #include +#include "base/files/file.h" #include "components/filesystem/files_test_base.h" +#include "mojo/platform_handle/platform_handle_functions.h" #include "mojo/public/cpp/bindings/interface_request.h" #include "mojo/public/cpp/bindings/type_converter.h" @@ -623,5 +625,62 @@ TEST_F(FileImplTest, Truncate) { EXPECT_EQ(kTruncatedSize, file_info->size); } +TEST_F(FileImplTest, AsHandle) { + DirectoryPtr directory; + GetTemporaryRoot(&directory); + Error error; + + { + // Create my_file. + FilePtr file1; + error = ERROR_FAILED; + directory->OpenFile("my_file", GetProxy(&file1), + kFlagRead | kFlagWrite | kFlagCreate, Capture(&error)); + ASSERT_TRUE(directory.WaitForIncomingResponse()); + EXPECT_EQ(ERROR_OK, error); + + // Fetch the handle + error = ERROR_FAILED; + mojo::ScopedHandle handle; + file1->AsHandle(Capture(&error, &handle)); + ASSERT_TRUE(file1.WaitForIncomingResponse()); + EXPECT_EQ(ERROR_OK, error); + + // Pull a file descriptor out of the scoped handle. + MojoPlatformHandle platform_handle; + MojoResult extract_result = + MojoExtractPlatformHandle(handle.release().value(), &platform_handle); + EXPECT_EQ(MOJO_RESULT_OK, extract_result); + + // Pass this raw file descriptor to a base::File. + base::File raw_file(platform_handle); + ASSERT_TRUE(raw_file.IsValid()); + EXPECT_EQ(5, raw_file.WriteAtCurrentPos("hello", 5)); + } + + { + // Reopen my_file. + FilePtr file2; + error = ERROR_FAILED; + directory->OpenFile("my_file", GetProxy(&file2), kFlagRead | kFlagOpen, + Capture(&error)); + ASSERT_TRUE(directory.WaitForIncomingResponse()); + EXPECT_EQ(ERROR_OK, error); + + // Verify that we wrote data raw on the file descriptor. + mojo::Array bytes_read; + error = ERROR_FAILED; + file2->Read(5, 0, WHENCE_FROM_BEGIN, Capture(&error, &bytes_read)); + ASSERT_TRUE(file2.WaitForIncomingResponse()); + EXPECT_EQ(ERROR_OK, error); + ASSERT_EQ(5u, bytes_read.size()); + EXPECT_EQ(static_cast('h'), bytes_read[0]); + EXPECT_EQ(static_cast('e'), bytes_read[1]); + EXPECT_EQ(static_cast('l'), bytes_read[2]); + EXPECT_EQ(static_cast('l'), bytes_read[3]); + EXPECT_EQ(static_cast('o'), bytes_read[4]); + } +} + } // namespace } // namespace filesystem diff --git a/components/filesystem/public/interfaces/file.mojom b/components/filesystem/public/interfaces/file.mojom index a61697adf555..8148ec414a48 100644 --- a/components/filesystem/public/interfaces/file.mojom +++ b/components/filesystem/public/interfaces/file.mojom @@ -33,8 +33,6 @@ interface File { Write(array bytes_to_write, int64 offset, Whence whence) => (Error error, uint32 num_bytes_written); - // TODO(erg): Add stream variants of Read()/Write(). - // Gets the current file position. On success, |position| is the current // offset (in bytes) from the beginning of the file). Tell() => (Error error, int64 position); @@ -60,4 +58,7 @@ interface File { // I.e., the access mode, etc. (as specified to |Directory::OpenFile()| by the // |open_flags| argument) as well as file position. Dup(File& file) => (Error error); + + // Returns a handle to the file for raw access. + AsHandle() => (Error error, handle file_handle); }; -- 2.11.4.GIT