InsightSoftwareConsortium / InsightSoftwareConsortium/ITK
BUG: DCMTKImageIO segfaults on any unreadable DICOM — swallowed `LoadFile()` failure leaves `m_Dataset` null
- Dominant language
- C++
- Stars
- 1.7k
- Forks
- 748
- Avg merge
- 1d 1h
- Merged PRs (30d)
- 64
Description
**Reading any missing, truncated, or otherwise unreadable DICOM file through `itk::DCMTKImageIO` crashes the process with a SEGV instead of throwing an `itk::ExceptionObject`.**
`DCMTKImageIO::ReadImageInformation()` catches the failure from `DCMTKFileReader::LoadFile()`, logs it to `std::cerr`, and then falls through — leaving `m_Dataset` null. The very next call, `reader.GetElementSL(...)`, dereferences that null pointer. Applications get an unrecoverable crash where they should get a catchable exception. Present on stock `main`.
Failure trace
macOS, Release build:
```
frame #0 DcmItem::findAndGetElement(...) + 56 EXC_BAD_ACCESS address=0x0
frame #1 itk::DCMTKFileReader::GetElementSL(unsigned short, unsigned short, int&, bool) const
frame #2 itk::DCMTKImageIO::ReadImageInformation()
frame #3 itk::ImageFileReader<...>::GenerateOutputInformation()
```
Originally observed while validating #6393 when ExternalData test inputs were absent: 13 DCMTK tests SEGV'd identically on both the patched build and a stock-`main` baseline, confirming the defect is pre-existing and independent of that PR.
Evidence — current upstream/main line numbers
**Swallowed failure** — `Modules/IO/DCMTK/src/itkDCMTKImageIO.cxx`, lines **371-390** (`ReadImageInformation()` begins at line 365):
```cpp
371: DCMTKFileReader reader;
372: reader.SetFileName(this->m_FileName);
373: try
374: {
375: reader.LoadFile();
376: }
377: catch (...)
378: {
379: std::cerr << "DCMTKImageIO::ReadImageInformation: "
380: << "DicomImage could not read the file." << std::endl;
381: }
382:
383: // check for multiframe > 3D
384: itk::int32_t numPhases;
385: unsigned int numDim(3);
386:
387: if (reader.GetElementSL(0x2001, 0x1017, numPhases, false) != EXIT_SUCCESS)
```
The `catch` neither rethrows nor returns; execution continues at line 387 with `reader.m_Dataset == nullptr`.
**Null dereference** — `Modules/IO/DCMTK/src/itkDCMTKFileReader.cxx`, `GetElementSL` declared at line **784**, deref at line **791**:
```cpp
784: DCMTKFileReader::GetElementSL(const unsigned short group,
785: const unsigned short element,
786: int32_t & target,
787: const bool throwException) const
788: {
789: DcmTagKey tagkey(group, element);
790: DcmElement * el;
791: if (this->m_Dataset->findAndGetElement(tagkey, el) != EC_Normal) // m_Dataset can be null
```
There are **21** unguarded `this->m_Dataset->` dereferences in `itkDCMTKFileReader.cxx`, so `GetElementSL` is representative rather than unique. The `GetElementSL` declaration lives at `Modules/IO/DCMTK/include/itkDCMTKFileReader.h:412`.
Suggested fix
1. In `ReadImageInformation()` (and any sibling that wraps `LoadFile()` in a `catch(...)` that merely logs), convert the swallowed failure into a hard stop — replace the `std::cerr` line with
`itkExceptionMacro("Could not read DICOM file: " << this->m_FileName);`
so the reader never continues with a null dataset.
2. Defensively guard `DCMTKFileReader::GetElementSL` and the other `GetElement*` accessors with a null-`m_Dataset` check that reports through the existing `DCMTKExceptionOrErrorReturn` macro, so a null dataset can never reach `findAndGetElement`.
3. Audit all 21 `m_Dataset->` sites in `itkDCMTKFileReader.cxx` for the same unguarded access.
Proposed regression test
Add a test that asserts an exception rather than a crash. Sketch (CTest style, alongside the existing `itkDCMTKImageIONoPreambleTest` / `itkDCMTKImageIOPreambleCanReadTest` fixtures in `Modules/IO/DCMTK/test/`):
```cpp
// itkDCMTKImageIOReadFailureTest.cxx
int itkDCMTKImageIOReadFailureTest(int argc, char * argv[])
{
// argv[1]: a path that does not exist, and/or a truncated/garbage file
auto imageIO = itk::DCMTKImageIO::New();
imageIO->SetFileName(argv[1]);
bool caught = false;
try
{
imageIO->ReadImageInformation();
}
catch (const itk::ExceptionObject & e)
{
caught = true;
std::cout << "Caught expected exception: " << e.GetDescription() << std::endl;
}
if (!caught)
{
std::cerr << "Expected itk::ExceptionObject for unreadable DICOM input" << std::endl;
return EXIT_FAILURE;
}
return EXIT_SUCCESS;
}
```
Run it for at least two inputs: (a) a nonexistent path, (b) a small truncated / non-DICOM file written by the test itself into `${ITK_TEST_OUTPUT_DIR}` (avoids a new ExternalData fixture). Before the fix this test segfaults; after it, it passes. A `GetElementSL`-level unit check on a default-constructed `DCMTKFileReader` would cover item 2 directly.
Related
- #6393 — DCMTK `find_package` fix; this bug surfaced during its validation but is independent of it.
Contributor guide
Research direction
Start in Modules/IO/DCMTK/src/itkDCMTKImageIO.cxx at ReadImageInformation() and review the LoadFile() catch, then inspect GetElementSL and the other m_Dataset dereferences in itkDCMTKFileReader.cxx. Review the existing DCMTK tests in Modules/IO/DCMTK/test/, especially itkDCMTKImageIONoPreambleTest and itkDCMTKImageIOPreambleCanReadTest. Done means unreadable or missing inputs produce itk::ExceptionObject rather than a crash, with regression coverage for both cases.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- cpp
- Domain
- computer-vision, testing-qa
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Quiet
- Clarity
- Clearly specified
- Newbie friendliness
- 58/100