Fix 32bits dicom read crash

[lassoan]

Fix crash in ITK DICOM reader when attempting to read an unusual but valid DICOM file, which has 32 bits allocated. Fixes the error reported at https://discourse.slicer.org/t/slicer-crashes-while-trying-to-load-dicom-files/34931/3

PR Checklist

• [x] No API changes were made (or the changes have been approved)
• [x] No major design changes were made (or the changes have been approved)
• [x] Added test (or behavior not changed)
• [x] Updated API documentation (or API not changed)
• [x] Added license to new files (if any)
• [x] Added Python wrapping to new files (if any) as described in ITK Software Guide Section 9.5
• [x] Added ITK examples for all new major features (if any)

Refer to the ITK Software Guide for further development details if necessary.

[lassoan]

• split the GDCM change into a patch and PR against malaterre/GDCM for the release branch

malaterre/GDCM: I got a message when I committed that I would need to send something to sourceforge. The link did not work, so I hoped that I didn't need to deal with it. It seems that GDCM is on github now - https://github.com/malaterre/GDCM - do you mean I should send a pull request there?

• Add a test here with the file reported on the Slicer Discourse for both ITKIOGDCM and ITKIODCMTK to ensure we do not crash

I've asked for permission for using a slice of the data set as test data: https://discourse.slicer.org/t/slicer-crashes-while-trying-to-load-dicom-files/34931/5

[thewtex]

https://github.com/malaterre/GDCM - do you mean I should send a pull request there?

Yes, please. We integrate patches from there into ITK if accepted.

I've asked for permission for using a slice of the data set as test data: https://discourse.slicer.org/t/slicer-crashes-while-trying-to-load-dicom-files/34931/5

Most excellent.

[lassoan]

Submitted a PR to GDCM:

https://github.com/malaterre/GDCM/pull/168

[thewtex]

Thank you @lassoan !

LGTM.

Path to integration:

1. Merge #4521
2. I will create another PR that integrates malaterre/GDCM#168 via our subtree merge process.
3. Rebase this PR

[lassoan]

Thank you! Let me know if you need any help from me.

[lassoan]

The GDCM pull request has been merged.

[hjmjohnson]

@lassoan Thanks!

[thewtex]

Both the GDCM patch for this branch and the GDCM patch for #4521 have been integrated into GDCM, and these are integrated in #4521 -> after #4521 has been merged, this branch can be rebased and merged.

[thewtex]

Dropped the GDCM patch (integrated via malaterre/GDCM#168 and #4521 ) and rebased.

[lassoan]

Thank you @thewtex!

[dzenanz]

From the dashboard:

/Users/builder/external/ITK/Modules/IO/GDCM/src/itkGDCMImageIO.cxx:191:3: error: invalid use of 'this' outside of a non-static member function
  itkDebugMacro(<< "No DICOM magic number found, but the file appears to be DICOM without a preamble.");
  ^

Probably use itkWarningMacro instead of itkDebugMacro?

[SimonRit]

It believe it will be the same problem for itkWarningMacro (use of this->GetNameOfClass()). I was about to submit the following patch:

-  itkDebugMacro(<< "No DICOM magic number found, but the file appears to be DICOM without a preamble.");
+#ifndef NDEBUG
+  itkGenericOutputMacro("No DICOM magic number found, but the file appears to be DICOM without a preamble.");
+#endif

which compiles, what do you think?

[dzenanz]

I would prefer to remove the debug statement entirely.

[SimonRit]

Sounds good to me, that should also compile!

[lassoan]

Sorry, all testing was done in release mode, probably that's why this error was not caught.

The log message was there before, I just switched to using a macro. Since detection of a DICOM file without a preamble requires some heuristics, so when investigating why a file is detected/not detected as DICOM it might be useful to know the result of the heuristic check. But it should not be logged every time when a DICOM file without a preamble is encountered.

Has somebody started to work on a new pull request or I should do it?

[dzenanz]

I have not started any work. I am giving other people a chance to fix this 😄

[SimonRit]

Nope, I haven't done more than testing the patch I suggested. Go ahead!

[seanm]

@lassoan do you have a fix in the works?

[dzenanz]

@seanm There is #4544.