Skip to content

Commit 25ed31f

Browse files
lgritzlgritz
committed
Better error checking when requesting nonexistant subimages/mips
Found a bunch of places (ImageBuf, ImageInput, openexr and tiff) where ImageInput::spec() and spec_dimensions() were returning false but not printing a specific error message. Fix that up in most places (but not seek_subimage, at least not yet, because it can make things rather noisy for people who are counting on it returning false to detect the end of subimages/miplevels but not clearing the error messages). Signed-off-by: Larry Gritz <[email protected]>
1 parent 318232b commit 25ed31f

File tree

11 files changed

+191
-35
lines changed

11 files changed

+191
-35
lines changed

‎src/libOpenImageIO/imagebuf.cpp

Lines changed: 32 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1088,15 +1088,29 @@ ImageBufImpl::init_spec(string_view filename, int subimage, int miplevel,
10881088
// If no configspec, just do a regular soft invalidate
10891089
invalidate(m_name, false);
10901090
}
1091-
m_imagecache->get_image_info(m_name, subimage, miplevel, s_subimages,
1092-
TypeInt, &m_nsubimages);
1093-
m_imagecache->get_image_info(m_name, subimage, miplevel, s_miplevels,
1094-
TypeInt, &m_nmiplevels);
10951091
const char* fmt = NULL;
1096-
m_imagecache->get_image_info(m_name, subimage, miplevel, s_fileformat,
1097-
TypeString, &fmt);
1092+
if (!(m_imagecache->get_image_info(m_name, subimage, miplevel,
1093+
s_subimages, TypeInt, &m_nsubimages)
1094+
&& m_imagecache->get_image_info(m_name, subimage, miplevel,
1095+
s_miplevels, TypeInt,
1096+
&m_nmiplevels)
1097+
&& m_imagecache->get_image_info(m_name, subimage, miplevel,
1098+
s_fileformat, TypeString, &fmt))) {
1099+
error(m_imagecache->geterror());
1100+
return false;
1101+
}
10981102
m_fileformat = ustring(fmt);
10991103

