From 099ac2cd2fa91e73ba1a6b48331e21141b693901 Mon Sep 17 00:00:00 2001 From: Larry Gritz Date: Sat, 17 Dec 2022 10:54:00 -0800 Subject: [PATCH] IOProxy read/write error handling improvements IOMemReader::pread catch out-of-range read positions. Fixes #3711 Along the way, I noticed that the system pread & pwrite return a ssize_t rather than a size_t as our IOProxy methods do, so I need to catch the negative value returns that they use to indicate errors. --- src/include/OpenImageIO/filesystem.h | 6 ++++++ src/libutil/filesystem.cpp | 21 +++++++++++++++++---- 2 files changed, 23 insertions(+), 4 deletions(-) diff --git a/src/include/OpenImageIO/filesystem.h b/src/include/OpenImageIO/filesystem.h index aac776b5ba..722bf4f6b5 100644 --- a/src/include/OpenImageIO/filesystem.h +++ b/src/include/OpenImageIO/filesystem.h @@ -400,6 +400,12 @@ OIIO_UTIL_API std::string filename_to_regex(string_view pattern, /// Proxy class for I/O. This provides a simplified interface for file I/O /// that can have custom overrides. All char-based filenames are assumed to be /// UTF-8 encoded. +/// +/// Note: We intended for read, write, pread, pwrite to closely mimic the +/// standard C functions, but we erred, declaring these as returning size_t +/// instead of ssize_t. We can't change this now without breaking source +/// compatibility, so we have to live with it, but we'll probably change it at +/// the next major compatibility break (OIIO 3.0). class OIIO_UTIL_API IOProxy { public: enum Mode { Closed = 0, Read = 'r', Write = 'w' }; diff --git a/src/libutil/filesystem.cpp b/src/libutil/filesystem.cpp index 47619c7dd9..b59de52e8a 100644 --- a/src/libutil/filesystem.cpp +++ b/src/libutil/filesystem.cpp @@ -1303,7 +1303,9 @@ Filesystem::IOFile::pread(void* buf, size_t size, int64_t offset) return r; #else /* Non-Windows: assume POSIX pread is available */ int fd = fileno(m_file); - return ::pread(fd, buf, size, offset); + auto r = ::pread(fd, buf, size, offset); + // FIXME: the system pread returns ssize_t and is -1 on error. + return r < 0 ? size_t(0) : size_t(r); #endif } @@ -1332,8 +1334,10 @@ Filesystem::IOFile::pwrite(const void* buf, size_t size, int64_t offset) 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); + int fd = fileno(m_file); + auto r = ::pwrite(fd, buf, size, offset); + // FIXME: the system pwrite returns ssize_t and is -1 on error. + return r < 0 ? size_t(0) : size_t(r); #endif offset += r; if (m_pos > int64_t(m_size)) @@ -1400,8 +1404,17 @@ size_t Filesystem::IOMemReader::pread(void* buf, size_t size, int64_t offset) { // N.B. No lock necessary - if (size + size_t(offset) > size_t(m_buf.size())) + if (!m_buf.size() || !size) + return 0; + if (size + size_t(offset) > size_t(m_buf.size())) { + if (offset < 0 || offset >= m_buf.size()) { + error(Strutil::fmt::format( + "Invalid pread offset {} for an IOMemReader buffer of size {}", + offset, m_buf.size())); + return 0; + } size = m_buf.size() - size_t(offset); + } memcpy(buf, m_buf.data() + offset, size); return size; }