Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BUG] TextureSystem: sample with gray_to_rgb possible crash if there are filesystem errors #4544

Open
aras-p opened this issue Nov 29, 2024 · 0 comments

Comments

@aras-p
Copy link
Contributor

aras-p commented Nov 29, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant