BUG: PNG IO fails on corrupt file

[issakomi]

S. explanation and link to sample file in the post #2928 (comment)

Also this post is related #2933 (comment)

[jhlegarreta]

💯 To this effort @issakomi.

Cross-referencing comments
#2933 (comment)
#2933 (comment)
#2933 (comment)

so that corrupted test files are added, and the degenerate situations they trigger are tested.

[issakomi]

Cross-referencing comments

Not sure what i could answer here, here are no new "warning" trigger points, no changes in debug output, etc.
Just fixed setjmp wrapping and added one jump point in ReadImageInformation to avoid crash with corrupted example file.

I am afraid i have no more time, already spent 4 full days with bugs in JPEG and PNG IOs.
Consider this as suggestion and feel free to do with the PR whatever you find correct, close, accept, modify. No problem.

cc @Hconk

[jhlegarreta]

Just fixed setjmp wrapping and added one jump point in ReadImageInformation to avoid crash with corrupted example file.

I am afraid i have no more time, already spent 4 full days with bugs in JPEG and PNG IOs.
Consider this as suggestion and feel free to do with the PR whatever you find correct, close, accept, modify. No problem.

@issakomi I understand. Thanks for the effort. Can at least the corrupted, readable and corrupted, unreadable images used to test this be shared?

[Hconk]

Just fixed setjmp wrapping and added one jump point in ReadImageInformation to avoid crash with corrupted example file.

I am afraid i have no more time, already spent 4 full days with bugs in JPEG and PNG IOs.
Consider this as suggestion and feel free to do with the PR whatever you find correct, close, accept, modify. No problem.

@issakomi I understand. Thanks for the effort. Can at least the corrupted, readable and corrupted, unreadable images used to test this be shared?

You can get corrupted png image from here.
#2928 (comment)

[jhlegarreta]

You can get corrupted png image from here.
#2928 (comment)

Thanks @Hconk. I will add it to the testing suite as time permits.

[dzenanz]

LGTM. I guess we should wait until test(s) are added before merging.

[dzenanz]

Looks good to me. Why not add it as the second commit in your branch pngio? Then we could merge it, without waiting for Jon's follow-up with improvements.

[dzenanz]

Data addition document is not long. Web interface is the most convenient for uploading a small number of files.

[dzenanz]

There is also a visual guide.

[issakomi]

I have used itkPNGImageIOTest3.cxx, it is already for corrupted files with ITK_TRY_EXPECT_EXCEPTION.

P.S. BTW, interesting, this old test file triggers jump at the only one point where setjmp was not ruined with wrapping.

[issakomi]

BTW, the fix is minimal. There are more question.
In Read function not 100% sure about std::unique_ptr, how the auto-deletion works during jump, it is not volatile (BTW, the setjmp point after that std::unique_ptr is not changed, so the PR doesn't change this behavior, just FYI). This pointer was a usual pointer in the past, than was turned into std::unique_ptr not so long ago. Probably there was a memory leak due to setjmp unlucky issues.
Do we want PNG print output or not? The empty ITK custom function (to suppress debug from the library) is set at the very end of the story. There is a lot of PNG debug output in tests log. It is not changed by the PR too.

[dzenanz]

Keeping the current situation regarding the PNG error and warning messages is fine. I don't have a preference for making it more consistent by more or less messages.