refactor: Use spans to solve a number of memory safety issues (#4148)

* exif.cpp: use spans to avoid several possible buffer overruns
* paramlist_test.cpp: use spans to be sure we're ok on memory bounds
* jpeg: use spans in jpeg icc profile manipulation for memory safety
* psd output: switch cmyk conversion to use spans for memory safety

I've been cobbling these together bit by bit over the last several
weeks, tracking down the memory safety issues identified by the Sonar
static analysis. I think that *mostly* they are not real bugs, but the
code is confusing enough that it's hard to verify. This refactor makes
it safer, and even if the code was previously correct but merely hard to
analyze, the bounds checking in span (for debug mode) gives the static
analyzer the clues it needs to know this is safe.

As further explanation, these situations generally involve something
that looks like this, schematically:

    caller () {
        T buffer[known_size];
        call function(buffer, ...params...);
    }

    function(T* buffer, ...params...) {
        i, j = ...very complex things, hard to analyze...
        foo = buffer[i];
        buffer[j] = bar;
        // Did we do a buffer overrun??? ¯\_(ツ)_/¯
        // This function doesn't necessarily have knowldege of the
        // true bounds, even if it wanted to check every access.
    }

By rewriting this function as follows:

    function(span<T> buffer) {
        // all indexed access to buffer will be bounds checked in debug
    }

we are passing the bounds as well, and at least for debug builds, are
also checking bounds with an assertion on every indexed access into
buffer, without the function needing a lot of clutter. Fire and forget.

---------

Signed-off-by: Larry Gritz <[email protected]>

Author: lgritz

src/include/OpenImageIO/span_util.h

@@ -69,6 +69,13 @@ make_span(T (&arg)[N])  // span from C array of known length
     return { arg };
 }
 
+template<typename T>
+inline constexpr span<T>
+make_span(T* data, oiio_span_size_type size)  // span from ptr + size
+{
+    return { data, size };
+}
+
 template<typename T, size_t N>
 inline constexpr cspan<T>
 make_cspan(T (&arg)[N])  // cspan from C array of known length
@@ -83,6 +90,13 @@ make_cspan(const T& arg)  // cspan from a single value
     return { &arg, 1 };
 }
 
+template<typename T>
+inline constexpr cspan<T>
+make_cspan(const T* data, oiio_span_size_type size)  // cspan from ptr + size
+{
+    return { data, size };
+}
+
 
 
 /// Try to copy `n` items of type `T` from `src[srcoffset...]` to

src/jpeg.imageio/jpeg_pvt.h

