Skip to content

BUG: Fixes crashes in unit tests when argv[0] == nullptr#564

Merged
dzenanz merged 1 commit intoInsightSoftwareConsortium:masterfrom
N-Dekker:Fix-Test-argv-nullptr-crashes
Mar 4, 2019
Merged

BUG: Fixes crashes in unit tests when argv[0] == nullptr#564
dzenanz merged 1 commit intoInsightSoftwareConsortium:masterfrom
N-Dekker:Fix-Test-argv-nullptr-crashes

Conversation

@N-Dekker
Copy link
Copy Markdown
Contributor

@N-Dekker N-Dekker commented Mar 3, 2019

Many unit tests could crash (or have undefined behavior), when doing something
like the following with argv[0] == nullptr:

std::cerr << "Usage: " << argv[0] << std::endl;

This commit prevents such crashes by using a new macro,
itkNameOfTestExecutableMacro(argv), instead of argv[0].

@N-Dekker N-Dekker changed the title WIP: BUG: Fixes crashes in unit tests when argv[0] == nullptr BUG: Fixes crashes in unit tests when argv[0] == nullptr Mar 3, 2019
Many unit tests could crash (or have undefined behavior), when doing something
like the following with `argv[0] == nullptr`:

    std::cerr << "Usage: " << argv[0] << std::endl;

This commit prevents such crashes by using a new macro,
`itkNameOfTestExecutableMacro(argv)`, instead of `argv[0]`.
@N-Dekker
Copy link
Copy Markdown
Contributor Author

N-Dekker commented Mar 4, 2019

This pull request fixes crashes (undefined behavior) from many ITK test drivers, when the user manually selects a test from the command-line interface, for example:

Run ITKBiasCorrectionTestDriver. The following output appears:

Available tests:
  0. itkCompositeValleyFunctionTest
  1. itkMRIBiasFieldCorrectionFilterTest
  2. itkN4BiasFieldCorrectionImageFilterTest
To run a test, enter the test number:

Then type 2 and press Enter

Without this fix, it would crash, trying to print argv[0] at

std::cerr << "Usage: " << argv[0] << " imageDimension inputImage "

When this fix is applied, it won't crash anymore. Instead, it will say:

Usage: <itkN4BiasFieldCorrectionImageFilterTest executable> imageDimension inputImage outputLogControlPointLattice [shrinkFactor,default=1] [numberOfIterations,default=100x50x50]  [maskImageWithLabelEqualTo1] [splineDistance,default=200]

What do you think?

Comment thread Modules/Core/TestKernel/include/itkTestingMacros.h
@dzenanz
Copy link
Copy Markdown
Member

dzenanz commented Mar 4, 2019

I was bothered by that behavior, but never enough to spend the time to fix it. Thanks Niels!

Copy link
Copy Markdown
Member

@hjmjohnson hjmjohnson left a comment

Choose a reason for hiding this comment

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

+1

@dzenanz dzenanz merged commit 1fa53fe into InsightSoftwareConsortium:master Mar 4, 2019
@jhlegarreta
Copy link
Copy Markdown
Member

jhlegarreta commented Mar 9, 2019

@N-Dekker I have just bumped into this PR. Very nice 👍 !

I've got two comments whose scope is a little bit broader but related to this topic:

  • About the test driver message:

Run ITKBiasCorrectionTestDriver. The following output appears:
Available tests:

  1. itkCompositeValleyFunctionTest
  2. itkMRIBiasFieldCorrectionFilterTest
  3. itkN4BiasFieldCorrectionImageFilterTest

To run a test, enter the test number:
Then type 2 and press Enter
Without this fix, it would crash, trying to print argv[0] at

Then the test driver message is also/still misleading right? Even if I usually call the tests by their name, I remember having experienced this, and dot not specifically remember the crash conditions, but I would dare to say that hitting the right number does execute the test (?). If this was not the case, and even if the usage is now printed and this prevents the crash, then test instruction does still not honor the behavior, right? Sorry if I'm missing some piece of information.

  • Has this change been incorporated into the ITK SW Guide? This is a broader concern, but also applies to this topic: I feel that there have been quite a few enhancements concerning class declarations (C++11, performance improvements, etc.), in the last few months that have not been incorporated into the SW Guide. An effort has been made to update the Release Notes, but I my feeling is that some have not made their way into the SW Guide. After a few weeks, it is all the way harder to remember them, so as to add them to the SW Guide.

Thanks.

jhlegarreta added a commit to jhlegarreta/ITKModuleTemplate that referenced this pull request Apr 9, 2019
Use the `itkNameOfTestExecutableMacro` to get the test name and prevent
the the test from crashing when `argv[0]` is null, as recommended in:
InsightSoftwareConsortium/ITK#564
thewtex pushed a commit to jhlegarreta/ITKModuleTemplate that referenced this pull request Jul 30, 2019
Use the `itkNameOfTestExecutableMacro` to get the test name and prevent
the the test from crashing when `argv[0]` is null, as recommended in:
InsightSoftwareConsortium/ITK#564
@jhlegarreta
Copy link
Copy Markdown
Member

I realize that this missed all av[0] cases. In some cases, where I was editing a given test file to increase the code coverage, I changed them.

A PR to change the remaining ones: #2939

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

Successfully merging this pull request may close these issues.

4 participants