BUG: Fix GDCMImageIO::Write crash on unknown DICOM tags (supersedes #2553)#6185
Merged
hjmjohnson merged 2 commits intoInsightSoftwareConsortium:mainfrom May 5, 2026
Conversation
github-actions
Bot
added
type:Bug
7 tasks
Adds two regression tests that demonstrate the bug originally reported in PR InsightSoftwareConsortium#2553: when MetaDataDictionary contains a key that parses as a well-formed (group,element) DICOM tag but is not in GDCM's public dictionary (e.g., a private vendor tag, or any custom tag added by ITK clients without a private creator), GDCMImageIO::Write() looks up its VR via gdcm::Dict::GetDictEntry(tag).GetVR() which returns gdcm::VR::INVALID. The downstream call to gdcm::StringFilter::FromString hits the switch's default branch in gdcmStringFilter.cxx, which calls gdcm_assert(0) and terminates the process. These two tests run against the unfixed code and reproduce the crash (EXPECT_NO_THROW fails because the writer dies inside gdcm_assert(0)). They are added in this separate commit so the bisection / regression window is unambiguous. The accompanying COMP commit lifts the gdcm::VR::INVALID detection out of the per-VR dispatch in itkGDCMImageIO.cxx and routes unknown tags through the existing problematicKeys warning channel, after which both tests pass. Adds the first creategoogletestdriver() entry to Modules/IO/GDCM/test/ CMakeLists.txt; subsequent gtests for this module can be appended to ITKIOGDCMGTests. Co-Authored-By: mrc-sys <65188023+mrc-sys@users.noreply.github.com>
hjmjohnson
force-pushed
the
fix-gdcm-invalid-vrtype-crash
branch
from
fb8351b to
dae43f9
Compare
github-actions
Bot
removed
the
area:Documentation
Member
Author
|
@greptileai review this draft before I make it official |
This comment was marked as resolved.
This comment was marked as resolved.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
GDCMImageIO::Write looks up each MetaDataDictionary entry's DICOM VR via gdcm::Dict::GetDictEntry(tag).GetVR() against the public dictionary. For tags that are not in the public dictionary (private vendor tags, custom client-added tags, typos), the lookup returns gdcm::VR::INVALID. The downstream call to gdcm::StringFilter::FromString in the VRASCII branch hits the switch's default case in gdcmStringFilter.cxx and calls gdcm_assert(0), terminating the host process. PR InsightSoftwareConsortium#2553 (mrc-sys, 2021-05-26) proposed guarding the inner FromString call only, but as Mathieu Malaterre noted in his CHANGES_REQUESTED review, that approach silently writes an empty Data Element to the DICOM header --- masking the symptom rather than fixing the underlying issue. Lift the VR::INVALID check ahead of the per-VR dispatch and route the tag through the existing problematicKeys channel (already used in this function for unrecognized non-DICOM keys). The problematicKeys accumulator emits a single warning at the end of the dictionary iteration listing every unknown key, which is the same UX a typo or a mis-encoded private tag has always produced for ITK clients --- now extended to cover well-formed-(group,element)-but-unknown tags. The two regression tests added in the preceding commit (itkGDCMImageIOInvalidVRTypeGTest.cxx) which crashed against the previous tip now complete cleanly and produce the expected warning. Supersedes PR InsightSoftwareConsortium#2553. Co-Authored-By: mrc-sys <65188023+mrc-sys@users.noreply.github.com>
hjmjohnson
force-pushed
the
fix-gdcm-invalid-vrtype-crash
branch
from
dae43f9 to
08f9fcd
Compare
3 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes a process-terminating crash in
GDCMImageIO::Write()when MetaDataDictionary contains a well-formed(group,element)key that is not in GDCM's public DICOM dictionary (private vendor tags, custom client-added tags, typos). Supersedes #2553. The first commit is the regression test (fails againstmain); the second commit is the fix.Root cause
GDCMImageIO::Write()looks up each metadata key's VR viapubdict.GetDictEntry(tag).GetVR(). For tags missing from the public dictionary, the lookup returnsgdcm::VR::INVALID. The downstream call togdcm::StringFilter::FromString(tag, value, len)falls through to itsswitch(vr)default branch ingdcmStringFilter.cxx, which callsgdcm_assert(0)— terminating the host process.Reproducer (in commit 1): create an
itk::Image<unsigned short, 2>, populate metadata with a normal CT-style tag set, addEncapsulateMetaData<std::string>(dict, "9999|9999", "x"), write viaImageFileWriter. On unfixedmain, the writer dies withgdcm::Exception "An invalid logic behavior occurred"(gdcmVR.cxx:263). On the fixed code the writer completes, the unknown tag is reported via the existingitkWarningMacro("ignoring non-DICOM and non-ITK standard keys = ...")channel, and all valid metadata + pixel data is written.Why the original PR #2553 approach was insufficient
PR #2553 (mrc-sys, 2021-05-26) proposed
else→else if (vrtype != gdcm::VR::INVALID)inside the VRASCII branch, which suppresses the crash but constructs an emptygdcm::DataElementand inserts it into the DICOM header anyway. malaterre's CHANGES_REQUESTED review flagged this exact concern: "You're avoiding a segfault by hiding the symptoms. I suspect this creates an empty Data Element, this is not a desired behavior AFAIK."This PR lifts the
VR::INVALIDcheck ahead of the per-VR dispatch and routes unknown tags throughproblematicKeys(the existing warning channel already used in this function for non-DICOM keys) — so no empty DataElement is ever constructed.Files changed
Modules/IO/GDCM/test/itkGDCMImageIOInvalidVRTypeGTest.cxxTEST()casesModules/IO/GDCM/test/CMakeLists.txtcreategoogletestdriver()for the GDCM module +target_compile_definitions(ITK_TEST_OUTPUT_DIR=...)Documentation/docs/releases/5.4.6.mdpre-commit run --all-files(introduced today by upstream 5.4.5 release-notes commit)Modules/IO/GDCM/src/itkGDCMImageIO.cxxif (vrtype == gdcm::VR::INVALID) { problematicKeys.push_back(key); }branch, ahead of the per-VR dispatchLocal verification
Built
ITKIOGDCMGTestDriveragainst~/src/ITK/buildat:bin/ITKIOGDCMGTestDriver --gtest_filter='GDCMImageIOInvalidVRType*'⇒ first test crashes withgdcm::Exception "An invalid logic behavior occurred".WARNING: ... ignoring non-DICOM and non-ITK standard keys = 9999|9999.pre-commit run --all-filesexits 0 against the pushed HEAD.PR Checklist