[BUG] Segfault Due to Logic Error; read negative size

[GreyHak]

Describe the bug
I'm using a IOMemReader with an ImageInput to parse an image. When doing this I sometimes get segfaults because of a logic error causing a negative memcpy. The problem is in src/libutil/filesystem.cpp in the Filesystem::IOMemReader::pread routine. There's an if to check if the read will exceed the buffer size. The problem is with the change to size. If offset somehow becomes greater than m_buf.size(), then size becomes negative; well, really big because it's unsigned. I would guess that the m_pos became greater than m_buf.size() because the seek operation does not validate offset against the size.

To Reproduce
Steps to reproduce the behavior:

1. Two lines of code

OIIO::Filesystem::IOMemReader memReader(imgString.c_str(), imgString.length());
auto in = OIIO::ImageInput::open("name", nullptr, &memReader);

2. Open an image. Like this one https://ercim-news.ercim.eu/images/flags/gr.png
3. Segfault

Expected behavior
The file should open without errors.

Evidence

Thread 2 received signal SIGSEGV, Segmentation fault.
[Switching to Thread 0x7ffff564d640 (LWP 3719)]
__memmove_sse2_unaligned_erms () at ../sysdeps/x86_64/multiarch/memmove-vec-unaligned-erms.S:273
Downloading source file /usr/src/debug/glibc-2.34-49.el9.x86_64/string/../sysdeps/x86_64/multiarch/memmove-vec-unaligned-erms.S...
273             VMOVU   (%rsi), %VEC(0)
(gdb) bt
#0  __memmove_sse2_unaligned_erms () at ../sysdeps/x86_64/multiarch/memmove-vec-unaligned-erms.S:273
#1  0x00007ffff6d253a8 in OpenImageIO_v2_5_0::Filesystem::IOMemReader::read(void*, unsigned long) () from ../external/oiio/dist/lib64/libOpenImageIO_Util.so.2.5.0
#2  0x00007ffff661e36f in OpenImageIO_v2_5_0::ImageInput::ioread(void*, unsigned long, unsigned long) () from ../external/oiio/dist/lib64/libOpenImageIO.so.2.5.0
#3  0x00007ffff6796a65 in OpenImageIO_v2_5_0::IffInput::read_header() () from ../external/oiio/dist/lib64/libOpenImageIO.so.2.5.0
#4  0x00007ffff6798104 in OpenImageIO_v2_5_0::IffInput::open(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, OpenImageIO_v2_5_0::ImageSpec&) ()
   from ../external/oiio/dist/lib64/libOpenImageIO.so.2.5.0
#5  0x00007ffff66405f3 in OpenImageIO_v2_5_0::ImageInput::create(OpenImageIO_v2_5_0::basic_string_view<char, std::char_traits<char> >, bool, OpenImageIO_v2_5_0::ImageSpec const*, OpenImageIO_v2_5_0::Filesystem::IOProxy*, OpenImageIO_v2_5_0::basic_string_view<char, std::char_traits<char> >) () from ../external/oiio/dist/lib64/libOpenImageIO.so.2.5.0
#6  0x00007ffff661d9ed in OpenImageIO_v2_5_0::ImageInput::open(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, OpenImageIO_v2_5_0::ImageSpec const*, OpenImageIO_v2_5_0::Filesystem::IOProxy*) () from ../external/oiio/dist/lib64/libOpenImageIO.so.2.5.0

Platform information:

• OIIO branch/version: master commit 1449b46
• OS: CentOS Stream 9
• C++ compiler: gcc version 11.3.1 20221121
• Any non-default build flags when you build OIIO: None

Fix:
This is the quick fix I tested to verify the problem. A better fix might be to validate the seek offset. Both issues should probably be fixed. I'm not sure how you would want to handle this error. At least this code will show you how to catch the problem.

diff --git a/src/libutil/filesystem.cpp b/src/libutil/filesystem.cpp
index 47619c7d..5c1a213e 100644
--- a/src/libutil/filesystem.cpp
+++ b/src/libutil/filesystem.cpp
@@ -1401,7 +1401,14 @@ 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 (size_t(offset) > size_t(m_buf.size()))
+        {
+           std::cerr << "ERROR: offset greater than buffer" << std::endl;
+           return 0;
+        }
         size = m_buf.size() - size_t(offset);
+    }
     memcpy(buf, m_buf.data() + offset, size);
     return size;
 }

[lgritz]

Thanks, good catch. Here's the fix I a proposing: #3712