BUG: Allow nifti load to fail #3529

hjmjohnson · 2022-08-04T18:12:49Z

Ensure that attempts to write to invalid file paths causes error to be reported. Ref #1958.

PR Checklist

No API changes were made (or the changes have been approved)
No major design changes were made (or the changes have been approved)
Added test (or behavior not changed)

Modules/IO/ImageBase/include/itkIOTestHelper.h

When attempting to write to an invalid file location, ensure that an itk::ExceptionObject is thrown.

When attempting to write a nifti file to an invalid location (for example, C:\Windows\something.nii) then no file is created there yet no error is reported. The issue is that the error is swallowed in both niftilib and nifti2 (just "reported" with LNI_FERR macro, which does not throw an exception or reports the error to ITK in any other way): Resolves: InsightSoftwareConsortium#1958

Ensure that caught exceptions are converted to itk::Exception objects.

When re-using the HDF IO object, all internal state variables should be reset to the initial state as if the object had been constructed anew. Previously, the WriteImageInformation function would set the value m_ImageInformationWritten to true, and it would never again be set to false.

The HDF5ImageIO objects are single use for writing. After the initial use, the WriteImageInformation function is short circuited, and can not be reset for writing a second image. This bug was exposed while fixing a problem with the not throwing an exception when attempting to write to a path that does not exist. This temporary work around in the test framework allows fixing the initial problem without requiring fixing the HDF5ImageIO re-use issues. NOTE: Resetting the HDF5ImageIO object at the end of writing fixes the re-use problem for non-streaming case, but that causes the streaming tests to fail.

hjmjohnson · 2022-08-07T23:57:36Z

@dzenanz I tried to use ITK macros for the excepion handling in the tests, but did not have time to figure out why the builds failed due to missing header files.

jhlegarreta · 2022-08-10T16:04:16Z

Not sure whether this caused the memory leaks that appeared today:
https://open.cdash.org/viewDynamicAnalysis.php?buildid=8091249

The ones for itkMeshTest and itkFreeSurferMeshIOTestBinary were already there, but not the NIfTI-related ones. Not sure if the MINCImageIO one is related to this either.

jhlegarreta · 2022-08-12T00:05:32Z

Not sure whether this caused the memory leaks that appeared today:
https://open.cdash.org/viewDynamicAnalysis.php?buildid=8091249

The ones for itkMeshTest and itkFreeSurferMeshIOTestBinary were already there, but not the NIfTI-related ones. Not sure if the MINCImageIO one is related to this either.

Fix proposed in PR #3545.

jhlegarreta · 2022-08-13T13:25:51Z

The MINC-related Valgrind defects persist:
https://open.cdash.org/viewDynamicAnalysis.php?buildid=8097352

Detail
https://open.cdash.org/viewDynamicAnalysisFile.php?id=9661400

Looks like the changes to the itkIOTestHelper.h file in https://github.com/InsightSoftwareConsortium/ITK/pull/3529/files#diff-0e320d0a08552a8dd7eea3ec3fe6ee5f2f6f91208ce9509f51a7a2383fdd34ef could be the source.

The most prior recent changes to MINC classes
d90e635#diff-5df59ff7e425b43eb59c43686c0a58f24c35cb222328b5f332b30565ccb4d45f

did not incur Valgrind defects:
https://open.cdash.org/viewDynamicAnalysis.php?buildid=8085647

Have not investigated further.

issakomi · 2022-08-13T23:47:17Z

The problems are in MINC IO. The changes in this PR just made the problems visible.

There are several leaks if MINC IO exits with exception, it doesn't delete many allocations.

The easiest one is e.g. at line 1166
if (minew_volume_props(&hprops) < 0)

That hprops is freed at the end of WriteImageInformation(), line 1300
mifree_volume_props(hprops);
but there are many exceptions between the lines and hprops is not freed. If mifree_volume_props(hprops); is called before every exception -- this will reduce the number of memory leaks, related to minew_volume_props like this one

==15585== 64 bytes in 1 blocks are definitely lost in loss record 374 of 486
==15585==    at 0x483B7F3: malloc (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so)
==15585==    by 0x679D69B: minew_volume_props (volprops.c:45)

But there are many more, unfortunately.

I have done trivial test, without using itkIOTestHelper.h, just read MHA and write MNC to wrong path, the leaks are there. If the path is correct -- there are no problems, so this PR has done a good job and discovered ancient problems, IMHO.

MINC IO often did not free allocations before thowing an exception, detected by InsightSoftwareConsortium#3529

issakomi · 2022-08-14T02:59:56Z

This seems to work
master...issakomi:ITK:minc_valgr1
it is rather tricky with micreate_dimension, after some point (volume is created) it will be freed by CloseVolume() (in destructor), but before it has to be freed with mifree_dimension_handle.

MINC IO often did not free allocations before thowing an exception, detected by InsightSoftwareConsortium#3529

MINC IO often did not free allocations before throwing an exception, detected by InsightSoftwareConsortium#3529

MINC IO doesn't always free allocations before throwing an exception, detected by InsightSoftwareConsortium#3529.

MINC IO doesn't always free allocations before throwing an exception, detected by #3529.

MINC IO doesn't always free allocations before throwing an exception, detected by InsightSoftwareConsortium#3529.

github-actions bot added area:IO Issues affecting the IO module type:Bug Inconsistencies or issues which will cause an incorrect result under some or all circumstances type:Testing Ensure that the purpose of a class is met/the results on a wide set of test cases are correct labels Aug 4, 2022

