From 8e7294f9953a98cfaae7e3bf5a6c852018cbcb83 Mon Sep 17 00:00:00 2001 From: Rafael Espindola Date: Fri, 28 Jun 2013 03:48:47 +0000 Subject: [PATCH] Improvements to unique_file and createUniqueDirectory. * Don't try to create parent directories in unique_file. It had two problem: * It violates the contract that it is atomic. If the directory creation success and the file creation fails, we would return an error but the file system was modified. * When creating a temporary file clang would have to first check if the parent directory existed or not to avoid creating one when it was not supposed to. * More efficient implementations of createUniqueDirectory and the unique_file that produces only the file name. Now all 3 just call into a static function passing what they want (name, file or directory). Clang also has to be updated, so tests might fail if a bot picks up this commit and not the corresponding clang one. git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@185126 91177308-0d34-0410-b5e6-96231b3b80d8 --- lib/Support/Path.cpp | 55 ++++--- lib/Support/Unix/Path.inc | 156 +++++++++----------- lib/Support/Windows/Path.inc | 269 +++++++++++++++++------------------ 3 files changed, 237 insertions(+), 243 deletions(-) diff --git a/lib/Support/Path.cpp b/lib/Support/Path.cpp index 1f4f44acb08..fa03df5a24a 100644 --- a/lib/Support/Path.cpp +++ b/lib/Support/Path.cpp @@ -153,6 +153,18 @@ namespace { } } // end unnamed namespace +enum FSEntity { + FS_Dir, + FS_File, + FS_Name +}; + +// Implemented in Unix/Path.inc and Windows/Path.inc. +static llvm::error_code +createUniqueEntity(const llvm::Twine &Model, int &ResultFD, + llvm::SmallVectorImpl &ResultPath, + bool MakeAbsolute, unsigned Mode, FSEntity Type); + namespace llvm { namespace sys { namespace path { @@ -625,32 +637,31 @@ bool is_relative(const Twine &path) { namespace fs { -error_code unique_file(const Twine &Model, SmallVectorImpl &ResultPath, - bool MakeAbsolute) { - // FIXME: This is really inefficient. unique_path creates a path an tries to - // open it. We should factor the code so that we just don't create/open the - // file when we don't need it. - int FD; - error_code Ret = unique_file(Model, FD, ResultPath, MakeAbsolute, all_read); - if (Ret) - return Ret; - - if (close(FD)) - return error_code(errno, system_category()); - - StringRef P(ResultPath.begin(), ResultPath.size()); - return fs::remove(P); +// This is a mkostemps with a different pattern. Unfortunatelly OS X (ond *BSD) +// don't have it. It might be worth experimenting with mkostemps on systems +// that have it. +error_code unique_file(const Twine &Model, int &ResultFD, + SmallVectorImpl &ResultPath, bool MakeAbsolute, + unsigned Mode) { + return createUniqueEntity(Model, ResultFD, ResultPath, MakeAbsolute, Mode, + FS_File); } +// This is a mktemp with a differet pattern. We use createUniqueEntity mostly +// for consistency. It might be worth it experimenting with mktemp. +error_code unique_file(const Twine &Model, SmallVectorImpl &ResultPath, + bool MakeAbsolute) { + int Dummy; + return createUniqueEntity(Model, Dummy, ResultPath, MakeAbsolute, 0, FS_Name); +} + +// This is a mkdtemp with a different pattern. We use createUniqueEntity mostly +// for consistency. It might be worth it experimenting with mkdtemp. error_code createUniqueDirectory(const Twine &Prefix, SmallVectorImpl &ResultPath) { - // FIXME: This is double inefficient. We compute a unique file name, created - // it, delete it and keep only the directory. - error_code EC = unique_file(Prefix + "-%%%%%%/dummy", ResultPath); - if (EC) - return EC; - path::remove_filename(ResultPath); - return error_code::success(); + int Dummy; + return createUniqueEntity(Prefix + "-%%%%%%", Dummy, ResultPath, + true, 0, FS_Dir); } error_code make_absolute(SmallVectorImpl &path) { diff --git a/lib/Support/Unix/Path.inc b/lib/Support/Unix/Path.inc index 94f872077a5..8a94b935993 100644 --- a/lib/Support/Unix/Path.inc +++ b/lib/Support/Unix/Path.inc @@ -110,6 +110,76 @@ namespace { } } +static error_code createUniqueEntity(const Twine &Model, int &ResultFD, + SmallVectorImpl &ResultPath, + bool MakeAbsolute, unsigned Mode, + FSEntity Type) { + SmallString<128> ModelStorage; + Model.toVector(ModelStorage); + + if (MakeAbsolute) { + // Make model absolute by prepending a temp directory if it's not already. + bool absolute = sys::path::is_absolute(Twine(ModelStorage)); + if (!absolute) { + SmallString<128> TDir; + if (error_code ec = TempDir(TDir)) return ec; + sys::path::append(TDir, Twine(ModelStorage)); + ModelStorage.swap(TDir); + } + } + + // From here on, DO NOT modify model. It may be needed if the randomly chosen + // path already exists. + ResultPath = ModelStorage; + // Null terminate. + ResultPath.push_back(0); + ResultPath.pop_back(); + +retry_random_path: + // Replace '%' with random chars. + for (unsigned i = 0, e = ModelStorage.size(); i != e; ++i) { + if (ModelStorage[i] == '%') + ResultPath[i] = "0123456789abcdef"[sys::Process::GetRandomNumber() & 15]; + } + + // Try to open + create the file. + switch (Type) { + case FS_File: { + int RandomFD = ::open(ResultPath.begin(), O_RDWR | O_CREAT | O_EXCL, Mode); + if (RandomFD == -1) { + int SavedErrno = errno; + // If the file existed, try again, otherwise, error. + if (SavedErrno == errc::file_exists) + goto retry_random_path; + return error_code(SavedErrno, system_category()); + } + + ResultFD = RandomFD; + return error_code::success(); + } + + case FS_Name: { + bool Exists; + error_code EC = sys::fs::exists(ResultPath.begin(), Exists); + if (EC) + return EC; + if (Exists) + goto retry_random_path; + return error_code::success(); + } + + case FS_Dir: { + bool Existed; + error_code EC = sys::fs::create_directory(ResultPath.begin(), Existed); + if (EC) + return EC; + if (Existed) + goto retry_random_path; + return error_code::success(); + } + } +} + namespace llvm { namespace sys { namespace fs { @@ -564,92 +634,6 @@ error_code setLastModificationAndAccessTime(int FD, TimeValue Time) { return error_code::success(); } -error_code unique_file(const Twine &model, int &result_fd, - SmallVectorImpl &result_path, - bool makeAbsolute, unsigned mode) { - SmallString<128> Model; - model.toVector(Model); - // Null terminate. - Model.c_str(); - - if (makeAbsolute) { - // Make model absolute by prepending a temp directory if it's not already. - bool absolute = path::is_absolute(Twine(Model)); - if (!absolute) { - SmallString<128> TDir; - if (error_code ec = TempDir(TDir)) return ec; - path::append(TDir, Twine(Model)); - Model.swap(TDir); - } - } - - // From here on, DO NOT modify model. It may be needed if the randomly chosen - // path already exists. - SmallString<128> RandomPath = Model; - -retry_random_path: - // Replace '%' with random chars. - for (unsigned i = 0, e = Model.size(); i != e; ++i) { - if (Model[i] == '%') - RandomPath[i] = "0123456789abcdef"[sys::Process::GetRandomNumber() & 15]; - } - - // Make sure we don't fall into an infinite loop by constantly trying - // to create the parent path. - bool TriedToCreateParent = false; - - // Try to open + create the file. -rety_open_create: - int RandomFD = ::open(RandomPath.c_str(), O_RDWR | O_CREAT | O_EXCL, mode); - if (RandomFD == -1) { - int SavedErrno = errno; - // If the file existed, try again, otherwise, error. - if (SavedErrno == errc::file_exists) - goto retry_random_path; - // If path prefix doesn't exist, try to create it. - if (SavedErrno == errc::no_such_file_or_directory && !TriedToCreateParent) { - TriedToCreateParent = true; - StringRef p(RandomPath); - SmallString<64> dir_to_create; - for (path::const_iterator i = path::begin(p), - e = --path::end(p); i != e; ++i) { - path::append(dir_to_create, *i); - bool Exists; - if (error_code ec = exists(Twine(dir_to_create), Exists)) return ec; - if (!Exists) { - // Don't try to create network paths. - if (i->size() > 2 && (*i)[0] == '/' && - (*i)[1] == '/' && - (*i)[2] != '/') - return make_error_code(errc::no_such_file_or_directory); - if (::mkdir(dir_to_create.c_str(), 0700) == -1 && - errno != errc::file_exists) - return error_code(errno, system_category()); - } - } - goto rety_open_create; - } - - return error_code(SavedErrno, system_category()); - } - - // Make the path absolute. - char real_path_buff[PATH_MAX + 1]; - if (realpath(RandomPath.c_str(), real_path_buff) == NULL) { - int error = errno; - ::close(RandomFD); - ::unlink(RandomPath.c_str()); - return error_code(error, system_category()); - } - - result_path.clear(); - StringRef d(real_path_buff); - result_path.append(d.begin(), d.end()); - - result_fd = RandomFD; - return error_code::success(); -} - error_code mapped_file_region::init(int FD, bool CloseFD, uint64_t Offset) { AutoFD ScopedFD(FD); if (!CloseFD) diff --git a/lib/Support/Windows/Path.inc b/lib/Support/Windows/Path.inc index 2b24c5487f0..ab59fe88344 100644 --- a/lib/Support/Windows/Path.inc +++ b/lib/Support/Windows/Path.inc @@ -124,6 +124,140 @@ namespace { } } +// FIXME: mode should be used here and default to user r/w only, +// it currently comes in as a UNIX mode. +static error_code createUniqueEntity(const Twine &model, int &result_fd, + SmallVectorImpl &result_path, + bool makeAbsolute, unsigned mode, + FSEntity Type) { + // Use result_path as temp storage. + result_path.set_size(0); + StringRef m = model.toStringRef(result_path); + + SmallVector model_utf16; + if (error_code ec = UTF8ToUTF16(m, model_utf16)) return ec; + + if (makeAbsolute) { + // Make model absolute by prepending a temp directory if it's not already. + bool absolute = sys::path::is_absolute(m); + + if (!absolute) { + SmallVector temp_dir; + if (error_code ec = TempDir(temp_dir)) return ec; + // Handle c: by removing it. + if (model_utf16.size() > 2 && model_utf16[1] == L':') { + model_utf16.erase(model_utf16.begin(), model_utf16.begin() + 2); + } + model_utf16.insert(model_utf16.begin(), temp_dir.begin(), temp_dir.end()); + } + } + + // Replace '%' with random chars. From here on, DO NOT modify model. It may be + // needed if the randomly chosen path already exists. + SmallVector random_path_utf16; + + // Get a Crypto Provider for CryptGenRandom. + HCRYPTPROV HCPC; + if (!::CryptAcquireContextW(&HCPC, + NULL, + NULL, + PROV_RSA_FULL, + CRYPT_VERIFYCONTEXT)) + return windows_error(::GetLastError()); + ScopedCryptContext CryptoProvider(HCPC); + +retry_random_path: + random_path_utf16.set_size(0); + for (SmallVectorImpl::const_iterator i = model_utf16.begin(), + e = model_utf16.end(); + i != e; ++i) { + if (*i == L'%') { + BYTE val = 0; + if (!::CryptGenRandom(CryptoProvider, 1, &val)) + return windows_error(::GetLastError()); + random_path_utf16.push_back("0123456789abcdef"[val & 15]); + } + else + random_path_utf16.push_back(*i); + } + // Make random_path_utf16 null terminated. + random_path_utf16.push_back(0); + random_path_utf16.pop_back(); + + HANDLE TempFileHandle; + + switch (Type) { + case FS_File: { + // Try to create + open the path. + TempFileHandle = + ::CreateFileW(random_path_utf16.begin(), GENERIC_READ | GENERIC_WRITE, + FILE_SHARE_READ, NULL, + // Return ERROR_FILE_EXISTS if the file + // already exists. + CREATE_NEW, FILE_ATTRIBUTE_TEMPORARY, NULL); + if (TempFileHandle == INVALID_HANDLE_VALUE) { + // If the file existed, try again, otherwise, error. + error_code ec = windows_error(::GetLastError()); + if (ec == windows_error::file_exists) + goto retry_random_path; + + return ec; + } + + // Convert the Windows API file handle into a C-runtime handle. + int fd = ::_open_osfhandle(intptr_t(TempFileHandle), 0); + if (fd == -1) { + ::CloseHandle(TempFileHandle); + ::DeleteFileW(random_path_utf16.begin()); + // MSDN doesn't say anything about _open_osfhandle setting errno or + // GetLastError(), so just return invalid_handle. + return windows_error::invalid_handle; + } + + result_fd = fd; + break; + } + + case FS_Name: { + DWORD attributes = ::GetFileAttributesW(random_path_utf16.begin()); + if (attributes != INVALID_FILE_ATTRIBUTES) + goto retry_random_path; + error_code EC = make_error_code(windows_error(::GetLastError())); + if (EC != windows_error::file_not_found && + EC != windows_error::path_not_found) + return EC; + break; + } + + case FS_Dir: + if (!::CreateDirectoryW(random_path_utf16.begin(), NULL)) { + error_code EC = windows_error(::GetLastError()); + if (EC != windows_error::already_exists) + return EC; + goto retry_random_path; + } + break; + } + + // Set result_path to the utf-8 representation of the path. + if (error_code ec = UTF16ToUTF8(random_path_utf16.begin(), + random_path_utf16.size(), result_path)) { + switch (Type) { + case FS_File: + ::CloseHandle(TempFileHandle); + ::DeleteFileW(random_path_utf16.begin()); + case FS_Name: + break; + case FS_Dir: + ::RemoveDirectoryW(random_path_utf16.begin()); + break; + } + return ec; + } + + return error_code::success(); +} + namespace llvm { namespace sys { namespace fs { @@ -604,141 +738,6 @@ error_code setLastModificationAndAccessTime(int FD, TimeValue Time) { return error_code::success(); } -// FIXME: mode should be used here and default to user r/w only, -// it currently comes in as a UNIX mode. -error_code unique_file(const Twine &model, int &result_fd, - SmallVectorImpl &result_path, - bool makeAbsolute, unsigned mode) { - // Use result_path as temp storage. - result_path.set_size(0); - StringRef m = model.toStringRef(result_path); - - SmallVector model_utf16; - if (error_code ec = UTF8ToUTF16(m, model_utf16)) return ec; - - if (makeAbsolute) { - // Make model absolute by prepending a temp directory if it's not already. - bool absolute = path::is_absolute(m); - - if (!absolute) { - SmallVector temp_dir; - if (error_code ec = TempDir(temp_dir)) return ec; - // Handle c: by removing it. - if (model_utf16.size() > 2 && model_utf16[1] == L':') { - model_utf16.erase(model_utf16.begin(), model_utf16.begin() + 2); - } - model_utf16.insert(model_utf16.begin(), temp_dir.begin(), temp_dir.end()); - } - } - - // Replace '%' with random chars. From here on, DO NOT modify model. It may be - // needed if the randomly chosen path already exists. - SmallVector random_path_utf16; - - // Get a Crypto Provider for CryptGenRandom. - HCRYPTPROV HCPC; - if (!::CryptAcquireContextW(&HCPC, - NULL, - NULL, - PROV_RSA_FULL, - CRYPT_VERIFYCONTEXT)) - return windows_error(::GetLastError()); - ScopedCryptContext CryptoProvider(HCPC); - -retry_random_path: - random_path_utf16.set_size(0); - for (SmallVectorImpl::const_iterator i = model_utf16.begin(), - e = model_utf16.end(); - i != e; ++i) { - if (*i == L'%') { - BYTE val = 0; - if (!::CryptGenRandom(CryptoProvider, 1, &val)) - return windows_error(::GetLastError()); - random_path_utf16.push_back("0123456789abcdef"[val & 15]); - } - else - random_path_utf16.push_back(*i); - } - // Make random_path_utf16 null terminated. - random_path_utf16.push_back(0); - random_path_utf16.pop_back(); - - // Make sure we don't fall into an infinite loop by constantly trying - // to create the parent path. - bool TriedToCreateParent = false; - - // Try to create + open the path. -retry_create_file: - HANDLE TempFileHandle = ::CreateFileW(random_path_utf16.begin(), - GENERIC_READ | GENERIC_WRITE, - FILE_SHARE_READ, - NULL, - // Return ERROR_FILE_EXISTS if the file - // already exists. - CREATE_NEW, - FILE_ATTRIBUTE_TEMPORARY, - NULL); - if (TempFileHandle == INVALID_HANDLE_VALUE) { - // If the file existed, try again, otherwise, error. - error_code ec = windows_error(::GetLastError()); - if (ec == windows_error::file_exists) - goto retry_random_path; - // Check for non-existing parent directories. - if (ec == windows_error::path_not_found && !TriedToCreateParent) { - TriedToCreateParent = true; - - // Create the directories using result_path as temp storage. - if (error_code ec = UTF16ToUTF8(random_path_utf16.begin(), - random_path_utf16.size(), result_path)) - return ec; - StringRef p(result_path.begin(), result_path.size()); - SmallString<64> dir_to_create; - for (path::const_iterator i = path::begin(p), - e = --path::end(p); i != e; ++i) { - path::append(dir_to_create, *i); - bool Exists; - if (error_code ec = exists(Twine(dir_to_create), Exists)) return ec; - if (!Exists) { - // If c: doesn't exist, bail. - if (i->endswith(":")) - return ec; - - SmallVector dir_to_create_utf16; - if (error_code ec = UTF8ToUTF16(dir_to_create, dir_to_create_utf16)) - return ec; - - // Create the directory. - if (!::CreateDirectoryW(dir_to_create_utf16.begin(), NULL)) - return windows_error(::GetLastError()); - } - } - goto retry_create_file; - } - return ec; - } - - // Set result_path to the utf-8 representation of the path. - if (error_code ec = UTF16ToUTF8(random_path_utf16.begin(), - random_path_utf16.size(), result_path)) { - ::CloseHandle(TempFileHandle); - ::DeleteFileW(random_path_utf16.begin()); - return ec; - } - - // Convert the Windows API file handle into a C-runtime handle. - int fd = ::_open_osfhandle(intptr_t(TempFileHandle), 0); - if (fd == -1) { - ::CloseHandle(TempFileHandle); - ::DeleteFileW(random_path_utf16.begin()); - // MSDN doesn't say anything about _open_osfhandle setting errno or - // GetLastError(), so just return invalid_handle. - return windows_error::invalid_handle; - } - - result_fd = fd; - return error_code::success(); -} - error_code get_magic(const Twine &path, uint32_t len, SmallVectorImpl &result) { SmallString<128> path_storage;