Skip to content

Commit 8a39d36

Browse files
lgritzlgritz
authored
fix(tiff): Improve TIFF robustness for non-matching tag/metadata types (AcademySoftwareFoundation#5036)
This is a more comprehensive fix for issues discovered in PR AcademySoftwareFoundation#5035. The original problem reported in Issue AcademySoftwareFoundation#5023 was a crash when writing TIFF information that was supposed to be arrays of more than one rational, it was reading past the end of a memory array. AcademySoftwareFoundation#5035 is a minimal, immediate fix to address the crashes. But in the process, I saw a number of ways in which we were dropping metadata on the floor when the types didn't exactly match, but that we *could* handle with automatic conversion. The new cases that we handle with this PR are: * Exif RESOLUTIONUNIT tag is a short, but by convention we store it by the name as a string in OIIO metadata, so we need to convert back to a code (we did so for the main TIFF metadata, but not for Exif in TIFF). * Handle Exif "version" and "flashpixversion" metadata which have unusual encoding in TIFF files (they are 4-character strings, but must be stored in a TIFF tag of type BYTES, not as the usual type ASCII that most strings use. * Handle things that TIFF insists are ASCII but that come to us as metadata that's strings. Easy -- our `ParamValue.get_string()` automatically converts ther things like ints or floats into string representation. * Much more flexibility in automatically converting among the signed and unsigned, 16 and 32 bit, integer types when the metadata in our ImageSpec is integer but not the specific type of integer that TIFF/Exif thinks it should be. This doesn't appear to change the results of anything in our testsuite, but it's possible that some non-TIFF-to-TIFF image conversions that contain Exif data may now do certain type conversions properly instead of just silently dropping the metadata that had non-matching (but reasonably valid) types. Additionally, to do this nicely, I ended up adding a new TypeURational alias in typedesc.h (similar to TypeRational, but the case where both numerator and denominator are unsigned ints). And also fixed a random comment typo I noticed in tiffinput.cpp. --------- Signed-off-by: Larry Gritz <lg@larrygritz.com>
1 parent 27b6799 commit 8a39d36

File tree

4 files changed

+152
-85
lines changed

4 files changed

+152
-85
lines changed

‎src/include/OpenImageIO/typedesc.h‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -385,6 +385,7 @@ inline constexpr TypeDesc TypeHalf (TypeDesc::HALF);
385385
inline constexpr TypeDesc TypeTimeCode (TypeDesc::UINT, TypeDesc::SCALAR, TypeDesc::TIMECODE, 2);
386386
inline constexpr TypeDesc TypeKeyCode (TypeDesc::INT, TypeDesc::SCALAR, TypeDesc::KEYCODE, 7);
387387
inline constexpr TypeDesc TypeRational(TypeDesc::INT, TypeDesc::VEC2, TypeDesc::RATIONAL);
388+
inline constexpr TypeDesc TypeURational(TypeDesc::UINT, TypeDesc::VEC2, TypeDesc::RATIONAL);
388389
inline constexpr TypeDesc TypePointer(TypeDesc::PTR);
389390
inline constexpr TypeDesc TypeUstringhash(TypeDesc::USTRINGHASH);
390391

@@ -648,6 +649,7 @@ using v3_1::TypeHalf;
648649
using v3_1::TypeTimeCode;
649650
using v3_1::TypeKeyCode;
650651
using v3_1::TypeRational;
652+
using v3_1::TypeURational;
651653
using v3_1::TypePointer;
652654
using v3_1::TypeUstringhash;
653655
#endif

‎src/libutil/typedesc.cpp‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -325,6 +325,8 @@ TypeDesc::fromstring(string_view typestring)
325325
t = OIIO::TypeTimeCode;
326326
else if (type == "rational")
327327
t = OIIO::TypeRational;
328+
else if (type == "urational")
329+
t = OIIO::TypeURational;
328330
else if (type == "box2i")
329331
t = OIIO::TypeBox2i;
330332
else if (type == "box3i")
@@ -890,6 +892,12 @@ convert_type(TypeDesc srctype, const void* src, TypeDesc dsttype, void* dst,
890892
((float*)dst)[0] = den ? float(num) / float(den) : 0.0f;
891893
return true;
892894
}
895+
if (dsttype == TypeFloat && srctype == TypeURational) {
896+
auto num = ((const uint32_t*)src)[0];
897+
auto den = ((const uint32_t*)src)[1];
898+
((float*)dst)[0] = den ? float(num) / float(den) : 0.0f;
899+
return true;
900+
}
893901
if (dsttype == TypeFloat && srctype == TypeString) {
894902
// Only succeed for a string if it exactly holds something that
895903
// exactly parses to a float value.

‎src/tiff.imageio/tiffinput.cpp‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -671,7 +671,7 @@ static std::pair<int, const char*> tiff_input_compressions[] = {
671671
{ COMPRESSION_NEXT, "next" }, // NeXT 2-bit RLE
672672
{ COMPRESSION_CCITTRLEW, "ccittrle2" }, // #1 w/ word alignment
673673
{ COMPRESSION_PACKBITS, "packbits" }, // Macintosh RLE
674-
{ COMPRESSION_THUNDERSCAN, "thunderscan" }, // ThundeScan RLE
674+
{ COMPRESSION_THUNDERSCAN, "thunderscan" }, // ThunderScan RLE
675675
{ COMPRESSION_IT8CTPAD, "IT8CTPAD" }, // IT8 CT w/ patting
676676
{ COMPRESSION_IT8LW, "IT8LW" }, // IT8 linework RLE
677677
{ COMPRESSION_IT8MP, "IT8MP" }, // IT8 monochrome picture

‎src/tiff.imageio/tiffoutput.cpp‎

Lines changed: 141 additions & 84 deletions
Original file line numberDiff line numberDiff line change
@@ -149,8 +149,7 @@ class TIFFOutput final : public ImageOutput {
149149
void fix_bitdepth(void* data, int nvals);
150150

151151
// Add a parameter to the output
152-
bool put_parameter(const std::string& name, TypeDesc type,
153-
const void* data);
152+
bool put_parameter(const ParamValue& metadata);
154153
bool write_exif_data();
155154

156155
// Make our best guess about whether the spec is describing data that
@@ -921,10 +920,8 @@ TIFFOutput::open(const std::string& name, const ImageSpec& userspec,
921920
}
922921

923922
// Deal with all other params
924-
for (size_t p = 0; p < m_spec.extra_attribs.size(); ++p)
925-
put_parameter(m_spec.extra_attribs[p].name().string(),
926-
m_spec.extra_attribs[p].type(),
927-
m_spec.extra_attribs[p].data());
923+
for (const auto& p : m_spec.extra_attribs)
924+
put_parameter(p);
928925

929926
if (m_spec.get_int_attribute("tiff:write_iptc")) {
930927
// Enable IPTC block writing only if "tiff_write_iptc" hint is explicitly
@@ -956,120 +953,125 @@ TIFFOutput::open(const std::string& name, const ImageSpec& userspec,
956953

957954

958955

956+
inline int
957+
resunit_to_code(string_view s)
958+
{
959+
if (Strutil::iequals(s, "none"))
960+
return RESUNIT_NONE;
961+
else if (Strutil::iequals(s, "in") || Strutil::iequals(s, "inch"))
962+
return RESUNIT_INCH;
963+
else if (Strutil::iequals(s, "cm"))
964+
return RESUNIT_CENTIMETER;
965+
return 0;
966+
}
967+
968+
969+
959970
bool
960-
TIFFOutput::put_parameter(const std::string& name, TypeDesc type,
961-
const void* data)
971+
TIFFOutput::put_parameter(const ParamValue& param)
962972
{
963-
if (!data || (type == TypeString && *(char**)data == nullptr)) {
973+
ustring name = param.uname();
974+
TypeDesc type = param.type();
975+
if (!param.data()
976+
|| (type == TypeString && *(char**)param.data() == nullptr)) {
964977
// we got a null pointer, don't set the field
965978
return false;
966979
}
967-
if (Strutil::iequals(name, "Artist") && type == TypeDesc::STRING) {
968-
TIFFSetField(m_tif, TIFFTAG_ARTIST, *(char**)data);
980+
if (Strutil::iequals(name, "Artist")) {
981+
TIFFSetField(m_tif, TIFFTAG_ARTIST, param.get_string().c_str());
969982
return true;
970983
}
971-
if (Strutil::iequals(name, "Copyright") && type == TypeDesc::STRING) {
972-
TIFFSetField(m_tif, TIFFTAG_COPYRIGHT, *(char**)data);
984+
if (Strutil::iequals(name, "Copyright")) {
985+
TIFFSetField(m_tif, TIFFTAG_COPYRIGHT, param.get_string().c_str());
973986
return true;
974987
}
975-
if (Strutil::iequals(name, "DateTime") && type == TypeDesc::STRING) {
976-
TIFFSetField(m_tif, TIFFTAG_DATETIME, *(char**)data);
988+
if (Strutil::iequals(name, "DateTime") && type == TypeString) {
989+
TIFFSetField(m_tif, TIFFTAG_DATETIME, param.get_string().c_str());
977990
return true;
978991
}
979-
if ((Strutil::iequals(name, "name")
980-
|| Strutil::iequals(name, "DocumentName"))
981-
&& type == TypeDesc::STRING) {
982-
TIFFSetField(m_tif, TIFFTAG_DOCUMENTNAME, *(char**)data);
992+
if (Strutil::iequals(name, "name")
993+
|| Strutil::iequals(name, "DocumentName")) {
994+
TIFFSetField(m_tif, TIFFTAG_DOCUMENTNAME, param.get_string().c_str());
983995
return true;
984996
}
985-
if (Strutil::iequals(name, "fovcot") && type == TypeDesc::FLOAT) {
986-
double d = *(float*)data;
987-
TIFFSetField(m_tif, TIFFTAG_PIXAR_FOVCOT, d);
997+
if (Strutil::iequals(name, "fovcot") && type == TypeFloat) {
998+
TIFFSetField(m_tif, TIFFTAG_PIXAR_FOVCOT, param.get_float());
988999
return true;
9891000
}
990-
if ((Strutil::iequals(name, "host")
991-
|| Strutil::iequals(name, "HostComputer"))
992-
&& type == TypeDesc::STRING) {
993-
TIFFSetField(m_tif, TIFFTAG_HOSTCOMPUTER, *(char**)data);
1001+
if (Strutil::iequals(name, "host")
1002+
|| Strutil::iequals(name, "HostComputer")) {
1003+
TIFFSetField(m_tif, TIFFTAG_HOSTCOMPUTER, param.get_string().c_str());
9941004
return true;
9951005
}
9961006
if ((Strutil::iequals(name, "description")
9971007
|| Strutil::iequals(name, "ImageDescription"))
998-
&& type == TypeDesc::STRING) {
999-
TIFFSetField(m_tif, TIFFTAG_IMAGEDESCRIPTION, *(char**)data);
1008+
&& type == TypeString) {
1009+
TIFFSetField(m_tif, TIFFTAG_IMAGEDESCRIPTION,
1010+
param.get_string().c_str());
10001011
return true;
10011012
}
1002-
if (Strutil::iequals(name, "tiff:Predictor") && type == TypeDesc::INT) {
1003-
m_predictor = *(int*)data;
1013+
if (Strutil::iequals(name, "tiff:Predictor")) {
1014+
m_predictor = param.get_int();
10041015
TIFFSetField(m_tif, TIFFTAG_PREDICTOR, m_predictor);
10051016
return true;
10061017
}
1007-
if (Strutil::iequals(name, "ResolutionUnit") && type == TypeDesc::STRING) {
1008-
const char* s = *(char**)data;
1009-
bool ok = true;
1010-
if (Strutil::iequals(s, "none"))
1011-
TIFFSetField(m_tif, TIFFTAG_RESOLUTIONUNIT, RESUNIT_NONE);
1012-
else if (Strutil::iequals(s, "in") || Strutil::iequals(s, "inch"))
1013-
TIFFSetField(m_tif, TIFFTAG_RESOLUTIONUNIT, RESUNIT_INCH);
1014-
else if (Strutil::iequals(s, "cm"))
1015-
TIFFSetField(m_tif, TIFFTAG_RESOLUTIONUNIT, RESUNIT_CENTIMETER);
1016-
else
1017-
ok = false;
1018-
return ok;
1018+
if (Strutil::iequals(name, "ResolutionUnit") && type == TypeString) {
1019+
if (int r = resunit_to_code(param.get_string())) {
1020+
TIFFSetField(m_tif, TIFFTAG_RESOLUTIONUNIT, r);
1021+
return true;
1022+
}
1023+
return false;
10191024
}
10201025
if (Strutil::iequals(name, "tiff:RowsPerStrip")
10211026
&& !m_spec.tile_width /* don't set rps for tiled files */
10221027
&& m_planarconfig == PLANARCONFIG_CONTIG /* only for contig */) {
1023-
if (type == TypeDesc::INT) {
1024-
m_rowsperstrip = *(int*)data;
1025-
} else if (type == TypeDesc::STRING) {
1026-
// Back-compatibility with Entropy and PRMan
1027-
m_rowsperstrip = Strutil::stoi(*(char**)data);
1028-
} else {
1028+
int rps = param.get_int();
1029+
if (rps <= 0)
10291030
return false;
1030-
}
1031-
m_rowsperstrip = clamp(m_rowsperstrip, 1, m_spec.height);
1031+
m_rowsperstrip = clamp(rps, 1, m_spec.height);
10321032
TIFFSetField(m_tif, TIFFTAG_ROWSPERSTRIP, m_rowsperstrip);
10331033
return true;
10341034
}
1035-
if (Strutil::iequals(name, "Make") && type == TypeDesc::STRING) {
1036-
TIFFSetField(m_tif, TIFFTAG_MAKE, *(char**)data);
1035+
if (Strutil::iequals(name, "Make")) {
1036+
TIFFSetField(m_tif, TIFFTAG_MAKE, param.get_string().c_str());
10371037
return true;
10381038
}
1039-
if (Strutil::iequals(name, "Model") && type == TypeDesc::STRING) {
1040-
TIFFSetField(m_tif, TIFFTAG_MODEL, *(char**)data);
1039+
if (Strutil::iequals(name, "Model")) {
1040+
TIFFSetField(m_tif, TIFFTAG_MODEL, param.get_string().c_str());
10411041
return true;
10421042
}
1043-
if (Strutil::iequals(name, "Software") && type == TypeDesc::STRING) {
1044-
TIFFSetField(m_tif, TIFFTAG_SOFTWARE, *(char**)data);
1043+
if (Strutil::iequals(name, "Software")) {
1044+
TIFFSetField(m_tif, TIFFTAG_SOFTWARE, param.get_string().c_str());
10451045
return true;
10461046
}
1047-
if (Strutil::iequals(name, "tiff:SubFileType") && type == TypeDesc::INT) {
1048-
TIFFSetField(m_tif, TIFFTAG_SUBFILETYPE, *(int*)data);
1047+
if (Strutil::iequals(name, "tiff:SubFileType")) {
1048+
TIFFSetField(m_tif, TIFFTAG_SUBFILETYPE, param.get_int());
10491049
return true;
10501050
}
1051-
if (Strutil::iequals(name, "textureformat") && type == TypeDesc::STRING) {
1052-
TIFFSetField(m_tif, TIFFTAG_PIXAR_TEXTUREFORMAT, *(char**)data);
1051+
if (Strutil::iequals(name, "textureformat")) {
1052+
TIFFSetField(m_tif, TIFFTAG_PIXAR_TEXTUREFORMAT,
1053+
param.get_string().c_str());
10531054
return true;
10541055
}
1055-
if (Strutil::iequals(name, "wrapmodes") && type == TypeDesc::STRING) {
1056-
TIFFSetField(m_tif, TIFFTAG_PIXAR_WRAPMODES, *(char**)data);
1056+
if (Strutil::iequals(name, "wrapmodes")) {
1057+
TIFFSetField(m_tif, TIFFTAG_PIXAR_WRAPMODES,
1058+
param.get_string().c_str());
10571059
return true;
10581060
}
10591061
if (Strutil::iequals(name, "worldtocamera") && type == TypeMatrix) {
1060-
TIFFSetField(m_tif, TIFFTAG_PIXAR_MATRIX_WORLDTOCAMERA, data);
1062+
TIFFSetField(m_tif, TIFFTAG_PIXAR_MATRIX_WORLDTOCAMERA, param.data());
10611063
return true;
10621064
}
10631065
if (Strutil::iequals(name, "worldtoscreen") && type == TypeMatrix) {
1064-
TIFFSetField(m_tif, TIFFTAG_PIXAR_MATRIX_WORLDTOSCREEN, data);
1066+
TIFFSetField(m_tif, TIFFTAG_PIXAR_MATRIX_WORLDTOSCREEN, param.data());
10651067
return true;
10661068
}
1067-
if (Strutil::iequals(name, "XResolution") && type == TypeDesc::FLOAT) {
1068-
TIFFSetField(m_tif, TIFFTAG_XRESOLUTION, *(float*)data);
1069+
if (Strutil::iequals(name, "XResolution")) {
1070+
TIFFSetField(m_tif, TIFFTAG_XRESOLUTION, param.get_float());
10691071
return true;
10701072
}
1071-
if (Strutil::iequals(name, "YResolution") && type == TypeDesc::FLOAT) {
1072-
TIFFSetField(m_tif, TIFFTAG_YRESOLUTION, *(float*)data);
1073+
if (Strutil::iequals(name, "YResolution")) {
1074+
TIFFSetField(m_tif, TIFFTAG_YRESOLUTION, param.get_float());
10731075
return true;
10741076
}
10751077
return false;
@@ -1134,27 +1136,82 @@ TIFFOutput::write_exif_data()
11341136
int tag, tifftype, count;
11351137
if (exif_tag_lookup(p.name(), tag, tifftype, count)
11361138
&& tifftype != TIFF_NOTYPE) {
1139+
bool ok = false;
1140+
bool handled = false;
1141+
// Some special cases first
11371142
if (tag == EXIF_SECURITYCLASSIFICATION || tag == EXIF_IMAGEHISTORY
1138-
|| tag == EXIF_PHOTOGRAPHICSENSITIVITY)
1143+
|| tag == EXIF_PHOTOGRAPHICSENSITIVITY) {
11391144
continue; // libtiff doesn't understand these
1140-
bool ok = false;
1141-
if (tifftype == TIFF_ASCII) {
1142-
ok = TIFFSetField(m_tif, tag, *(char**)p.data());
1143-
} else if ((tifftype == TIFF_SHORT || tifftype == TIFF_LONG)
1144-
&& p.type() == TypeDesc::SHORT && count == 1) {
1145-
ok = TIFFSetField(m_tif, tag, (int)*(short*)p.data());
1146-
} else if ((tifftype == TIFF_SHORT || tifftype == TIFF_LONG)
1147-
&& p.type() == TypeDesc::INT && count == 1) {
1148-
ok = TIFFSetField(m_tif, tag, *(int*)p.data());
1149-
} else if ((tifftype == TIFF_RATIONAL || tifftype == TIFF_SRATIONAL)
1150-
&& p.type() == TypeDesc::FLOAT && count == 1) {
1151-
ok = TIFFSetField(m_tif, tag, *(float*)p.data());
1145+
}
1146+
if (tag == TIFFTAG_RESOLUTIONUNIT && p.type() == TypeString) {
1147+
// OIIO stores resolution unit as a string, but libtiff wants
1148+
// it as a short code, so we have to convert.
1149+
if (int r = resunit_to_code(p.get_string())) {
1150+
ok = TIFFSetField(m_tif, TIFFTAG_RESOLUTIONUNIT, r);
1151+
}
1152+
handled = true;
1153+
} else if (tag == EXIF_EXIFVERSION || tag == EXIF_FLASHPIXVERSION) {
1154+
if (p.type() == TypeString) {
1155+
// These tags are a 4-byte array of chars, but we
1156+
// allow users to set it as a string. Convert it if needed.
1157+
std::string version = p.get_string();
1158+
if (version.size() >= 4) {
1159+
ok = TIFFSetField(m_tif, tag, version.c_str());
1160+
}
1161+
handled = true;
1162+
} else if (p.type() == TypeInt) {
1163+
std::string s = Strutil::fmt::format("{:04}", p.get_int());
1164+
if (s.size() == 4)
1165+
ok = TIFFSetField(m_tif, tag, s.c_str());
1166+
handled = true;
1167+
}
1168+
}
1169+
// General cases...
1170+
else if (tifftype == TIFF_ASCII) {
1171+
ok = TIFFSetField(m_tif, tag, p.get_string().c_str());
1172+
handled = true;
1173+
} else if (tifftype == TIFF_SHORT || tifftype == TIFF_SSHORT
1174+
|| tifftype == TIFF_LONG || tifftype == TIFF_SLONG) {
1175+
if ((p.type() == TypeInt16 || p.type() == TypeInt32
1176+
|| p.type() == TypeUInt16 || p.type() == TypeUInt32)
1177+
&& count == 1) {
1178+
// Passing our kinda-int as TIFF kinda-int
1179+
ok = TIFFSetField(m_tif, tag, p.get_int());
1180+
handled = true;
1181+
} else if (p.type() == TypeString && count == 1) {
1182+
// Passing our string as TIFF kinda-int -- convert as long
1183+
// as the string looks like an int.
1184+
std::string s = p.get_string();
1185+
if (Strutil::string_is_int(s)) {
1186+
int val = Strutil::stoi(s);
1187+
ok = TIFFSetField(m_tif, tag, val);
1188+
handled = true;
1189+
}
1190+
}
11521191
} else if ((tifftype == TIFF_RATIONAL || tifftype == TIFF_SRATIONAL)
1153-
&& p.type() == TypeDesc::DOUBLE && count == 1) {
1154-
ok = TIFFSetField(m_tif, tag, *(double*)p.data());
1192+
&& (p.type() == TypeFloat || p.type() == TypeDesc::DOUBLE
1193+
|| p.type() == TypeUInt16 || p.type() == TypeUInt32
1194+
|| p.type() == TypeInt16 || p.type() == TypeInt32
1195+
|| p.type() == TypeRational
1196+
|| p.type() == TypeURational)
1197+
&& count == 1) {
1198+
// If the tag is a rational, there are a number of types we
1199+
// can force into that form by converting to and then passing
1200+
// a float.
1201+
ok = TIFFSetField(m_tif, tag, p.get_float());
1202+
handled = true;
1203+
}
1204+
if (!handled) {
1205+
# ifndef NDEBUG
1206+
print("Unhandled EXIF {} ({}) / tag {} tifftype {} count {}\n",
1207+
p.name(), p.type(), tag, tifftype, count);
1208+
# endif
11551209
}
1210+
// NOTE: We are not handling arrays of values, just scalars.
11561211
if (!ok) {
1157-
// std::cout << "Unhandled EXIF " << p.name() << " " << p.type() << "\n";
1212+
// print(
1213+
// "Error handling EXIF {} ({}) / tag {} tifftype {} count {}\n",
1214+
// p.name(), p.type(), tag, tifftype, count);
11581215
}
11591216
}
11601217
}

0 commit comments

Comments
 (0)