1104+
if (subimage < 0 || subimage >= m_nsubimages) {
1105+
error("Invalid subimage {} requested (of {} subimages)", subimage,
1106+
m_nsubimages);
1107+
}
1108+
if (miplevel < 0 || miplevel >= m_nmiplevels) {
1109+
error(
1110+
"Invalid MIP level {} requested for subimage {} (of {} levels)",
1111+
miplevel, subimage, m_nmiplevels);
1112+
}
1113+
11001114
m_imagecache->get_imagespec(m_name, m_nativespec, subimage);
11011115
m_spec = m_nativespec;
11021116
m_imagecache->get_cache_dimensions(m_name, m_spec, subimage, miplevel);
@@ -1169,6 +1183,18 @@ ImageBufImpl::init_spec(string_view filename, int subimage, int miplevel,
11691183
atomic_fetch_add(pvt::IB_total_open_time, float(timer()));
11701184
return false;
11711185
}
1186+
if (subimage > 0 && !input->supports("multiimage")) {
1187+
error(
1188+
"Invalid subimage {} requested ({} does not support subimages)",
1189+
subimage, input->format_name());
1190+
return false;
1191+
}
1192+
if (miplevel > 0 && !input->supports("mipmap")) {
1193+
error(
1194+
"Invalid miplevel {} requested ({} does not support miplevels)",
1195+
miplevel, input->format_name());
1196+
return false;
1197+
}
11721198
m_spec = input->spec(subimage, miplevel);
11731199
if (input->has_error()) {
11741200
m_err = input->geterror();

‎src/openexr.imageio/exrinput.cpp

Lines changed: 37 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -999,8 +999,11 @@ OpenEXRInput::PartInfo::compute_mipres(int miplevel, ImageSpec& spec) const
999999
bool
10001000
OpenEXRInput::seek_subimage(int subimage, int miplevel)
10011001
{
1002-
if (subimage < 0 || subimage >= m_nsubimages) // out of range
1002+
if (subimage < 0 || subimage >= m_nsubimages) { // out of range
1003+
// errorfmt("Could not seek to subimage={}: not in file", subimage,
1004+
// miplevel);
10031005
return false;
1006+
}
10041007

10051008
if (subimage == m_subimage && miplevel == m_miplevel) { // no change
10061009
return true;
@@ -1011,8 +1014,11 @@ OpenEXRInput::seek_subimage(int subimage, int miplevel)
10111014
const Imf::Header* header = NULL;
10121015
if (m_input_multipart)
10131016
header = &(m_input_multipart->header(subimage));
1014-
if (!part.parse_header(this, header))
1017+
if (!part.parse_header(this, header)) {
1018+
errorfmt("Could not seek to subimage={}: unable to parse header",
1019+
subimage, miplevel);
10151020
return false;
1021+
}
10161022
part.initialized = true;
10171023
}
10181024

@@ -1074,8 +1080,11 @@ OpenEXRInput::seek_subimage(int subimage, int miplevel)
10741080

10751081
m_subimage = subimage;
10761082

1077-
if (miplevel < 0 || miplevel >= part.nmiplevels) // out of range
1083+
if (miplevel < 0 || miplevel >= part.nmiplevels) { // out of range
1084+
// errorfmt("Could not seek to subimage={} miplevel={}: not in file",
1085+
// subimage, miplevel);
10781086
return false;
1087+
}
10791088

10801089
m_miplevel = miplevel;
10811090
m_spec = part.spec;
@@ -1105,20 +1114,30 @@ ImageSpec
11051114
OpenEXRInput::spec(int subimage, int miplevel)
11061115
{
11071116
ImageSpec ret;
1108-
if (subimage < 0 || subimage >= m_nsubimages)
1117+
if (subimage < 0 || subimage >= m_nsubimages) {
1118+
errorfmt("Could not seek to subimage={} miplevel={}: not in file",
1119+
subimage, miplevel);
11091120
return ret; // invalid
1121+
}
11101122
const PartInfo& part(m_parts[subimage]);
11111123
if (!part.initialized) {
11121124
// Only if this subimage hasn't yet been inventoried do we need
11131125
// to lock and seek.
11141126
lock_guard lock(*this);
11151127
if (!part.initialized) {
1116-
if (!seek_subimage(subimage, miplevel))
1128+
if (!seek_subimage(subimage, miplevel)) {
1129+
errorfmt(
1130+
"Could not seek to subimage={} miplevel={}: not in file",
1131+
subimage, miplevel);
11171132
return ret;
1133+
}
11181134
}
11191135
}
1120-
if (miplevel < 0 || miplevel >= part.nmiplevels)
1136+
if (miplevel < 0 || miplevel >= part.nmiplevels) {
1137+
errorfmt("Could not seek to subimage={} miplevel={}: not in file",
1138+
subimage, miplevel);
11211139
return ret; // invalid
1140+
}
11221141
ret = part.spec;
11231142
part.compute_mipres(miplevel, ret);
11241143
return ret;
@@ -1130,18 +1149,27 @@ ImageSpec
11301149
OpenEXRInput::spec_dimensions(int subimage, int miplevel)
11311150
{
11321151
ImageSpec ret;
1133-
if (subimage < 0 || subimage >= m_nsubimages)
1152+
if (subimage < 0 || subimage >= m_nsubimages) {
1153+
errorfmt("Could not seek to subimage={} miplevel={}: not in file",
1154+
subimage, miplevel);
11341155
return ret; // invalid
1156+
}
11351157
const PartInfo& part(m_parts[subimage]);
11361158
if (!part.initialized) {
11371159
// Only if this subimage hasn't yet been inventoried do we need
11381160
// to lock and seek.
11391161
lock_guard lock(*this);
1140-
if (!seek_subimage(subimage, miplevel))
1162+
if (!seek_subimage(subimage, miplevel)) {
1163+
errorfmt("Could not seek to subimage={} miplevel={}: not in file",
1164+
subimage, miplevel);
11411165
return ret;
1166+
}
11421167
}
1143-
if (miplevel < 0 || miplevel >= part.nmiplevels)
1168+
if (miplevel < 0 || miplevel >= part.nmiplevels) {
1169+
errorfmt("Could not seek to subimage={} miplevel={}: not in file",
1170+
subimage, miplevel);
11441171
return ret; // invalid
1172+
}
11451173
ret.copy_dimensions(part.spec);
11461174
part.compute_mipres(miplevel, ret);
11471175
return ret;

‎src/openexr.imageio/exrinput_c.cpp

Lines changed: 50 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -477,11 +477,15 @@ OpenEXRCoreInput::PartInfo::parse_header(OpenEXRCoreInput* in,
477477
spec = ImageSpec();
478478

479479
exr_result_t rv = exr_get_data_window(ctxt, subimage, &top_datawindow);
480-
if (rv != EXR_ERR_SUCCESS)
480+
if (rv != EXR_ERR_SUCCESS) {
481+
in->errorfmt("exr_get_data_window failed");
481482
return false;
483+
}
482484
rv = exr_get_display_window(ctxt, subimage, &top_displaywindow);
483-
if (rv != EXR_ERR_SUCCESS)
485+
if (rv != EXR_ERR_SUCCESS) {
486+
in->errorfmt("exr_get_display_window failed");
484487
return false;
488+
}
485489
spec.x = top_datawindow.min.x;
486490
spec.y = top_datawindow.min.y;
487491
spec.z = 0;
@@ -500,8 +504,10 @@ OpenEXRCoreInput::PartInfo::parse_header(OpenEXRCoreInput* in,
500504

501505
exr_storage_t storage;
502506
rv = exr_get_storage(ctxt, subimage, &storage);
503-
if (rv != EXR_ERR_SUCCESS)
507+
if (rv != EXR_ERR_SUCCESS) {
508+
in->errorfmt("exr_get_storage failed");
504509
return false;
510+
}
505511
uint32_t txsz, tysz;
506512
if ((storage == EXR_STORAGE_TILED || storage == EXR_STORAGE_DEEP_TILED)
507513
&& EXR_ERR_SUCCESS
@@ -512,8 +518,10 @@ OpenEXRCoreInput::PartInfo::parse_header(OpenEXRCoreInput* in,
512518

513519
int32_t levelsx, levelsy;
514520
rv = exr_get_tile_levels(ctxt, subimage, &levelsx, &levelsy);
515-
if (rv != EXR_ERR_SUCCESS)
521+
if (rv != EXR_ERR_SUCCESS) {
522+
in->errorfmt("exr_get_tile_levels failed");
516523
return false;
524+
}
517525
nmiplevels = std::max(levelsx, levelsy);
518526
} else {
519527
spec.tile_width = 0;
@@ -904,8 +912,10 @@ OpenEXRCoreInput::PartInfo::query_channels(OpenEXRCoreInput* in,
904912
spec.nchannels = 0;
905913
const exr_attr_chlist_t* chlist;
906914
exr_result_t rv = exr_get_channels(ctxt, subimage, &chlist);
907-
if (rv != EXR_ERR_SUCCESS)
915+
if (rv != EXR_ERR_SUCCESS) {
916+
in->errorfmt("exr_get_channels failed");
908917
return false;
918+
}
909919

910920
std::vector<CChanNameHolder> cnh;
911921
int c = 0;
@@ -1048,20 +1058,29 @@ OpenEXRCoreInput::PartInfo::compute_mipres(int miplevel, ImageSpec& spec) const
10481058
bool
10491059
OpenEXRCoreInput::seek_subimage(int subimage, int miplevel)
10501060
{
1051-
if (subimage < 0 || subimage >= m_nsubimages) // out of range
1061+
if (subimage < 0 || subimage >= m_nsubimages) { // out of range
1062+
// errorfmt("Could not seek to subimage={}: not in file", subimage,
1063+
// miplevel);
10521064
return false;
1065+
}
10531066

10541067
PartInfo& part(m_parts[subimage]);
10551068
if (!part.initialized) {
1056-
if (!part.parse_header(this, m_exr_context, subimage, miplevel))
1069+
if (!part.parse_header(this, m_exr_context, subimage, miplevel)) {
1070+
errorfmt("Could not seek to subimage={}: unable to parse header",
1071+
subimage, miplevel);
10571072
return false;
1073+
}
10581074
part.initialized = true;
10591075
}
10601076

10611077
m_subimage = subimage;
10621078

1063-
if (miplevel < 0 || miplevel >= part.nmiplevels) // out of range
1079+
if (miplevel < 0 || miplevel >= part.nmiplevels) { // out of range
1080+
// errorfmt("Could not seek to subimage={} miplevel={}: not in file",
1081+
// subimage, miplevel);
10641082
return false;
1083+
}
10651084

10661085
m_miplevel = miplevel;
10671086
m_spec = part.spec;
@@ -1088,20 +1107,30 @@ ImageSpec
10881107
OpenEXRCoreInput::spec(int subimage, int miplevel)
10891108
{
10901109
ImageSpec ret;
1091-
if (subimage < 0 || subimage >= m_nsubimages)
1110+
if (subimage < 0 || subimage >= m_nsubimages) {
1111+
errorfmt("Could not seek to subimage={} miplevel={}: not in file",
1112+
subimage, miplevel);
10921113
return ret; // invalid
1114+
}
10931115
const PartInfo& part(m_parts[subimage]);
10941116
if (!part.initialized) {
10951117
// Only if this subimage hasn't yet been inventoried do we need
10961118
// to lock and seek.
10971119
lock_guard lock(*this);
10981120
if (!part.initialized) {
1099-
if (!seek_subimage(subimage, miplevel))
1121+
if (!seek_subimage(subimage, miplevel)) {
1122+
errorfmt(
1123+
"Could not seek to subimage={} miplevel={}: not in file",
1124+
subimage, miplevel);
11001125
return ret;
1126+
}
11011127
}
11021128
}
1103-
if (miplevel < 0 || miplevel >= part.nmiplevels)
1129+
if (miplevel < 0 || miplevel >= part.nmiplevels) {
1130+
errorfmt("Could not seek to subimage={} miplevel={}: not in file",
1131+
subimage, miplevel);
11041132
return ret; // invalid
1133+
}
11051134
ret = part.spec;
11061135
part.compute_mipres(miplevel, ret);
11071136
return ret;
@@ -1120,11 +1149,17 @@ OpenEXRCoreInput::spec_dimensions(int subimage, int miplevel)
11201149
// Only if this subimage hasn't yet been inventoried do we need
11211150
// to lock and seek.
11221151
lock_guard lock(*this);
1123-
if (!seek_subimage(subimage, miplevel))
1152+
if (!seek_subimage(subimage, miplevel)) {
1153+
errorfmt("Could not seek to subimage={} miplevel={}: not in file",
1154+
subimage, miplevel);
11241155
return ret;
1156+
}
11251157
}
1126-
if (miplevel < 0 || miplevel >= part.nmiplevels)
1158+
if (miplevel < 0 || miplevel >= part.nmiplevels) {
1159+
errorfmt("Could not seek to subimage={} miplevel={}: not in file",
1160+
subimage, miplevel);
11271161
return ret; // invalid
1162+
}
11281163
ret.copy_dimensions(part.spec);
11291164
part.compute_mipres(miplevel, ret);
11301165
return ret;
@@ -1434,7 +1469,7 @@ OpenEXRCoreInput::read_native_tiles(int subimage, int miplevel, int xbegin,
14341469
int zend, void* data)
14351470
{
14361471
if (!m_exr_context) {
1437-
errorfmt("called OpenEXRInput::read_native_tile without an open file");
1472+
errorfmt("called OpenEXRInput::read_native_tiles without an open file");
14381473
return false;
14391474
}
14401475

@@ -1453,7 +1488,7 @@ OpenEXRCoreInput::read_native_tiles(int subimage, int miplevel, int xbegin,
14531488
void* data)
14541489
{
14551490
if (!m_exr_context) {
1456-
errorfmt("called OpenEXRInput::read_native_tile without an open file");
1491+
errorfmt("called OpenEXRInput::read_native_tiles without an open file");
14571492
return false;
14581493
}
14591494

‎src/tiff.imageio/tiffinput.cpp

Lines changed: 18 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -775,17 +775,26 @@ TIFFInput::open(const std::string& name, ImageSpec& newspec,
775775
bool
776776
TIFFInput::seek_subimage(int subimage, int miplevel)
777777
{
778-
if (subimage < 0) // Illegal
778+
if (subimage < 0 || miplevel < 0) { // Illegal
779+
errorfmt("Could not seek to subimage={} miplevel={}: invalid", subimage,
780+
miplevel);
779781
return false;
782+
}
780783
if (m_emulate_mipmap) {
781784
// Emulating MIPmap? Pretend one subimage, many MIP levels.
782-
if (subimage != 0)
785+
if (subimage != 0) {
786+
errorfmt("Could not seek to subimage={} miplevel={}: not in file",
787+
subimage, miplevel);
783788
return false;
789+
}
784790
subimage = miplevel;
785791
} else {
786792
// No MIPmap emulation
787-
if (miplevel != 0)
793+
if (miplevel != 0) {
794+
errorfmt("Could not seek to subimage={} miplevel={}: not in file",
795+
subimage, miplevel);
788796
return false;
797+
}
789798
}
790799

791800
if (subimage == m_subimage) {
@@ -881,7 +890,9 @@ TIFFInput::seek_subimage(int subimage, int miplevel)
881890
return true;
882891
} else {
883892
std::string e = oiio_tiff_last_error();
884-
errorfmt("Err: {}", e.length() ? e : m_filename);
893+
errorfmt("could not seek to {} {}",
894+
m_emulate_mipmap ? "miplevel" : "subimage", subimage,
895+
e.length() ? ": " : "", e.length() ? e : std::string(""));
885896
m_subimage = -1;
886897
return false;
887898
}
@@ -932,8 +943,10 @@ TIFFInput::spec_dimensions(int subimage, int miplevel)
932943
if (m_emulate_mipmap) {
933944
// This is the kind of TIFF file where we are emulating MIPmap
934945
// levels with TIFF subimages.
935-
if (subimage != 0)
946+
if (subimage != 0) {
947+
errorfmt("Requst for subimage {} invalid", subimage);
936948
return ret; // Invalid subimage request, return the empty spec
949+
}
937950
// Index into the spec list by miplevel instead, because that's
938951
// what it really contains.
939952
s = miplevel;

‎testsuite/jpeg2000/ref/out.txt

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,3 +4,5 @@ Comparing "../oiio-images/jpeg2000/broken/issue_3427.jp2" and "issue_3427.jp2"
44
idiff ERROR: Could not read ../oiio-images/jpeg2000/broken/issue_3427.jp2:
55
Invalid image file "../oiio-images/jpeg2000/broken/issue_3427.jp2": Tile part length size inconsistent with stream length
66
Failed to decode the codestream in the JP2 file
7+
Invalid image file "../oiio-images/jpeg2000/broken/issue_3427.jp2": Tile part length size inconsistent with stream length
8+
Failed to decode the codestream in the JP2 file

‎testsuite/png-damaged/ref/out.txt

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,3 +9,5 @@ libpng error: tEXt: Read error: hit end of file in png reader
99
idiff ERROR: Could not read invalid_gray_alpha_sbit.png:
1010
Invalid image file "invalid_gray_alpha_sbit.png": Read error: hit end of file in png reader
1111
PNG read error: tEXt: Read error: hit end of file in png reader
12+
Invalid image file "invalid_gray_alpha_sbit.png": Read error: hit end of file in png reader
13+
PNG read error: tEXt: Read error: hit end of file in png reader

‎testsuite/python-imagebuf/ref/out-alt-python3.txt

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -234,6 +234,14 @@ Testing metadata copying
234234
camera:x = "Bx"
235235
camera:y = "By"
236236

237+
Testing error handling for out-of-range subimage, miplevel
238+
bayer.png subimage 1 mip 0: Invalid subimage 1 requested (png does not support subimages)
239+
bayer.png subimage 0 mip 0:
240+
bayer.png subimage 0 mip 1: Invalid miplevel 1 requested (png does not support miplevels)
241+
grid-small.exr subimage 1 mip 0: Could not seek to subimage=1 miplevel=0: not in file
242+
grid-small.exr subimage 0 mip 0:
243+
grid-small.exr subimage 0 mip 1: Could not seek to subimage=0 miplevel=1: not in file
244+
237245
Done.
238246
Comparing "out.tif" and "ref/out.tif"
239247
PASS

0 commit comments

Comments
 (0)