ImageBufImpl doesn't check validity of subimage before checking mips

[meshula]

If I pass a subimage in, greater than or equal to the number of subimages that exist, getting the number of mips will assert.

I have an underlying assumption that if I read in vanilla jpeg file, then the number of subimages is 1, and the valid index is zero. Please let me know if I've got that wrong.

bool
ImageBufImpl::init_spec (string_view filename, int subimage, int miplevel)
{
...
    m_imagecache->get_image_info (m_name, subimage, miplevel, s_subimages,
                                  TypeDesc::TypeInt, &m_nsubimages);

The assert will occur in the next line because ultimately ImageCacheFile::levelInfo will assert that the subimage value is valid.

    m_imagecache->get_image_info (m_name, subimage, miplevel, s_miplevels,
                                  TypeDesc::TypeInt, &m_nmiplevels);

    LevelInfo &levelinfo (int subimage, int miplevel) {
        DASSERT ((int)m_subimages.size() > subimage);
        DASSERT ((int)m_subimages[subimage].levels.size() > miplevel);
        return m_subimages[subimage].levels[miplevel];
    }

Rather than asserting, wouldn't it make sense to range check it before calling get_image_info, and not call the function if out of range? In a release build it'll crash since it results in an out-of-bounds read in levelInfo.

I ran into this because Usd does the following. As you can see it starts off with an out of range query, since the range index starts at 1 instead of 0. (At least according to my base assumption 1 is out of range.)

int
GlfUVTextureData::_GetNumMipLevelsValid(const GlfImageSharedPtr image) const
{
    int potentialMipLevels = image->GetNumMipLevels();

    // Some of our texture loaders will always return an image (even if that
    // mip is not available) so the easiest way to figure out the number of 
    // mip levels is by loading mips and looking at the sizes.
    int previousWidth = image->GetWidth();
    int previousHeight = image->GetHeight();
    
    // Count mips since certain formats will not fail when quering mips,
    // in that case 
    for (int mipCounter = 1; mipCounter < 32; mipCounter++) {
        GlfImageSharedPtr image = GlfImage::OpenForReading(_filePath, mipCounter);
        if (!image) {
            potentialMipLevels = mipCounter;
            break;
        }

The call stack is pretty deep, so I'm not reproducing it here. I'm guessing I'm doing something off the main track from what Pixar is doing, causing me to hit this assertion where they aren't.

Update:
This gets me past the crash, and functioning properly, at least if my assumption is correct.

diff --git a/src/libtexture/imagecache.cpp b/src/libtexture/imagecache.cpp
index d4c6ca6..8b12968 100644
--- a/src/libtexture/imagecache.cpp
+++ b/src/libtexture/imagecache.cpp
@@ -2525,7 +2525,11 @@ ImageCacheImpl::get_image_info (ImageCacheFile *file,
         return true;
     }

-    const ImageSpec &spec (file->spec(subimage,miplevel));
+       if (subimage >= file->subimages()) {
+               return false;
+       }
+
+       const ImageSpec &spec (file->spec(subimage,miplevel));
     if (dataname == s_resolution && datatype==TypeDesc(TypeDesc::INT,2)) {
         int *d = (int *)data;
         d[0] = spec.width;

[lgritz]

Yes, both the subimage and miplevel indexing starts at 0.

I agree, it would be better to fail gracefully than to assert. I'll see if your suggested patch covers all the cases.

[lgritz]

Proposed fix (slightly more comprehensive) in #1672.

[meshula]

Looks good to me :) Thanks!

[lgritz]

Merged into master and RB-1.7.