OpenImageIO
OpenImageIO copied to clipboard
AcademySoftwareFoundation
DDS file lock not released when compression type is not supported
Problem
DDS file lock is not released when compression type is not supported. The DDSInput::open function opens the file (m_file = Filesystem::fopen(name, "rb");) and the early returns do not close it. This results in a locked file.
Expected behavior: The file lock is released correctly.
Actual behavior: The file lock is not released.
Steps to Reproduce
- Load a .dds file with an unsupported compression type
Versions
- OIIO branch/version: master
- OS: Windows
- C++ compiler: msvc
Quite right! I will post a patch right away. Good catch, thanks for bringing this to our attention.
Thanks a lot for the quick feedback and fix!! Did you also check for the same kind of error on the other file formats?
Yes, indeed there are a couple more spots. I will amend the PR.
Please hold any reviews of this, there is more coming.
As an aside, even though I'm finding some places where an error inside of open() doesn't immediately close the open file handle, so far it seems that all of the ImageInput subclasses will automatically close any open handles when the ImageInput is destroyed. Our presumption all along was that if open fails, the caller will very soon end up deleting the II, which also does the close. So although I agree that it's better if open() does the close right away, the only way that it's a problem is that if you open() the image input, it fails, but then you just keep holding onto the II handle for an extended period of time.
Actually from what I saw the issue was the following: ImageInput::valid_file(const std::string& filename) const { ImageSpec tmpspec; bool ok = const_cast<ImageInput*>(this)->open(filename, tmpspec); if (ok) const_cast<ImageInput*>(this)->close(); return ok; } was called, the file was opened there and not closed because ok == false. Then open was called again and the first file pointer stored in m_file was lost forever.
The file was then not released even after deleting the ImageInput.
I think the fixes I already made will fix that case.
But perhaps it's also best if that code closes the file unconditionally. I may need to do some additional testing to be sure that every file format's close() function is safe to call in this way.
Yes I think that your fix will also fix what I described. It makes sense to close the file unconditionally there (if it is safe to do so), but probably it would be more robust on "open" to check if m_file == NULL... to avoid overwriting the pointer and losing it forever.