fix(ImageInput): Don't leave a rejected subimage as the current one

src/dds.imageio/ddsinput.cpp

@@ -82,6 +82,10 @@ class DDSInput final : public ImageInput {
         ioproxy_clear();
     }
 
+    /// Helper function: parse a miplevel's header into m_spec.
+    ///
+    bool read_miplevel_spec(int miplevel);
+
     /// Helper function: read the image as scanlines (all but cubemaps).
     ///
     bool readimg_scanlines();
@@ -776,6 +780,31 @@ DDSInput::seek_subimage(int subimage, int miplevel)
     // clear buffer so that readimage is called
     m_buf.clear();
 
+    // We're about to overwrite m_spec for the requested level, so give up the
+    // current one first -- failing partway then leaves us on no level, rather
+    // than on one whose spec describes a different level.
+    m_subimage = -1;
+    m_miplevel = -1;
+    m_spec     = ImageSpec();
+
+    if (!read_miplevel_spec(miplevel)) {
+        // Nor keep the spec of a level we refused: spec() would report it,
+        // and read_scanline() would size its buffer from it.
+        m_spec = ImageSpec();
+        return false;
+    }
+
+    m_subimage = subimage;
+    m_miplevel = miplevel;
+    return true;
+}
+
+
+// Parse the requested miplevel's header into m_spec and check it against
+// the limits. On failure m_spec is left partly filled; the caller clears it.
+bool
+DDSInput::read_miplevel_spec(int miplevel)
+{
     // for cube maps, the seek will be performed when reading a tile instead
     size_t w = 0, h = 0, d = 0;
     TypeDesc::BASETYPE basetype = GetBaseType(m_compression);
@@ -962,8 +991,6 @@ DDSInput::seek_subimage(int subimage, int miplevel)
     if (!check_compression_ratio(m_spec, ioproxy()->size()))
         return false;
 
-    m_subimage = subimage;
-    m_miplevel = miplevel;
     return true;
 }
 

src/dpx.imageio/dpxinput.cpp

@@ -185,12 +185,12 @@ DPXInput::seek_subimage(int subimage, int miplevel)
 {
     if (miplevel != 0)
         return false;
-    if (subimage == m_subimage)
-        return true;
+    // Range check before the "already there" early out, so that the -1 we
+    // leave behind on a rejected seek can't satisfy it.
     if (subimage < 0 || subimage >= m_dpx.header.ImageElementCount())
         return false;
-
-    m_subimage = subimage;
+    if (subimage == m_subimage)
+        return true;
 
     // create imagespec
     TypeDesc typedesc;
@@ -211,12 +211,19 @@ DPXInput::seek_subimage(int subimage, int miplevel)
     case dpx::kDouble: typedesc = TypeDesc::DOUBLE; break;
     default: errorfmt("Invalid component data size"); return false;
     }
-    m_spec = ImageSpec(m_dpx.header.Width(), m_dpx.header.Height(),
-                       m_dpx.header.ImageElementComponentCount(subimage),
-                       typedesc);
+    // We're about to overwrite m_spec, so give up the current element first.
+    // Every failure below then leaves us on none, rather than on one whose
+    // spec describes a different element -- the early out above would hand
+    // that spec back on a repeat of this same seek.
+    m_subimage = -1;
+    m_spec     = ImageSpec(m_dpx.header.Width(), m_dpx.header.Height(),
+                           m_dpx.header.ImageElementComponentCount(subimage),
+                           typedesc);
     if (!check_open(m_spec, { 0, 1 << 30, 0, 1 << 30, 0, 1 << 16, 0, 8 })
-        || !check_compression_ratio(m_spec, m_filesize))
+        || !check_compression_ratio(m_spec, m_filesize)) {
+        m_spec = ImageSpec();
         return false;
+    }
 
     // xOffset/yOffset are defined as unsigned 32-bit integers, but m_spec.x/y are signed
     // avoid casts that would result in negative values
@@ -562,6 +569,7 @@ DPXInput::seek_subimage(int subimage, int miplevel)
             errorfmt(
                 "Corrupt userbuf: {} bytes of user data at offset {} run past the {} byte file",
                 m_dpx.header.UserSize(), userdata_offset, m_filesize);
+            m_spec = ImageSpec();
             return false;
         }
         m_userBuf.resize(m_dpx.header.UserSize());
