Fix 32bits dicom read crash by lassoan · Pull Request #4522 · InsightSoftwareConsortium/ITK

lassoan · 2024-03-17T23:22:29Z

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

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)
Updated API documentation (or API not changed)
Added license to new files (if any)
Added Python wrapping to new files (if any) as described in ITK Software Guide Section 9.5
Added ITK examples for all new major features (if any)

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

N-Dekker · 2024-03-18T14:48:45Z

-static bool
-readPreambleDicom(std::ifstream & file)
+bool
+isPreambleDicom(std::ifstream & file)
+{
+  file.seekg(0, std::ios::beg);
+  std::vector<char> buffer(DCM_PreambleLen + DCM_MagicLen);
+  file.read(&buffer[0], buffer.size());
+  if (!file)
+  {
+    return false;
+  }
+  const char * magicReadFromFile = &buffer[DCM_PreambleLen];
+  return strncmp(magicReadFromFile, DCM_Magic, DCM_MagicLen) == 0;
+}


Thanks @lassoan Just wondering, how does your pull request relate to BUG: DCMTK reader wrongly rejects file with preamble #4501 ? Didn't pull request 4501 fix the problem already?
More in detail, isPreambleDicom is just a style improvement to readPreambleDicom, right?

Yes, my first commit is just a style improvement (and a little performance improvement) over #4501. There is no functional difference.

#4501 did not fix the issue because the application first tries to read the DICOM image using GDCM and if it fails then it uses DCMTK. However, GDCM did not return with an error but it crashed the application, so the application did not have a chance to use DCMTK.

thewtex

Very nice @lassoan !

Could you please:

split the GDCM change into a patch and PR against malaterre/GDCM for the release branch
Add a test here with the file reported on the Slicer Discourse for both ITKIOGDCM and ITKIODCMTK to ensure we do not crash

?

lassoan · 2024-03-18T15:55:53Z

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 · 2024-03-18T16:21:08Z

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 · 2024-03-19T03:07:34Z

Submitted a PR to GDCM:

malaterre/GDCM#168

thewtex · 2024-03-20T19:29:49Z

Thank you @lassoan !

LGTM.

Path to integration:

Merge GDCM Secondary Capture spatial metadata #4521
I will create another PR that integrates BUG: Fix GDCM crash when reading DICOM image malaterre/GDCM#168 via our subtree merge process.
Rebase this PR

lassoan · 2024-03-21T01:58:58Z

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

lassoan · 2024-03-22T22:34:15Z

The GDCM pull request has been merged.

hjmjohnson · 2024-03-23T12:40:54Z

@lassoan Thanks!

thewtex · 2024-03-25T11:34:58Z

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.

Do not test for no-preamble DICOM file format if standard preamble format is found. Use DCMTK constants (instead of hardcoding the preamble size, magic word, etc.). Remove unnecessary "static" keyword from function in anonymous namespace. Simplified debug message of a DICOM file without preamble.

GDCM used to crash the application when it encountered a DICOM image that had 32 bits stored. This new test checks that GDCM does not crash anymore for such images.

thewtex · 2024-03-26T14:27:07Z

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

lassoan · 2024-03-27T01:47:30Z

Thank you @thewtex!

dzenanz · 2024-03-27T11:54:40Z

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 · 2024-03-27T12:06:13Z

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 · 2024-03-27T12:12:54Z

I would prefer to remove the debug statement entirely.

SimonRit · 2024-03-27T12:17:07Z

Sounds good to me, that should also compile!

lassoan · 2024-03-27T12:49:56Z

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 · 2024-03-27T13:19:56Z

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

SimonRit · 2024-03-27T13:26:21Z

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

seanm · 2024-03-28T14:44:29Z

@lassoan do you have a fix in the works?

dzenanz · 2024-03-28T15:31:59Z

@seanm There is #4544.

…bits-dicom-read-crash Fix 32bits dicom read crash

github-actions Bot added area:IO Issues affecting the IO module area:ThirdParty Issues affecting the ThirdParty module labels Mar 17, 2024

dzenanz requested review from issakomi, malaterre and thewtex March 18, 2024 12:33

N-Dekker reviewed Mar 18, 2024

View reviewed changes

thewtex requested changes Mar 18, 2024

View reviewed changes

N-Dekker reviewed Mar 18, 2024

View reviewed changes

Comment thread Modules/IO/DCMTK/src/itkDCMTKImageIO.cxx Outdated

lassoan force-pushed the fix-32bits-dicom-read-crash branch from e3058e6 to 55f2f0c Compare March 19, 2024 03:12

github-actions Bot added type:Infrastructure Infrastructure/ecosystem related changes, such as CMake or buildbots type:Testing Ensure that the purpose of a class is met/the results on a wide set of test cases are correct labels Mar 19, 2024

lassoan added 2 commits March 26, 2024 10:10

ENH: Add GDCM test for 32 bits stored DICOM image

7e350ef

GDCM used to crash the application when it encountered a DICOM image that had 32 bits stored. This new test checks that GDCM does not crash anymore for such images.

thewtex force-pushed the fix-32bits-dicom-read-crash branch from 55f2f0c to 7e350ef Compare March 26, 2024 14:25

github-actions Bot removed the area:ThirdParty Issues affecting the ThirdParty module label Mar 26, 2024

thewtex approved these changes Mar 26, 2024

View reviewed changes

thewtex merged commit 5f3adbd into InsightSoftwareConsortium:master Mar 26, 2024

lassoan deleted the fix-32bits-dicom-read-crash branch March 27, 2024 01:45

hjmjohnson pushed a commit to hjmjohnson/ITK that referenced this pull request May 6, 2026

Merge pull request InsightSoftwareConsortium#4522 from lassoan/fix-32…

b7fe77a

…bits-dicom-read-crash Fix 32bits dicom read crash

Uh oh!

Conversation

lassoan commented Mar 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Checklist

Uh oh!

N-Dekker Mar 18, 2024

Choose a reason for hiding this comment

Uh oh!

lassoan Mar 18, 2024

Choose a reason for hiding this comment

Uh oh!

thewtex left a comment

Choose a reason for hiding this comment

Uh oh!

lassoan commented Mar 18, 2024

Uh oh!

thewtex commented Mar 18, 2024

Uh oh!

Uh oh!

lassoan commented Mar 19, 2024

Uh oh!

thewtex commented Mar 20, 2024

Uh oh!

lassoan commented Mar 21, 2024

Uh oh!

lassoan commented Mar 22, 2024

Uh oh!

hjmjohnson commented Mar 23, 2024

Uh oh!

thewtex commented Mar 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thewtex commented Mar 26, 2024

Uh oh!

lassoan commented Mar 27, 2024

Uh oh!

dzenanz commented Mar 27, 2024

Uh oh!

SimonRit commented Mar 27, 2024

Uh oh!

dzenanz commented Mar 27, 2024

Uh oh!

SimonRit commented Mar 27, 2024

Uh oh!

lassoan commented Mar 27, 2024

Uh oh!

dzenanz commented Mar 27, 2024

Uh oh!

SimonRit commented Mar 27, 2024

Uh oh!

seanm commented Mar 28, 2024

Uh oh!

dzenanz commented Mar 28, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

lassoan commented Mar 17, 2024 •

edited

Loading

thewtex commented Mar 25, 2024 •

edited

Loading