@@ -36,7 +36,7 @@ extern "C" {
 OIIO_PLUGIN_NAMESPACE_BEGIN
 
 
-#define MAX_DATA_BYTES_IN_MARKER 65519L
+#define MAX_DATA_BYTES_IN_MARKER 65519UL
 #define ICC_HEADER_SIZE 14
 #define ICC_PROFILE_ATTR "ICCProfile"
 

src/jpeg.imageio/jpegoutput.cpp

@@ -9,6 +9,7 @@
 #include <OpenImageIO/filesystem.h>
 #include <OpenImageIO/fmath.h>
 #include <OpenImageIO/imageio.h>
+#include <OpenImageIO/span_util.h>
 #include <OpenImageIO/tiffutils.h>
 
 #include "jpeg_pvt.h"
@@ -280,36 +281,33 @@ JpgOutput::open(const std::string& name, const ImageSpec& newspec,
     m_spec.set_format(TypeDesc::UINT8);  // JPG is only 8 bit
 
     // Write ICC profile, if we have anything
-    const ParamValue* icc_profile_parameter = m_spec.find_attribute(
-        ICC_PROFILE_ATTR);
-    if (icc_profile_parameter != NULL) {
-        unsigned char* icc_profile
-            = (unsigned char*)icc_profile_parameter->data();
-        unsigned int icc_profile_length = icc_profile_parameter->type().size();
-        if (icc_profile && icc_profile_length) {
+    if (auto icc_profile_parameter = m_spec.find_attribute(ICC_PROFILE_ATTR)) {
+        cspan<unsigned char> icc_profile((unsigned char*)
+                                             icc_profile_parameter->data(),
+                                         icc_profile_parameter->type().size());
+        if (icc_profile.size() && icc_profile.data()) {
             /* Calculate the number of markers we'll need, rounding up of course */
-            int num_markers = icc_profile_length / MAX_DATA_BYTES_IN_MARKER;
-            if ((unsigned int)(num_markers * MAX_DATA_BYTES_IN_MARKER)
-                != icc_profile_length)
+            size_t num_markers = icc_profile.size() / MAX_DATA_BYTES_IN_MARKER;
+            if (num_markers * MAX_DATA_BYTES_IN_MARKER
+                != std::size(icc_profile))
                 num_markers++;
-            int curr_marker     = 1; /* per spec, count starts at 1*/
-            size_t profile_size = MAX_DATA_BYTES_IN_MARKER + ICC_HEADER_SIZE;
-            std::vector<JOCTET> profile(profile_size);
+            int curr_marker = 1; /* per spec, count starts at 1*/
+            std::vector<JOCTET> profile(MAX_DATA_BYTES_IN_MARKER
+                                        + ICC_HEADER_SIZE);
+            size_t icc_profile_length = icc_profile.size();
             while (icc_profile_length > 0) {
                 // length of profile to put in this marker
-                unsigned int length
-                    = std::min(icc_profile_length,
-                               (unsigned int)MAX_DATA_BYTES_IN_MARKER);
+                size_t length = std::min(icc_profile_length,
+                                         size_t(MAX_DATA_BYTES_IN_MARKER));
                 icc_profile_length -= length;
                 // Write the JPEG marker header (APP2 code and marker length)
                 strcpy((char*)profile.data(), "ICC_PROFILE");  // NOSONAR
                 profile[11] = 0;
                 profile[12] = curr_marker;
                 profile[13] = (JOCTET)num_markers;
-                OIIO_ASSERT(profile_size >= ICC_HEADER_SIZE + length);
-                memcpy(profile.data() + ICC_HEADER_SIZE,
-                       icc_profile + length * (curr_marker - 1),
-                       length);  //NOSONAR
+                OIIO_ASSERT(profile.size() >= ICC_HEADER_SIZE + length);
+                spancpy(make_span(profile), ICC_HEADER_SIZE, icc_profile,
+                        length * (curr_marker - 1), length);
                 jpeg_write_marker(&m_cinfo, JPEG_APP0 + 2, profile.data(),
                                   ICC_HEADER_SIZE + length);
                 curr_marker++;

src/libOpenImageIO/exif.cpp

@@ -639,85 +639,117 @@ add_exif_item_to_spec(ImageSpec& spec, const char* name,
                       int offset_adjustment = 0)
 {
     OIIO_ASSERT(dirp);
-    const uint8_t* dataptr = (const uint8_t*)pvt::dataptr(*dirp, buf,
-                                                          offset_adjustment);
-    if (!dataptr)
-        return;
     TypeDesc type = tiff_datatype_to_typedesc(*dirp);
+    size_t count  = dirp->tdir_count;
     if (dirp->tdir_type == TIFF_SHORT) {
-        std::vector<uint16_t> d((const uint16_t*)dataptr,
-                                (const uint16_t*)dataptr + dirp->tdir_count);
-        if (swab)
-            swap_endian(d.data(), d.size());
-        spec.attribute(name, type, d.data());
+        cspan<uint8_t> dspan
+            = pvt::dataspan<uint16_t>(*dirp, buf, offset_adjustment, count);
+        if (dspan.empty())
+            return;
+        if (swab) {
+            // In the byte swap case, copy it into a vector because the
+            // upstream source isn't mutable.
+            std::vector<uint16_t> dswab((const uint16_t*)dspan.begin(),
+                                        (const uint16_t*)dspan.end());
+            swap_endian(dswab.data(), dswab.size());
+            spec.attribute(name, type, dswab.data());
+        } else {
+            spec.attribute(name, type, dspan.data());
+        }
         return;
     }
     if (dirp->tdir_type == TIFF_LONG) {
-        std::vector<uint32_t> d((const uint32_t*)dataptr,
-                                (const uint32_t*)dataptr + dirp->tdir_count);
-        if (swab)
-            swap_endian(d.data(), d.size());
-        spec.attribute(name, type, d.data());
+        cspan<uint8_t> dspan
+            = pvt::dataspan<uint32_t>(*dirp, buf, offset_adjustment, count);
+        if (dspan.empty())
+            return;
+        if (swab) {
+            // In the byte swap case, copy it into a vector because the
+            // upstream source isn't mutable.
+            std::vector<uint32_t> dswab((const uint32_t*)dspan.begin(),
+                                        (const uint32_t*)dspan.end());
+            swap_endian(dswab.data(), dswab.size());
+            spec.attribute(name, type, dswab.data());
+        } else {
+            spec.attribute(name, type, dspan.data());
+        }
         return;
     }
     if (dirp->tdir_type == TIFF_RATIONAL) {
-        int n    = dirp->tdir_count;  // How many
-        float* f = OIIO_ALLOCA(float, n);
-        for (int i = 0; i < n; ++i) {
+        cspan<uint8_t> dspan
+            = pvt::dataspan<uint32_t>(*dirp, buf, offset_adjustment, 2 * count);
+        if (dspan.empty())
+            return;
+        float* f = OIIO_ALLOCA(float, count);
+        for (size_t i = 0; i < count; ++i) {
+            // Because the values in the blob aren't 32-bit-aligned, memcpy
+            // them into ints to do the swapping.
             unsigned int num, den;
-            memcpy(&num, dataptr + (2 * i) * sizeof(unsigned int),
-                   sizeof(unsigned int));
-            memcpy(&den, dataptr + (2 * i + 1) * sizeof(unsigned int),
+            memcpy(&num, dspan.data() + (2 * i) * sizeof(unsigned int),
+                   sizeof(unsigned int));  //NOSONAR
+            memcpy(&den, dspan.data() + (2 * i + 1) * sizeof(unsigned int),
                    sizeof(unsigned int));  //NOSONAR
             if (swab) {
-                swap_endian(&num);
-                swap_endian(&den);
+                num = byteswap(num);
+                den = byteswap(den);
             }
             f[i] = (float)((double)num / (double)den);
         }
         if (dirp->tdir_count == 1)
-            spec.attribute(name, *f);
+            spec.attribute(name, f[0]);
         else
-            spec.attribute(name, TypeDesc(TypeDesc::FLOAT, n), f);
+            spec.attribute(name, TypeDesc(TypeDesc::FLOAT, int(count)), f);
         return;
     }
     if (dirp->tdir_type == TIFF_SRATIONAL) {
-        int n    = dirp->tdir_count;  // How many
-        float* f = OIIO_ALLOCA(float, n);
-        for (int i = 0; i < n; ++i) {
+        cspan<uint8_t> dspan
+            = pvt::dataspan<int32_t>(*dirp, buf, offset_adjustment, 2 * count);
+        if (dspan.empty())
+            return;
+        float* f = OIIO_ALLOCA(float, count);
+        for (size_t i = 0; i < count; ++i) {
+            // Because the values in the blob aren't 32-bit-aligned, memcpy
+            // them into ints to do the swapping.
             int num, den;
-            memcpy(&num, dataptr + (2 * i) * sizeof(int), sizeof(int));
-            memcpy(&den, dataptr + (2 * i + 1) * sizeof(int),
+            memcpy(&num, dspan.data() + (2 * i) * sizeof(int),
+                   sizeof(int));  //NOSONAR
+            memcpy(&den, dspan.data() + (2 * i + 1) * sizeof(int),
                    sizeof(int));  //NOSONAR
             if (swab) {
-                swap_endian(&num);
-                swap_endian(&den);
+                num = byteswap(num);
+                den = byteswap(den);
             }
             f[i] = (float)((double)num / (double)den);
         }
         if (dirp->tdir_count == 1)
-            spec.attribute(name, *f);
+            spec.attribute(name, f[0]);
         else
-            spec.attribute(name, TypeDesc(TypeDesc::FLOAT, n), f);
+            spec.attribute(name, TypeDesc(TypeDesc::FLOAT, int(count)), f);
         return;
     }
     if (dirp->tdir_type == TIFF_ASCII) {
-        int len         = tiff_data_size(*dirp);
-        const char* ptr = (const char*)dataptr;
-        while (len && ptr[len - 1] == 0)  // Don't grab the terminating null
-            --len;
-        std::string str(ptr, len);
+        size_t len           = tiff_data_size(*dirp);
+        cspan<uint8_t> dspan = pvt::dataspan<char>(*dirp, buf,
+                                                   offset_adjustment, len);
+        if (dspan.empty())
+            return;
+        // Don't grab the terminating null
+        while (dspan.size() && dspan.back() == 0)
+            dspan = dspan.subspan(0, dspan.size() - 1);
+        std::string str(dspan.begin(), dspan.end());
         if (strlen(str.c_str()) < str.length())  // Stray \0 in the middle
             str = std::string(str.c_str());
         spec.attribute(name, str);
         return;
     }
-    if (dirp->tdir_type == TIFF_BYTE && dirp->tdir_count == 1) {
+    if (dirp->tdir_type == TIFF_BYTE && count == 1) {
         // Not sure how to handle "bytes" generally, but certainly for just
         // one, add it as an int.
-        unsigned char d;
-        d = *dataptr;  // byte stored in offset itself
-        spec.attribute(name, (int)d);
+        cspan<uint8_t> dspan = pvt::dataspan<uint8_t>(*dirp, buf,
+                                                      offset_adjustment, count);
+        if (dspan.empty())
+            return;
+        spec.attribute(name, (int)dspan[0]);
         return;
     }
 

src/libOpenImageIO/exif.h

@@ -44,6 +44,30 @@ dataptr(const TIFFDirEntry& td, cspan<uint8_t> data, int offset_adjustment)
 
 
 
+// Return a span that bounds offset data within the dir entry.
+// No matter what the type T, we still return a cspan<uint8_t> because it's
+// unaligned data somewhere in the middle of a byte array. We would have
+// alignment errors if we try to access it as some other type if it's not
+// properly aligned.
+template<typename T>
+inline cspan<uint8_t>
+dataspan(const TIFFDirEntry& td, cspan<uint8_t> data, int offset_adjustment,
+         size_t count)
+{
+    size_t len = tiff_data_size(td);
+    OIIO_DASSERT(len == sizeof(T) * count);
+    if (len <= 4)
+        return { (const uint8_t*)&td.tdir_offset, oiio_span_size_type(len) };
+    else {
+        int offset = td.tdir_offset + offset_adjustment;
+        if (offset < 0 || size_t(offset) + len > std::size(data))
+            return {};  // out of bounds! return empty span
+        return { data.data() + offset, oiio_span_size_type(len) };
+    }
+}
+
+
+
 struct LabelIndex {
     int value;
     const char* label;