@@ -570,6 +578,7 @@ DPXInput::seek_subimage(int subimage, int miplevel)
             // about to be handed out as metadata. Don't let that escape.
             errorfmt("Corrupt userbuf: could not read {} bytes of user data",
                      m_dpx.header.UserSize());
+            m_spec = ImageSpec();
             return false;
         }
     }
@@ -583,6 +592,7 @@ DPXInput::seek_subimage(int subimage, int miplevel)
     if (m_spec.nchannels == 1)
         m_rawcolor = true;
 
+    m_subimage = subimage;
     return true;
 }
 

src/fits.imageio/fitsinput.cpp

@@ -203,16 +203,24 @@ FitsInput::seek_subimage(int subimage, int miplevel)
         return true;
     }
 
-    // setting file pointer to the beginning of IMAGE extension
-    m_cur_subimage = subimage;
-    if (Filesystem::fseek(m_fd, m_subimages[m_cur_subimage].offset, SEEK_SET)) {
+    // We're about to overwrite m_spec and the file position, so give up the
+    // current subimage first. Failing partway then leaves us on none, rather
+    // than on one whose spec was never validated.
+    m_cur_subimage = -1;
+    m_spec         = ImageSpec();
+    if (Filesystem::fseek(m_fd, m_subimages[subimage].offset, SEEK_SET)) {
         errorfmt("Seek error");
         return false;
     }
 
-    if (!set_spec_info())
+    if (!set_spec_info()) {
+        // Nor keep the spec of an HDU we refused: spec() would report it,
+        // and read_scanline() would size its buffer from it.
+        m_spec = ImageSpec();
         return false;
+    }
 
+    m_cur_subimage = subimage;
     return true;
 }
 

src/gif.imageio/gifinput.cpp