dzenanz approved these changes Aug 4, 2022

View reviewed changes

Modules/IO/ImageBase/include/itkIOTestHelper.h Show resolved Hide resolved

Modules/IO/ImageBase/include/itkIOTestHelper.h Show resolved Hide resolved

hjmjohnson force-pushed the allow-nifti-load-to-fail branch from 35f2d5b to 7d14b45 Compare August 5, 2022 18:55

hjmjohnson and others added 4 commits August 6, 2022 08:29

ENH: Force test failure on bad file writing

a8cc0a5

When attempting to write to an invalid file location, ensure that an itk::ExceptionObject is thrown.

ENH: Isolate img that is being corrupted.

3dfab27

ENH: Convert unspecified exceptions to itk

62e307a

Ensure that caught exceptions are converted to itk::Exception objects.

hjmjohnson force-pushed the allow-nifti-load-to-fail branch 3 times, most recently from 9bcfb58 to 5e6824f Compare August 7, 2022 14:52

hjmjohnson added 3 commits August 7, 2022 12:00

STYLE: Fix test debugging statement format.

6fc1c88

hjmjohnson force-pushed the allow-nifti-load-to-fail branch from 5e6824f to f244a81 Compare August 7, 2022 17:00

dzenanz approved these changes Aug 8, 2022

View reviewed changes

hjmjohnson merged commit 2ddf427 into InsightSoftwareConsortium:master Aug 8, 2022

issakomi added a commit to issakomi/ITK that referenced this pull request Aug 14, 2022

BUG: fixed memory leaks in MINC IO

80ece1b

MINC IO often did not free allocations before thowing an exception, detected by InsightSoftwareConsortium#3529

issakomi added a commit to issakomi/ITK that referenced this pull request Aug 14, 2022

BUG: fixed memory leaks in MINC IO

53a69d0

MINC IO often did not free allocations before thowing an exception, detected by InsightSoftwareConsortium#3529

issakomi added a commit to issakomi/ITK that referenced this pull request Aug 14, 2022

BUG: fixed memory leaks in MINC IO

4aaf4a6

MINC IO often did not free allocations before throwing an exception, detected by InsightSoftwareConsortium#3529

issakomi mentioned this pull request Aug 14, 2022

BUG: fixed memory leaks in MINC IO #3548

Merged

issakomi added a commit to issakomi/ITK that referenced this pull request Aug 14, 2022

BUG: fixed memory leaks in MINC IO

bc0be93

MINC IO doesn't always free allocations before throwing an exception, detected by InsightSoftwareConsortium#3529.

issakomi added a commit to issakomi/ITK that referenced this pull request Aug 14, 2022

BUG: fixed memory leaks in MINC IO

54b017d

MINC IO doesn't always free allocations before throwing an exception, detected by InsightSoftwareConsortium#3529.

dzenanz pushed a commit that referenced this pull request Aug 15, 2022

BUG: fixed memory leaks in MINC IO

c5bc6a6

MINC IO doesn't always free allocations before throwing an exception, detected by #3529.

hjmjohnson pushed a commit to hjmjohnson/ITK that referenced this pull request Aug 26, 2022

BUG: fixed memory leaks in MINC IO

bb88862

MINC IO doesn't always free allocations before throwing an exception, detected by InsightSoftwareConsortium#3529.

hjmjohnson pushed a commit to hjmjohnson/ITK that referenced this pull request Aug 26, 2022

BUG: fixed memory leaks in MINC IO

50e1fd4

MINC IO doesn't always free allocations before throwing an exception, detected by InsightSoftwareConsortium#3529.

hjmjohnson pushed a commit to hjmjohnson/ITK that referenced this pull request Aug 26, 2022

BUG: fixed memory leaks in MINC IO

a785ef0

MINC IO doesn't always free allocations before throwing an exception, detected by InsightSoftwareConsortium#3529.

hjmjohnson pushed a commit to hjmjohnson/ITK that referenced this pull request Aug 26, 2022

BUG: fixed memory leaks in MINC IO

5d209cb

MINC IO doesn't always free allocations before throwing an exception, detected by InsightSoftwareConsortium#3529.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BUG: Allow nifti load to fail #3529

BUG: Allow nifti load to fail #3529

hjmjohnson commented Aug 4, 2022 •

edited by dzenanz

Loading

hjmjohnson commented Aug 7, 2022

jhlegarreta commented Aug 10, 2022

jhlegarreta commented Aug 12, 2022

jhlegarreta commented Aug 13, 2022

issakomi commented Aug 13, 2022 •

edited

Loading

issakomi commented Aug 14, 2022 •

edited

Loading

BUG: Allow nifti load to fail #3529

BUG: Allow nifti load to fail #3529

Conversation

hjmjohnson commented Aug 4, 2022 • edited by dzenanz Loading

PR Checklist

hjmjohnson commented Aug 7, 2022

jhlegarreta commented Aug 10, 2022

jhlegarreta commented Aug 12, 2022

jhlegarreta commented Aug 13, 2022

issakomi commented Aug 13, 2022 • edited Loading

issakomi commented Aug 14, 2022 • edited Loading

hjmjohnson commented Aug 4, 2022 •

edited by dzenanz

Loading

issakomi commented Aug 13, 2022 •

edited

Loading

issakomi commented Aug 14, 2022 •

edited

Loading