From 864580de417e938ad965a1710e03da35f1175b5a Mon Sep 17 00:00:00 2001 From: Larry Gritz Date: Fri, 3 May 2019 17:44:37 -0700 Subject: [PATCH] Fix IOProxy::pread on Windows that botched PNG & JPEG read (#2231) We've been plagued by recently-introduced bugs with PNG and JPEG reading that stemmed from the first IOProxy-related patch. The problem was our use of the Windows ReadFile call, which seems to not preserve the file pointer as we'd assumed (the MSDN docs are ambiguous about this). I also began to doubt its thread safety, so in the end I just replace the code with a simpler approach with a mutex and calls to tell/seek/read. Same changes to pwrite implementation. --- CHANGES.md | 3 ++- src/include/OpenImageIO/filesystem.h | 1 + src/libutil/filesystem.cpp | 33 ++++++++++------------------ 3 files changed, 15 insertions(+), 22 deletions(-) diff --git a/CHANGES.md b/CHANGES.md index cbadeb616b..3e3ff46a9b 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -1,5 +1,6 @@ -Release 2.0.8 (1 May, 2019) -- compared to 2.0.7 +Release 2.0.8 (3 May, 2019) -- compared to 2.0.7 ------------------------------------------------ +* Fix Windows broken read of JPEG & PNG in some circumstances. #2231 * Some minor fixes to JPEG & PNG reading and file error robustness. #2187 * Fix crash reading certain old nconvert-written TIFF files. #2207 * Internals: The `OIIO_DISPATCH_COMMON_TYPES2/3` macros used by many diff --git a/src/include/OpenImageIO/filesystem.h b/src/include/OpenImageIO/filesystem.h index 5b00cbf1de..7a0c212f65 100644 --- a/src/include/OpenImageIO/filesystem.h +++ b/src/include/OpenImageIO/filesystem.h @@ -420,6 +420,7 @@ class OIIO_API IOFile : public IOProxy { FILE* m_file = nullptr; size_t m_size = 0; bool m_auto_close = false; + std::mutex m_mutex; }; diff --git a/src/libutil/filesystem.cpp b/src/libutil/filesystem.cpp index 07d1e229f3..aec38f0806 100644 --- a/src/libutil/filesystem.cpp +++ b/src/libutil/filesystem.cpp @@ -988,16 +988,12 @@ Filesystem::IOFile::pread(void* buf, size_t size, int64_t offset) if (!m_file || !size || offset < 0 || m_mode != Read) return 0; #ifdef _WIN32 - // See MSDN ReadFile docs: - // https://msdn.microsoft.com/en-us/library/windows/desktop/aa365467(v=vs.85).aspx - HANDLE h = HANDLE(_get_osfhandle(_fileno(m_file))); - OVERLAPPED overlapped; - memset(&overlapped, 0, sizeof(overlapped)); - overlapped.Offset = DWORD(offset); - overlapped.OffsetHigh = DWORD(offset >> 32); - DWORD bytesread = 0; - return ReadFile(h, buf, DWORD(size), &bytesread, &overlapped) ? bytesread - : 0; + std::lock_guard lock(m_mutex); + auto origpos = tell(); + seek(offset); + size_t r = read(buf, size); + seek(origpos); + return r; #else /* Non-Windows: assume POSIX pread is available */ int fd = fileno(m_file); return ::pread(fd, buf, size, offset); @@ -1022,17 +1018,12 @@ Filesystem::IOFile::pwrite(const void* buf, size_t size, int64_t offset) if (!m_file || !size || offset < 0 || m_mode != Write) return 0; #ifdef _WIN32 - // See MSDN WriteFile docs: - // https://msdn.microsoft.com/en-us/library/windows/desktop/aa365747(v=vs.85).aspx - HANDLE h = HANDLE(_get_osfhandle(_fileno(m_file))); - OVERLAPPED overlapped; - memset(&overlapped, 0, sizeof(overlapped)); - overlapped.Offset = DWORD(offset); - overlapped.OffsetHigh = DWORD(offset >> 32); - DWORD byteswritten = 0; - size_t r = WriteFile(h, buf, DWORD(size), &byteswritten, &overlapped) - ? byteswritten - : 0; + std::lock_guard lock(m_mutex); + auto origpos = tell(); + seek(offset); + size_t r = write(buf, size); + seek(origpos); + return r; #else /* Non-Windows: assume POSIX pwrite is available */ int fd = fileno(m_file); size_t r = ::pwrite(fd, buf, size, offset);