@@ -73,7 +73,11 @@ class GIFInput final : public ImageInput {
     std::string m_filename;          ///< Stash the filename
     GifFileType* m_gif_file;         ///< GIFLIB handle
     int m_transparent_color;         ///< Transparent color index
-    int m_subimage;                  ///< Current subimage index
+    int m_subimage;                  ///< Current subimage index, or -1 if we
+                                     ///  aren't positioned on a valid one.
+    int m_stream_subimage;           ///< Subimage the giflib stream is next
+                                     ///  positioned to read, or -1 if the
+                                     ///  position is unknown.
     int m_disposal_method;           ///< Disposal method of current subimage.
                                      ///  Indicates what to do with canvas
                                      ///  before drawing the _next_ subimage.
@@ -92,9 +96,14 @@ class GIFInput final : public ImageInput {
     ///
     bool read_subimage_metadata(ImageSpec& newspec);
 
-    /// Read current subimage data (ie. draw it on canvas).
-    ///
-    bool read_subimage_data(void);
+    // Read the header of the subimage the stream is positioned at: its
+    // metadata, the canvas dimensions, and the validity checks.
+    bool read_subimage_header(ImageSpec& newspec);
+
+    // Read the data of the subimage the stream is positioned at (ie. draw it
+    // on canvas). `subimage` is its index, which says whether it starts from
+    // a blank canvas.
+    bool read_subimage_data(int subimage);
 
     /// Helper: read gif extension.
     ///
@@ -150,7 +159,9 @@ OIIO_PLUGIN_EXPORTS_END
 void
 GIFInput::init(void)
 {
-    m_gif_file = nullptr;
+    m_gif_file        = nullptr;
+    m_subimage        = -1;
+    m_stream_subimage = -1;
     ioproxy_clear();
 }
 
@@ -159,8 +170,9 @@ GIFInput::init(void)
 bool
 GIFInput::open(const std::string& name, ImageSpec& newspec)
 {
-    m_filename = name;
-    m_subimage = -1;
+    m_filename        = name;
+    m_subimage        = -1;
+    m_stream_subimage = -1;
     m_canvas.clear();
 
     if (seek_subimage(0, 0)) {
@@ -338,7 +350,28 @@ GIFInput::read_subimage_metadata(ImageSpec& newspec)
 
 
 bool
-GIFInput::read_subimage_data()
+GIFInput::read_subimage_header(ImageSpec& newspec)
+{
+    if (!read_subimage_metadata(newspec))
+        return false;
+
+    // Subimages are drawn on a shared canvas, so they all have the dimensions
+    // the file declares for that canvas.
+    newspec.width       = m_gif_file->SWidth;
+    newspec.height      = m_gif_file->SHeight;
+    newspec.depth       = 1;
+    newspec.full_width  = newspec.width;
+    newspec.full_height = newspec.height;
+    newspec.full_depth  = newspec.depth;
+
+    return check_open(newspec, { 0, 32768, 0, 32768, 0, 1, 0, 4 })
+           && check_compression_ratio(newspec, ioproxy()->size());
+}
+
+
+
+bool
+GIFInput::read_subimage_data(int subimage)
 {
     GifColorType* colormap = NULL;
     int colormap_count;
@@ -353,7 +386,7 @@ GIFInput::read_subimage_data()
         return false;
     }
 
-    if (m_subimage == 0 || m_previous_disposal_method == DISPOSE_BACKGROUND) {
+    if (subimage == 0 || m_previous_disposal_method == DISPOSE_BACKGROUND) {
         // make whole canvas transparent
         size_t canvas_pixels = m_spec.image_pixels();
         if (canvas_pixels > std::numeric_limits<int>::max() / size_t(4)) {
@@ -425,9 +458,18 @@ GIFInput::seek_subimage(int subimage, int miplevel)
         return true;
     }
 
-    if (m_subimage > subimage) {
-        // requested subimage is located before the current one
-        // file needs to be reopened
+    // We're about to overwrite m_spec and the canvas, and possibly to close
+    // the file, so give up the current subimage before anything below can
+    // fail. Every failure then leaves us on no subimage, rather than on one
+    // whose spec and canvas no longer describe what the reader holds.
+    m_subimage = -1;
+    m_spec     = ImageSpec();
+
+    if (m_stream_subimage < 0 || m_stream_subimage > subimage) {
+        // The requested subimage is behind the stream position, or we don't
+        // know where the stream is. giflib only reads forward, so the file
+        // needs to be reopened.
+        m_stream_subimage = -1;
         if (m_gif_file && !close()) {
             return false;
         }
@@ -442,42 +484,40 @@ GIFInput::seek_subimage(int subimage, int miplevel)
             errorfmt("{}", GifErrorString(giflib_error));
             return false;
         }
-        m_subimage = -1;
+        m_stream_subimage = 0;
     }
 
-    // skip subimages preceding the requested one
-    if (m_subimage < subimage) {
-        for (m_subimage += 1; m_subimage < subimage; m_subimage++) {
-            if (!read_subimage_metadata(m_spec) || !read_subimage_data()) {
-                return false;
-            }
+    // Reading forward through the stream from here invalidates the position
+    // we recorded, so forget it too: a failure partway then makes the next
+    // seek reopen the file rather than trust a half-read stream.
+    int pos           = m_stream_subimage;
+    m_stream_subimage = -1;
+
+    // skip subimages preceding the requested one. Every failure from here
+    // on clears m_spec: read_subimage_header() parses into it before the
+    // limit checks, so a refused frame must not be left for spec() to
+    // report or for read_scanline() to size its buffer from.
+    for (; pos < subimage; ++pos) {
+        if (!read_subimage_header(m_spec) || !read_subimage_data(pos)) {
+            m_spec = ImageSpec();
+            return false;
         }
     }
 
-    // read metadata of current subimage
-    if (!read_subimage_metadata(m_spec)) {
+    if (!read_subimage_header(m_spec)) {
+        m_spec = ImageSpec();
         return false;
     }
 
-    m_spec.width       = m_gif_file->SWidth;
-    m_spec.height      = m_gif_file->SHeight;
-    m_spec.depth       = 1;
-    m_spec.full_height = m_spec.height;
-    m_spec.full_width  = m_spec.width;
-    m_spec.full_depth  = m_spec.depth;
-
-    if (!check_open(m_spec, { 0, 32768, 0, 32768, 0, 1, 0, 4 })
-        || !check_compression_ratio(m_spec, ioproxy()->size())) {
-        return false;
-    }
-
-    m_subimage = subimage;
-
     // draw subimage on canvas
-    if (!read_subimage_data()) {
+    if (!read_subimage_data(subimage)) {
+        m_spec = ImageSpec();
         return false;
     }
 
+    m_subimage        = subimage;
+    m_stream_subimage = subimage + 1;
+
     return true;
 }