InsightSoftwareConsortium / InsightSoftwareConsortium/ITK

BUG: DCMTKImageIO segfaults on any unreadable DICOM — swallowed `LoadFile()` failure leaves `m_Dataset` null

Open
#6,719 0 comments 1 reaction 0 assignees View on GitHub
area:IO type:Bug
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

Open the contributing 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.