AcademySoftwareFoundation / AcademySoftwareFoundation/OpenImageIO
[BUG] TextureSystem: sample with gray_to_rgb possible crash if there are filesystem errors
- Dominant language
- C++
- Stars
- 2.4k
- Forks
- 698
- Avg merge
- 3d 4h
- Merged PRs (30d)
- 47
Description
**Describe the bug**
Been playing around with TextureSystem, and (for other reasons that I'm looking into) there have been spurious filesystem errors (due to too many open files - without setting `max_open_files_strict` in my use case the actual opened file count went way over the `max_open_files` of 100, and up to 510, at which point Windows MSVC runtime says "nope" for further file opens).
But, once that happens, the `ImageCacheFile::mark_broken` is called, which clears `m_subimages`. This invalidates the spec reference that the texture sampling function has locally.
Here's the problematic place:
- `TextureSystemImpl::texture` does `texturefile = verify_texturefile` (fine), and then
- `const ImageSpec& spec(texturefile->spec(options.subimage, 0))` -- note that this is a *reference*
- Later on, for an UDIM texture, the sampling callstack can end up being:
```
TextureSystemImpl::sample_bilinear
ImageCacheImpl::find_tile
ImageCacheImpl::find_tile_main_cache
ImageCacheImpl::add_tile_to_cache # needs to read a different udim tile file!
ImageCacheTile::read
ImageCacheFile::read_tile
ImageCacheFile::open # if this fails for whatever reason (my case: too many open files),
ImageCacheFile::mark_broken # this invalidates the ImageSpec fetched above
```
- Now, right before the filtering lookup if `fill_gray_channels` is called, this will read `spec` that points to free'd memory.
**OpenImageIO version and dependencies**
2.5.11.0, but looking at file history of latest 3.0.0 the problem seems to exist there too.
**To Reproduce**
Reproducing is a bit involved since it needs `ImageCacheFile::open` to be failing. Whether that is due to too many open files (my case), or network/disk connectivity, etc. And it needs to be failing whereas the parent file check earlier in the texture function succeeds; which I think can only happen with UDIM textures.
I could "easily" reproduce this by using the Animal Logic ALab 2.2 data set, which is all UDIM exr files, on Windows MSVC with default open file limit of 100 (OIIO starts to use up to 500 open files, at which point this blows up). Setting `max_open_files_strict` makes this problem go away.
**Evidence**
I think the callstack and code flow outline above should point out the possible issue. Probably could be fixed by changing `fill_gray_channels` to not use the reference of the `ImageSpec`, but rather directly get the `int nchannels` and `int alpha_channel`, and the calling code reading those right after it gets the spec reference, so that they are not invalidated.
Contributor guide
Research direction
Start at TextureSystemImpl::texture and trace the ImageSpec reference through sample_bilinear, ImageCacheImpl::find_tile, and ImageCacheFile::mark_broken. Reproduce the UDIM path with a filesystem-open failure, then verify that gray-channel filling no longer accesses invalidated data and that normal texture sampling remains unchanged.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- cpp
- Domain
- computer-graphics
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Stale
- Clarity
- Mostly clear
- Newbie friendliness
- 38/100