int: Switch to spans for some exif manipulation, fixing warnings (#4689)

We recently started getting some CI failures on the "bleeding edge" test
that uses the new gcc 14, seems that the newest cut of that compiler was
giving us new warnings where it suspected we were writing past the valid
bounds of some memory areas, but it was very hard to decipher the
circumstances and either find the bug or convince myself that is was a
false positive.

To help me debug it, I switched some of the internals of exif.cpp that
passed around raw pointers in very hard-to-validate ways to instead
use spans with explicit lengths, in the hopes that the debug-mode
bounds checking it provides would allow me to more easily track down the
problem.

Now it no longer gets compiler warnings.

Did I fix a very subtle bug in the process?

Does the reformulation with spans make it easier for the compiler to
verify that we aren't stomping on memory, and that suppresses the
warning?

Either way, it now passes all CI tests and I think the new code is safer
and aligned with our strategy of slowly changing raw unbounded pointers
to using spans in order to make these kinds of bugs harder to introduce.

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

Author: lgritz

src/include/OpenImageIO/span.h

@@ -575,6 +575,17 @@ as_writable_bytes(T* ptr, size_t len) noexcept
 
 
 
+/// Convert a reference to a single variable to a const byte span of that
+/// object's memory.
+template<typename T>
+span<const std::byte>
+as_bytes_ref(const T& ref) noexcept
+{
+    return make_cspan(reinterpret_cast<const std::byte*>(&ref), sizeof(T));
+}
+
+
+
 /// Try to copy `n` items of type `T` from `src[srcoffset...]` to
 /// `dst[dstoffset...]`. Don't read or write outside the respective span
 /// boundaries. Return the number of items actually copied, which should be

src/libOpenImageIO/exif-canon.cpp

@@ -990,8 +990,9 @@ encode_indexed_tag (int tifftag, TIFFDataType tifftype, // TIFF tag and type
         }
     }
     if (anyfound)
-        append_tiff_dir_entry (dirs, data, tifftag, tifftype,
-                               array.size(), array.data(), offset_correction);
+        append_tiff_dir_entry(dirs, data, tifftag, tifftype,
+                              array.size(), as_bytes(make_cspan(array)),
+                              offset_correction);
 }
 
 
@@ -1015,8 +1016,9 @@ encode_canon_makernote (std::vector<char>& data,
                 d = param->get_ustring().c_str();
                 count = param->get_ustring().size() + 1;
             }
-            append_tiff_dir_entry (makerdirs, data, t.tifftag, t.tifftype,
-                                   count, d, offset_correction);
+            append_tiff_dir_entry(makerdirs, data, t.tifftag, t.tifftype,
+                                  count, as_bytes(d, count),
+                                  offset_correction);
         }
     }
 

src/libOpenImageIO/exif.cpp

