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

Assert at app exit when opening corrupted image #1817

Closed
zavinator opened this issue Nov 29, 2017 · 11 comments
Closed

Assert at app exit when opening corrupted image #1817

zavinator opened this issue Nov 29, 2017 · 11 comments

Comments

@zavinator
Copy link

zavinator commented Nov 29, 2017

Problem

This simple program shows assert at exit:

#include <OpenImageIO/imageio.h>
int main()
{
OIIO::ImageInput *in = OIIO::ImageInput::open("obrtest.png");
//_fcloseall();
return 0;
}

obrtest.png:

obrtest

The problem is probably related to Windows - Visual Studio - zlib.
If the file is invalid, then all plugins are tried (including zfile plugin).
It seems that there can be a problem in zfile.cpp with closing file handles in zlib and msvc.

The included program shows me in debug mode an assert at app exit (or calling _fcloseall manually):
Expression: (_osfile(fh) & FOPEN)

And it also crashes in release mode on some machines.

When I exclude the zfile plugin from the OIIO lib, it works - but there could be a better solution...

Versions

  • OIIO branch/version: 1.8.5 (static lib), zlib (static) - all compiled with /MD (/MDd in debug)
  • OS: Windows 10 x64
  • C++ compiler: Visual Studio 2017 x64
@zavinator zavinator changed the title Crash at app exit when opening corrupted PNG Crash at app exit when opening corrupted image Nov 29, 2017
@zavinator zavinator changed the title Crash at app exit when opening corrupted image Assert at app exit when opening corrupted image Nov 30, 2017
@zavinator zavinator reopened this Nov 30, 2017
@lgritz
Copy link
Collaborator

lgritz commented Feb 12, 2018

I think this was fixed with #1839. Can you confirm?

@zavinator
Copy link
Author

It works (1.8.9)

@zavinator
Copy link
Author

zavinator commented Apr 6, 2018

