refactor: Use spans to solve a number of memory safety issues

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;