@@ -949,22 +949,24 @@ append_tiff_dir_entry_integer(const ParamValue& p,
 {
     T i;
     switch (p.type().basetype) {
-    case TypeDesc::UINT: i = (T) * (unsigned int*)p.data(); break;
-    case TypeDesc::INT: i = (T) * (int*)p.data(); break;
-    case TypeDesc::UINT16: i = (T) * (unsigned short*)p.data(); break;
-    case TypeDesc::INT16: i = (T) * (short*)p.data(); break;
+    case TypeDesc::UINT: i = static_cast<T>(p.get<uint32_t>()); break;
+    case TypeDesc::INT: i = static_cast<T>(p.get<int32_t>()); break;
+    case TypeDesc::UINT16: i = static_cast<T>(p.get<uint16_t>()); break;
+    case TypeDesc::INT16: i = static_cast<T>(p.get<int16_t>()); break;
+    case TypeDesc::UINT8: i = static_cast<T>(p.get<uint8_t>()); break;
+    case TypeDesc::INT8: i = static_cast<T>(p.get<int8_t>()); break;
     case TypeDesc::STRING: {
         if (Strutil::string_is_int(p.get_ustring())) {
-            i = (T)p.get_int();
+            i = static_cast<T>(p.get_int());
             break;
         } else {
             return false;
         }
     }
     default: return false;
     }
-    append_tiff_dir_entry(dirs, data, tag, type, 1, &i, offset_correction, 0,
-                          endianreq);
+    append_tiff_dir_entry(dirs, data, tag, type, 1, as_bytes_ref(i),
+                          offset_correction, 0, endianreq);
     return true;
 }
 
@@ -992,30 +994,31 @@ encode_exif_entry(const ParamValue& p, int tag, std::vector<TIFFDirEntry>& dirs,
             ustring s = *(const ustring*)p.data();
             if (s.size()) {
                 int len = size_t(s.size()) + 1;
-                append_tiff_dir_entry(dirs, data, tag, type, len, s.data(),
+                append_tiff_dir_entry(dirs, data, tag, type, len,
+                                      as_bytes(s.data(), len),
                                       offset_correction, 0, endianreq);
             }
             return;
         }
         break;
     case TIFF_RATIONAL:
         if (element == TypeDesc::FLOAT) {
-            unsigned int* rat = OIIO_ALLOCA(unsigned int, 2 * count);
-            const float* f    = (const float*)p.data();
+            auto rat       = OIIO_ALLOCA_SPAN(unsigned int, 2 * count);
+            const float* f = (const float*)p.data();
             for (size_t i = 0; i < count; ++i)
                 float_to_rational(f[i], rat[2 * i], rat[2 * i + 1]);
-            append_tiff_dir_entry(dirs, data, tag, type, count, rat,
+            append_tiff_dir_entry(dirs, data, tag, type, count, as_bytes(rat),
                                   offset_correction, 0, endianreq);
             return;
         }
         break;
     case TIFF_SRATIONAL:
         if (element == TypeDesc::FLOAT) {
-            int* rat       = OIIO_ALLOCA(int, 2 * count);
+            auto rat       = OIIO_ALLOCA_SPAN(int, 2 * count);
             const float* f = (const float*)p.data();
             for (size_t i = 0; i < count; ++i)
                 float_to_rational(f[i], rat[2 * i], rat[2 * i + 1]);
-            append_tiff_dir_entry(dirs, data, tag, type, count, rat,
+            append_tiff_dir_entry(dirs, data, tag, type, count, as_bytes(rat),
                                   offset_correction, 0, endianreq);
             return;
         }
@@ -1081,7 +1084,7 @@ pvt::decode_ifd(cspan<uint8_t> buf, size_t ifd_offset, ImageSpec& spec,
 void
 pvt::append_tiff_dir_entry(std::vector<TIFFDirEntry>& dirs,
                            std::vector<char>& data, int tag, TIFFDataType type,
-                           size_t count, const void* mydata,
+                           size_t count, cspan<std::byte> mydata,
                            size_t offset_correction, size_t offset_override,
                            OIIO::endian endianreq)
 {
@@ -1095,16 +1098,19 @@ pvt::append_tiff_dir_entry(std::vector<TIFFDirEntry>& dirs,
     if (len <= 4) {
         dir.tdir_offset = 0;
         data_in_offset  = true;
-        if (mydata) {
+        if (mydata.size()) {
             ptr = (char*)&dir.tdir_offset;
-            memcpy(ptr, mydata, len);
+            memcpy(ptr, mydata.data(), mydata.size());
         }
     } else {
-        if (mydata) {
+        if (mydata.size()) {
             // Add to the data vector and use its offset
             size_t oldsize  = data.size();
             dir.tdir_offset = data.size() - offset_correction;
-            data.insert(data.end(), (char*)mydata, (char*)mydata + len);
+            OIIO_DASSERT(len == mydata.size());
+            data.insert(data.end(),
+                        reinterpret_cast<const char*>(mydata.begin()),
+                        reinterpret_cast<const char*>(mydata.end()));
             ptr = &data[oldsize];
         } else {
             // An offset override was given, use that, it means that data
@@ -1365,22 +1371,22 @@ encode_exif(const ImageSpec& spec, std::vector<char>& blob,
     if (exifdirs.size() || makerdirs.size()) {
         // Add some required Exif tags that wouldn't be in the spec
         append_tiff_dir_entry(exifdirs, blob, EXIF_EXIFVERSION, TIFF_UNDEFINED,
-                              4, "0230", tiffstart, 0, endianreq);
+                              4, as_bytes("0230", 4), tiffstart, 0, endianreq);
         append_tiff_dir_entry(exifdirs, blob, EXIF_FLASHPIXVERSION,
-                              TIFF_UNDEFINED, 4, "0100", tiffstart, 0,
-                              endianreq);
+                              TIFF_UNDEFINED, 4, as_bytes("0100", 4), tiffstart,
+                              0, endianreq);
         static char componentsconfig[] = { 1, 2, 3, 0 };
         append_tiff_dir_entry(exifdirs, blob, EXIF_COMPONENTSCONFIGURATION,
-                              TIFF_UNDEFINED, 4, componentsconfig, tiffstart, 0,
-                              endianreq);
+                              TIFF_UNDEFINED, 4, as_bytes(componentsconfig, 4),
+                              tiffstart, 0, endianreq);
     }
 
     // If any GPS info was found, add a version tag to the GPS fields.
     if (gpsdirs.size()) {
         // Add some required Exif tags that wouldn't be in the spec
         static char ver[] = { 2, 2, 0, 0 };
         append_tiff_dir_entry(gpsdirs, blob, GPSTAG_VERSIONID, TIFF_BYTE, 4,
-                              &ver, tiffstart, 0, endianreq);
+                              as_bytes(ver, 4), tiffstart, 0, endianreq);
     }
 
     // Compute offsets:
@@ -1422,22 +1428,22 @@ encode_exif(const ImageSpec& spec, std::vector<char>& blob,
         OIIO_ASSERT(exifdirs.size());
         // unsigned int size = (unsigned int) makerdirs_offset;
         append_tiff_dir_entry(exifdirs, blob, EXIF_MAKERNOTE, TIFF_BYTE,
-                              makerdirs_size, nullptr, 0, makerdirs_offset,
+                              makerdirs_size, {}, 0, makerdirs_offset,
                               endianreq);
     }
 
     // If any Exif info was found, add a Exif IFD tag to the TIFF dirs
     if (exifdirs.size()) {
         unsigned int size = (unsigned int)exifdirs_offset;
         append_tiff_dir_entry(tiffdirs, blob, TIFFTAG_EXIFIFD, TIFF_LONG, 1,
-                              &size, tiffstart, 0, endianreq);
+                              as_bytes_ref(size), tiffstart, 0, endianreq);
     }
 
     // If any GPS info was found, add a GPS IFD tag to the TIFF dirs
     if (gpsdirs.size()) {
         unsigned int size = (unsigned int)gpsdirs_offset;
         append_tiff_dir_entry(tiffdirs, blob, TIFFTAG_GPSIFD, TIFF_LONG, 1,
-                              &size, tiffstart, 0, endianreq);
+                              as_bytes_ref(size), tiffstart, 0, endianreq);
     }
 
     // All the tag dirs need to be sorted

src/libOpenImageIO/exif.h

@@ -133,7 +133,7 @@ cspan<ExplanationTableEntry> canon_explanation_table ();
 void append_tiff_dir_entry (std::vector<TIFFDirEntry> &dirs,
                             std::vector<char> &data,
                             int tag, TIFFDataType type, size_t count,
-                            const void *mydata, size_t offset_correction,
+                            cspan<std::byte> mydata, size_t offset_correction,
                             size_t offset_override = 0,
                             OIIO::endian endianreq = OIIO::endian::native);