Just tested it in debug mode and it still shows the assert :(

@zavinator zavinator reopened this Apr 6, 2018
@zavinator
Copy link
Author

zavinator commented Nov 8, 2018

J've just tested 1.8.16 and it is still crashing :(

As a temporary workaround, I've put "return false" to
ZfileInput::valid_file (const std::string &filename) const
ZfileInput::open (const std::string &name, ImageSpec &newspec)
(I don't need a Z file format...)

@lgritz
Copy link
Collaborator

lgritz commented Nov 8, 2018

Which assert is it hitting? Is it an OIIO assert?

What source file and line does the assert implicate?

@zavinator
Copy link
Author

I've debug the issue and it seems that there could be a problem in zlib <-> MSVC.

FILE *f = fopen("obrtest.png", "rb");
gzFile gz = gzdopen(fileno(f), "rb");
gzclose(gz);
_fcloseall(); // this shows the assert:
/*
File: minkernel\crts\ucrt\src\appcrt\lowio\close.cpp
Line: 49

Expression: (_osfile(fh) & FOPEN)
*/

The _fcloseall func is called automatically when the app exits.

This works ok (without an assert):

FILE *f = fopen("obrtest.png", "rb");
gzFile gz = gzdopen(fileno(f), "rb");
fclose(f);
_fcloseall();

@lgritz
Copy link
Collaborator

lgritz commented Nov 15, 2018

Ah, I see. We are using a different open/close call sequence for open() versus valid_file().

@lgritz
Copy link
Collaborator

lgritz commented Nov 15, 2018

We could switch to the other sequence -- fopen(filename), then gzdopen(fileno(f)), for the valid_file (versus doing a direct gzopen(filename). And see if that helps?

@lgritz
Copy link
Collaborator

lgritz commented Nov 16, 2018

Do you want me to take a stab at a patch to address this?

@zavinator
Copy link
Author

I think that closing the file with gzclose should be valid - according to zlib docs, but it is not working in MSVC (?).
Using directly gzopen(filename)/gzclose should be working. Currently I don't need a patch, but it could be fixed in the next release. Of course if you provide a patch, I can test it for you...
Thanks

lgritz added a commit to lgritz/OpenImageIO that referenced this issue Nov 16, 2018
Spurious problems on Windows seem to happen with gzopen(filename) as we
used in ZfileInput::valid_file, but not with the gzdopen(fileno(fd)) as
we used in ZfileInput::open(). So change valid_file to the method that
seems to work. This has been linked to crashes during app shutdown on
Windows (even when no zfiles are encountered, since it can happen with
invalid files when all input readers are tried).

Also add a zfile testsuite entry.

Fixes AcademySoftwareFoundation#1817
@lgritz
Copy link
Collaborator

lgritz commented Nov 16, 2018

Proposed fix in #2070.

@riviera-kid, I've never seen the crash, so I'm going to need to rely on you to try it out on your end with the debug build and see if it appears to clear up the problem.

lgritz added a commit to lgritz/OpenImageIO that referenced this issue Nov 19, 2018
Spurious problems on Windows seem to happen with gzopen(filename) as we
used in ZfileInput::valid_file, but not with the gzdopen(fileno(fd)) as
we used in ZfileInput::open(). So change valid_file to the method that
seems to work. This has been linked to crashes during app shutdown on
Windows (even when no zfiles are encountered, since it can happen with
invalid files when all input readers are tried).

Also add a zfile testsuite entry.

Fixes AcademySoftwareFoundation#1817
lgritz added a commit to lgritz/OpenImageIO that referenced this issue Nov 19, 2018
Spurious problems on Windows seem to happen with gzopen(filename) as we
used in ZfileInput::valid_file, but not with the gzdopen(fileno(fd)) as
we used in ZfileInput::open(). So change valid_file to the method that
seems to work. This has been linked to crashes during app shutdown on
Windows (even when no zfiles are encountered, since it can happen with
invalid files when all input readers are tried).

Also add a zfile testsuite entry.

Fixes AcademySoftwareFoundation#1817
lgritz added a commit that referenced this issue Nov 20, 2018
Spurious problems on Windows seem to happen with improperly closed or leaking file handles.
Solved by using gzopen_w on Windows to handle UTF-8 filenames allows us to remove the odd idiom of opening with fopen() then passing fileno to gzdopen (we were botching exactly who was responsible for duplicating or closing those handles).

Also add a zfile testsuite entry.

Fixes #1817
lgritz added a commit to lgritz/OpenImageIO that referenced this issue Nov 20, 2018
Spurious problems on Windows seem to happen with improperly closed or leaking file handles.
Solved by using gzopen_w on Windows to handle UTF-8 filenames allows us to remove the odd idiom of opening with fopen() then passing fileno to gzdopen (we were botching exactly who was responsible for duplicating or closing those handles).

Also add a zfile testsuite entry.

Fixes AcademySoftwareFoundation#1817
lgritz added a commit to lgritz/OpenImageIO that referenced this issue Nov 20, 2018
Spurious problems on Windows seem to happen with improperly closed or leaking file handles.
Solved by using gzopen_w on Windows to handle UTF-8 filenames allows us to remove the odd idiom of opening with fopen() then passing fileno to gzdopen (we were botching exactly who was responsible for duplicating or closing those handles).

Also add a zfile testsuite entry.

Fixes AcademySoftwareFoundation#1817
lgritz added a commit to lgritz/OpenImageIO that referenced this issue Nov 20, 2018
Spurious problems on Windows seem to happen with improperly closed or leaking file handles.
Solved by using gzopen_w on Windows to handle UTF-8 filenames allows us to remove the odd idiom of opening with fopen() then passing fileno to gzdopen (we were botching exactly who was responsible for duplicating or closing those handles).

Also add a zfile testsuite entry.

Fixes AcademySoftwareFoundation#1817
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

2 participants