From cc3a595ab938352f3acf8652c5858ddf879524a5 Mon Sep 17 00:00:00 2001 From: "Michael J. Spencer" Date: Thu, 14 Mar 2013 00:20:10 +0000 Subject: [PATCH] [Support] Fix lifetime of file descriptors when using MemoryBuffer. Clients of MemoryBuffer::getOpenFile expect it not to take ownership of the file descriptor passed in. So don't. git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@176995 91177308-0d34-0410-b5e6-96231b3b80d8 --- include/llvm/Support/FileSystem.h | 6 +++-- lib/Support/FileOutputBuffer.cpp | 4 ++-- lib/Support/MemoryBuffer.cpp | 3 ++- lib/Support/Unix/PathV2.inc | 17 ++++++++------ lib/Support/Windows/PathV2.inc | 37 +++++++++++++++++++------------ 5 files changed, 41 insertions(+), 26 deletions(-) diff --git a/include/llvm/Support/FileSystem.h b/include/llvm/Support/FileSystem.h index bae3a5a608c..ffa642787b0 100644 --- a/include/llvm/Support/FileSystem.h +++ b/include/llvm/Support/FileSystem.h @@ -602,7 +602,7 @@ private: void *FileMappingHandle; #endif - error_code init(int FD, uint64_t Offset); + error_code init(int FD, bool CloseFD, uint64_t Offset); public: typedef char char_type; @@ -633,8 +633,10 @@ public: error_code &ec); /// \param fd An open file descriptor to map. mapped_file_region takes - /// ownership. It must have been opended in the correct mode. + /// ownership if closefd is true. It must have been opended in the correct + /// mode. mapped_file_region(int fd, + bool closefd, mapmode mode, uint64_t length, uint64_t offset, diff --git a/lib/Support/FileOutputBuffer.cpp b/lib/Support/FileOutputBuffer.cpp index cd430f218bc..1ee69b60234 100644 --- a/lib/Support/FileOutputBuffer.cpp +++ b/lib/Support/FileOutputBuffer.cpp @@ -70,8 +70,8 @@ error_code FileOutputBuffer::create(StringRef FilePath, if (EC) return EC; - OwningPtr MappedFile( - new mapped_file_region(FD, mapped_file_region::readwrite, Size, 0, EC)); + OwningPtr MappedFile(new mapped_file_region( + FD, true, mapped_file_region::readwrite, Size, 0, EC)); if (EC) return EC; diff --git a/lib/Support/MemoryBuffer.cpp b/lib/Support/MemoryBuffer.cpp index 1c354be2e09..80422372539 100644 --- a/lib/Support/MemoryBuffer.cpp +++ b/lib/Support/MemoryBuffer.cpp @@ -206,7 +206,7 @@ class MemoryBufferMMapFile : public MemoryBuffer { public: MemoryBufferMMapFile(bool RequiresNullTerminator, int FD, uint64_t Len, uint64_t Offset, error_code EC) - : MFR(FD, sys::fs::mapped_file_region::readonly, + : MFR(FD, false, sys::fs::mapped_file_region::readonly, getLegalMapSize(Len, Offset), getLegalMapOffset(Offset), EC) { if (!EC) { const char *Start = getStart(Len, Offset); @@ -281,6 +281,7 @@ error_code MemoryBuffer::getFile(const char *Filename, error_code ret = getOpenFile(FD, Filename, result, FileSize, FileSize, 0, RequiresNullTerminator); + close(FD); return ret; } diff --git a/lib/Support/Unix/PathV2.inc b/lib/Support/Unix/PathV2.inc index 44b31b3202f..a3dfd4b0a32 100644 --- a/lib/Support/Unix/PathV2.inc +++ b/lib/Support/Unix/PathV2.inc @@ -475,12 +475,14 @@ rety_open_create: return error_code::success(); } -error_code mapped_file_region::init(int fd, uint64_t offset) { - AutoFD FD(fd); +error_code mapped_file_region::init(int FD, bool CloseFD, uint64_t Offset) { + AutoFD ScopedFD(FD); + if (!CloseFD) + ScopedFD.take(); // Figure out how large the file is. struct stat FileInfo; - if (fstat(fd, &FileInfo) == -1) + if (fstat(FD, &FileInfo) == -1) return error_code(errno, system_category()); uint64_t FileSize = FileInfo.st_size; @@ -488,7 +490,7 @@ error_code mapped_file_region::init(int fd, uint64_t offset) { Size = FileSize; else if (FileSize < Size) { // We need to grow the file. - if (ftruncate(fd, Size) == -1) + if (ftruncate(FD, Size) == -1) return error_code(errno, system_category()); } @@ -497,7 +499,7 @@ error_code mapped_file_region::init(int fd, uint64_t offset) { #ifdef MAP_FILE flags |= MAP_FILE; #endif - Mapping = ::mmap(0, Size, prot, flags, fd, offset); + Mapping = ::mmap(0, Size, prot, flags, FD, Offset); if (Mapping == MAP_FAILED) return error_code(errno, system_category()); return error_code::success(); @@ -526,12 +528,13 @@ mapped_file_region::mapped_file_region(const Twine &path, return; } - ec = init(ofd, offset); + ec = init(ofd, true, offset); if (ec) Mapping = 0; } mapped_file_region::mapped_file_region(int fd, + bool closefd, mapmode mode, uint64_t length, uint64_t offset, @@ -545,7 +548,7 @@ mapped_file_region::mapped_file_region(int fd, return; } - ec = init(fd, offset); + ec = init(fd, closefd, offset); if (ec) Mapping = 0; } diff --git a/lib/Support/Windows/PathV2.inc b/lib/Support/Windows/PathV2.inc index 823f7583dac..0f657bf3b95 100644 --- a/lib/Support/Windows/PathV2.inc +++ b/lib/Support/Windows/PathV2.inc @@ -711,12 +711,13 @@ error_code get_magic(const Twine &path, uint32_t len, return error_code::success(); } -error_code mapped_file_region::init(int FD, uint64_t Offset) { +error_code mapped_file_region::init(int FD, bool CloseFD, uint64_t Offset) { FileDescriptor = FD; // Make sure that the requested size fits within SIZE_T. if (Size > std::numeric_limits::max()) { if (FileDescriptor) - _close(FileDescriptor); + if (CloseFD) + _close(FileDescriptor); else ::CloseHandle(FileHandle); return make_error_code(errc::invalid_argument); @@ -739,7 +740,8 @@ error_code mapped_file_region::init(int FD, uint64_t Offset) { if (FileMappingHandle == NULL) { error_code ec = windows_error(GetLastError()); if (FileDescriptor) - _close(FileDescriptor); + if (CloseFD) + _close(FileDescriptor); else ::CloseHandle(FileHandle); return ec; @@ -761,7 +763,8 @@ error_code mapped_file_region::init(int FD, uint64_t Offset) { error_code ec = windows_error(GetLastError()); ::CloseHandle(FileMappingHandle); if (FileDescriptor) - _close(FileDescriptor); + if (CloseFD) + _close(FileDescriptor); else ::CloseHandle(FileHandle); return ec; @@ -775,13 +778,23 @@ error_code mapped_file_region::init(int FD, uint64_t Offset) { ::UnmapViewOfFile(Mapping); ::CloseHandle(FileMappingHandle); if (FileDescriptor) - _close(FileDescriptor); + if (CloseFD) + _close(FileDescriptor); else ::CloseHandle(FileHandle); return ec; } Size = mbi.RegionSize; } + + // Close all the handles except for the view. It will keep the other handles + // alive. + ::CloseHandle(FileMappingHandle); + if (FileDescriptor) + if (CloseFD) + _close(FileDescriptor); // Also closes FileHandle. + else + ::CloseHandle(FileHandle); return error_code::success(); } @@ -821,7 +834,7 @@ mapped_file_region::mapped_file_region(const Twine &path, } FileDescriptor = 0; - ec = init(FileDescriptor, offset); + ec = init(FileDescriptor, true, offset); if (ec) { Mapping = FileMappingHandle = 0; FileHandle = INVALID_HANDLE_VALUE; @@ -830,6 +843,7 @@ mapped_file_region::mapped_file_region(const Twine &path, } mapped_file_region::mapped_file_region(int fd, + bool closefd, mapmode mode, uint64_t length, uint64_t offset, @@ -842,13 +856,14 @@ mapped_file_region::mapped_file_region(int fd, , FileMappingHandle() { FileHandle = reinterpret_cast(_get_osfhandle(fd)); if (FileHandle == INVALID_HANDLE_VALUE) { - _close(FileDescriptor); + if (closefd) + _close(FileDescriptor); FileDescriptor = 0; ec = make_error_code(errc::bad_file_descriptor); return; } - ec = init(FileDescriptor, offset); + ec = init(FileDescriptor, closefd, offset); if (ec) { Mapping = FileMappingHandle = 0; FileHandle = INVALID_HANDLE_VALUE; @@ -859,12 +874,6 @@ mapped_file_region::mapped_file_region(int fd, mapped_file_region::~mapped_file_region() { if (Mapping) ::UnmapViewOfFile(Mapping); - if (FileMappingHandle) - ::CloseHandle(FileMappingHandle); - if (FileDescriptor) - _close(FileDescriptor); - else if (FileHandle != INVALID_HANDLE_VALUE) - ::CloseHandle(FileHandle); } #if LLVM_HAS_RVALUE_REFERENCES