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: fixed memory leaks in MINC IO #3548

Merged
merged 1 commit into from
Aug 15, 2022

Conversation

issakomi
Copy link
Member

@issakomi issakomi commented Aug 14, 2022

MINC IO does not free allocations before throwing an exception sometimes,
s. discussion #3529

https://open.cdash.org/viewDynamicAnalysis.php?buildid=8097352

(reviewed writing an image).

@issakomi issakomi changed the title WIP: BUG: fixed memory leaks in MINC IO BUG: fixed memory leaks in MINC IO Aug 14, 2022
@issakomi issakomi changed the title BUG: fixed memory leaks in MINC IO WIP: BUG: fixed memory leaks in MINC IO Aug 14, 2022
@issakomi issakomi changed the title WIP: BUG: fixed memory leaks in MINC IO BUG: fixed memory leaks in MINC IO Aug 14, 2022
@issakomi issakomi marked this pull request as draft August 14, 2022 08:24
@issakomi issakomi marked this pull request as ready for review August 14, 2022 08:25
MINC IO doesn't always free allocations before throwing an exception,
detected by InsightSoftwareConsortium#3529.
@github-actions 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 labels Aug 14, 2022
Copy link
Member

@jhlegarreta jhlegarreta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for doing this @issakomi. Have not tested but looks good.

Maybe, following for comment in the related issue:

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,

Adding a test such as the one you did would be convenient.

@gdevenyi
Copy link
Contributor

@vfonov

@issakomi
Copy link
Member Author

issakomi commented Aug 15, 2022

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,

Adding a test such as the one you did would be convenient.

it is really very simple test, to try valgrind --leak-check=full ./test without ITK's testing subsystem, already merged new ITK tests with intentional wrong path (which discovered leaks) are doing the same. Maybe later more tests could be done to trigger exceptions and increase coverage, e.g. 4D image, unsupported data type, etc. I have tested all exit points manually, they work.

@dzenanz dzenanz merged commit c5bc6a6 into InsightSoftwareConsortium:master Aug 15, 2022
@issakomi issakomi deleted the minc_valgr1 branch August 15, 2022 14:46
@vfonov
Copy link
Contributor

vfonov commented Aug 15, 2022

@vfonov

Looks ok

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:IO Issues affecting the IO module type:Bug Inconsistencies or issues which will cause an incorrect result under some or all circumstances
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants