BUG: check if vtrype is not INVALID by mrc-sys · Pull Request #2553 · InsightSoftwareConsortium/ITK

mrc-sys · 2021-05-26T18:30:36Z

BUG: Only access the value of si , if vrtype is not invalid. This prevents a SegmentationFault in the case of an invalid tag

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.

dzenanz

LGTM

jhlegarreta · 2021-05-26T22:24:59Z

@mrc-sys Thanks for contributing to ITK 💯 !

Can you please remove commit 4ed3b6f in the branch, as it is a duplicate of the latter b300ae2, this one having the appropriate style for an ITK commit? Thanks !

malaterre

You're avoiding a segfault by hiding the symptoms. I suspect this creates an empty Data Element, this is not a desired behavior AFAIK.

mrc-sys · 2021-06-08T09:58:24Z

@mrc-sys Thanks for contributing to ITK 100 !

Can you please remove commit 4ed3b6f in the branch, as it is a duplicate of the latter b300ae2, this one having the appropriate style for an ITK commit? Thanks !

Can you describe me, how to remove the commit? I am happy to get the later commit in the appropriate style....

mrc-sys · 2021-06-08T10:06:49Z

You're avoiding a segfault by hiding the symptoms. I suspect this creates an empty Data Element, this is not a desired behavior AFAIK.

If it´s possible to avoid the crash in a better way, I would prefer the better solution. But for now I have not found this better solution and this segfault is a showstopper for me.

jhlegarreta · 2021-06-08T12:36:56Z

Can you describe me, how to remove the commit? I am happy to get the later commit in the appropriate style....

You should rebase interactively using git rebase -i HEAD~2 then mark the commit to be deleted with d (for drop), exit with :wq, then continue the rebase with git rebase --continue, and it should be fine. Then you would git push -f origin {your_branch_name} to push to the repository.

mrc-sys · 2021-06-08T13:35:47Z

Can you describe me, how to remove the commit? I am happy to get the later commit in the appropriate style....

You should rebase interactively using git rebase -i HEAD~2 then mark the commit to be deleted with d (for drop), exit with :wq, then continue the rebase with git rebase --continue, and it should be fine. Then you would git push -f origin {your_branch_name} to push to the repository.

Can this be done in the github web-gui too?

malaterre · 2021-06-08T13:53:40Z

@mrc-sys

If it´s possible to avoid the crash in a better way, I would prefer the better solution. But for now I have not found this better solution and this segfault is a showstopper for me.

You'll need to provide us more detail on how to reproduce the segfault. Or simply dump part of the backtrace (eg. 'bt' under gdb).

jhlegarreta · 2021-06-08T14:12:30Z

Can this be done in the github web-gui too?

I don't know. I use the command line for these tasks. Sorry.

dzenanz · 2021-06-08T14:41:02Z

I don't think rebase can be done in GitHub WebUI. But it can be done in e.g. TortoiseGit and gitk.

mrc-sys · 2021-06-08T15:41:43Z

@mrc-sys

If it´s possible to avoid the crash in a better way, I would prefer the better solution. But for now I have not found this better solution and this segfault is a showstopper for me.

You'll need to provide us more detail on how to reproduce the segfault. Or simply dump part of the backtrace (eg. 'bt' under gdb).

I will try to reproduce the error again, but I have patched ITK 4.13.1 about 4 years ago (ubuntu 16.04) and the bug popped up again on porting our application to ubuntu 20.04 and ITK 4.13.3. As the patch solved the problem again and the source was not changed in the meantime, I have not tested without the patch again.

Only access the value of si , if vrtype is not invalid.

thewtex · 2021-10-04T01:15:26Z

Addressing style issues

stale · 2022-04-16T09:34:32Z

This issue has been automatically marked as stale because it has not had recent activity. Thank you for your contributions.

hjmjohnson · 2026-05-01T22:43:05Z

Reviving this for resolution. I've opened #6185 which carries the same fix forward, addresses @malaterre's CHANGES_REQUESTED concern from this thread, and adds regression-test coverage (a Google Test that demonstrates the crash on main and confirms the fix). Original authorship is preserved via Co-Authored-By: mrc-sys trailers on both commits.

What changed vs. this PR

The fix in #6185 lifts the gdcm::VR::INVALID check above the per-VR dispatch and routes the unknown tag through the existing problematicKeys channel — so no empty gdcm::DataElement is ever constructed for an unknown tag. That directly addresses @malaterre's "You're avoiding a segfault by hiding the symptoms. I suspect this creates an empty Data Element" concern from this PR. The user gets a single itkWarning listing all unknown keys at the end of the dictionary iteration (same UX the function already produces for non-DICOM keys) instead of a process abort.

Two regression TEST() cases verify the path locally:

Pre-fix: writer dies with gdcm::Exception "An invalid logic behavior occurred" (gdcmVR.cxx:263).
Post-fix: writer completes, expected warning emitted, output file has all valid metadata.

@mrc-sys — thanks for spotting and reporting this in 2021. If #6185 looks reasonable to you and the maintainers, this PR can be closed as superseded.

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>

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>

BUG: Fix GDCMImageIO::Write crash on unknown DICOM tags (supersedes #2553)

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>

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>

…-gdcm-invalid-vrtype-crash BUG: Fix GDCMImageIO::Write crash on unknown DICOM tags (supersedes InsightSoftwareConsortium#2553)

github-actions Bot added area:IO Issues affecting the IO module type:Bug Inconsistencies or issues which will cause an incorrect result under some or all circumstances labels May 26, 2021

dzenanz reviewed May 26, 2021

View reviewed changes

dzenanz requested review from issakomi and malaterre May 26, 2021 18:44

malaterre requested changes May 27, 2021

View reviewed changes

BUG: check if vtrype is not INVALID

1051904

Only access the value of si , if vrtype is not invalid.

thewtex force-pushed the master branch from 4ed3b6f to 1051904 Compare October 4, 2021 01:15

stale Bot added the status:Use_Milestone_Backlog Use "Backlog" milestone instead of label for issues without a fixed deadline label Apr 16, 2022

hjmjohnson mentioned this pull request May 1, 2026

BUG: Fix GDCMImageIO::Write crash on unknown DICOM tags (supersedes #2553) #6185

Merged

7 tasks

hjmjohnson closed this May 1, 2026

hjmjohnson added a commit that referenced this pull request May 5, 2026

Merge pull request #6185 from hjmjohnson/fix-gdcm-invalid-vrtype-crash

da54da4

BUG: Fix GDCMImageIO::Write crash on unknown DICOM tags (supersedes #2553)

Uh oh!

Conversation

mrc-sys commented May 26, 2021

PR Checklist

Uh oh!

dzenanz left a comment

Choose a reason for hiding this comment

Uh oh!

jhlegarreta commented May 26, 2021

Uh oh!

malaterre left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mrc-sys commented Jun 8, 2021

Uh oh!

mrc-sys commented Jun 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jhlegarreta commented Jun 8, 2021

Uh oh!

mrc-sys commented Jun 8, 2021

Uh oh!

malaterre commented Jun 8, 2021

Uh oh!

jhlegarreta commented Jun 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dzenanz commented Jun 8, 2021

Uh oh!

mrc-sys commented Jun 8, 2021

Uh oh!

thewtex commented Oct 4, 2021

Uh oh!

stale Bot commented Apr 16, 2022

Uh oh!

hjmjohnson commented May 1, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

malaterre left a comment •

edited

Loading

mrc-sys commented Jun 8, 2021 •

edited

Loading

jhlegarreta commented Jun 8, 2021 •

edited

Loading