fix(dicom): EXPECT_FRAG crash on undefined-length UN sequences in fromXML - #431
Open
jessesaga wants to merge 1 commit into
Open
Conversation
…mXML
dcm4che2 2.0.29's XML round trip is asymmetric for sequences read from
implicit-VR private tags (e.g. FujiFILM 0029,E131 / Siemens MEDCOM
0029,1140). DicomInputStream treats UN + undefined length holding dataset
items as SQ in memory, but the streaming SAXWriter used by toXML emits the
wire VR (vr="UN" len="-1") with structured <item> children. On fromXML,
ContentHandlerAdapter only enters its sequence state for vr="SQ" and
otherwise expects fragments, throwing
IllegalStateException("state:EXPECT_FRAG") when the item's dataset
elements arrive. Net effect: any DICOM channel with a filter or
transformer fails on such messages.
Fix reader-side in fromXML's existing DOM walk: rewrite vr to "SQ" on
elements whose <item> children contain renamed <attr> elements. This is
the same content-based decision DicomInputStream makes for the binary
form, so the resulting DicomObject, and the DICOM bytes re-encoded from
it, are identical to the no-transformer pass-through path (verified
byte-for-byte for implicit and explicit VR LE). Fragment containers
(encapsulated pixel data, whose items hold only base64/hex text) are
untouched, toXML output is unchanged so transformer scripts see identical
XML, every previously-parsing input produces an identical DicomObject,
and the rewrite is a no-op if a future dcm4che emits SQ itself.
The test builds its DICOM programmatically (no binary fixtures), asserts
the toXML output actually carries the broken wire shape before parsing
it, and covers the encapsulated-pixel-data path the predicate must not
disturb. Pre-fix it fails with the exact production signature; post-fix
the server suite is green.
Signed-off-by: Jesse Dowell <jesse.dowell@gmail.com>
jcdlbs
force-pushed
the
fix/dicom-un-sequence-expect-frag
branch
from
September 2, 2026 19:09
98ad7ed to
b99c630
Compare
jessesaga
marked this pull request as ready for review
September 2, 2026 19:13
jessesaga
requested review from
a team,
gibson9583,
jonbartels,
kayyagari,
kpalang,
mgaffigan,
ssrowe and
tonygermano
September 2, 2026 19:13
mgaffigan
reviewed
Sep 2, 2026
mgaffigan
left a comment
mgaffigan
left a comment
Contributor
There was a problem hiding this comment.
The code looks reasonable, but I'd like to hear from someone who knows DICOM before approving.
From a cleanliness perspective, I don't love that we're editing the XML after construction rather than fixing the generator, though.
There was a problem hiding this comment.
🟢 Approval recommended
The change is narrowly scoped to the XML pre-pass, aligns with documented library behavior, and is covered by targeted regression tests.
Pull request overview
Fixes a dcm4che2 XML round-trip crash in DICOMSerializer.fromXML when undefined-length private-tag sequences are emitted with vr="UN" and structured <item> children, by rewriting those elements to vr="SQ" before SAX parsing (matching the binary reader’s content-based behavior).
Changes:
- Add a DOM pre-pass in
DICOMSerializer.fromXMLto detect dataset-like<item>content and forcevr="SQ"to avoidEXPECT_FRAGstate errors. - Introduce a small helper to detect dataset-item shape (
<item>containing<attr>children). - Add a new JUnit test class covering the undefined-length
UNsequence round-trip and guarding against regressions for encapsulated pixel data fragments.
File summaries
| File | Description |
|---|---|
| server/src/main/java/com/mirth/connect/plugins/datatypes/dicom/DICOMSerializer.java | Rewrites VR to SQ for dataset-like <item> containers before SAX parsing to prevent EXPECT_FRAG crashes. |
| server/src/test/java/com/mirth/connect/plugins/datatypes/dicom/DICOMSerializerUnSequenceTest.java | Adds regression tests for the undefined-length UN sequence crash and ensures fragment containers remain untouched. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+135
to
+148
| if (items != null) { | ||
| for (int i = 0; i < items.getLength(); i++) { | ||
| Node itemNode = items.item(i); | ||
| Node parentNode = itemNode.getParentNode(); | ||
|
|
||
| if (parentNode instanceof Element && hasAttrElementChild(itemNode)) { | ||
| Element parentElement = (Element) parentNode; | ||
|
|
||
| if (parentElement.hasAttribute("vr") && !parentElement.getAttribute("vr").equals("SQ")) { | ||
| parentElement.setAttribute("vr", "SQ"); | ||
| } | ||
| } | ||
| } | ||
| } |
Comment on lines
+117
to
+124
| private String writeDicomFile(BasicDicomObject dcm, String transferSyntaxUid) throws Exception { | ||
| dcm.initFileMetaInformation(transferSyntaxUid); | ||
| ByteArrayOutputStream baos = new ByteArrayOutputStream(); | ||
| DicomOutputStream dos = new DicomOutputStream(baos); | ||
| dos.writeDicomFile(dcm); | ||
| dos.close(); | ||
| return Base64.encodeBase64String(baos.toByteArray()); | ||
| } |
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
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 #430
Summary
DICOM channels with a filter or transformer fail with
IllegalStateException: state:EXPECT_FRAGon messages that carry an undefined-length sequence under a private tag dcm4che2 resolves to VRUN(FujiFILM0029,E131, Siemens MEDCOM0029,1140, and similar). Root cause is an asymmetry inside dcm4che2 2.0.29's XML round trip:SAXWriteremits the wire VR (vr="UN") for such sequences, butContentHandlerAdapteronly recognisesvr="SQ"as a sequence and treats everything else as fragments.This PR fixes it reader-side in
DICOMSerializer.fromXML, inside the DOM pre-pass that already exists there: an element whose<item>children contain dataset elements getsvr="SQ"before the SAX parse. 25 lines of code plus a helper.Why this shape
DicomInputStreamdecides SQ-vs-fragments by item content, not by VR. The predicate here ("item has an<attr>child") mirrors that, so the resultingDicomObjectmatches what a direct binary parse produces. I checked the alternative of keying onvr="UN" len="-1"and it diverges from the binary reader on a sequence whose items are all empty, so I did not use it.toXMLis untouched. Fixing the writer to emitSQwould silently change@vrfor existing scripts.vris alreadySQ, so it becomes a no-op if the DICOM backend ever emitsSQitself.Testing
DICOMSerializerUnSequenceTest(JUnit 4, builds its DICOM programmatically, no binary fixtures):testUndefinedLengthUnSequenceRoundTrip: asserts thetoXMLoutput actually carries the broken wire shape (vr="UN"with a structured<item>), then round-trips and checks the nested element and a defined-lengthUNblob survive.testEncapsulatedPixelDataFragmentsUntouched: regression guard for the fragment path.main, the first test fails with the exact production signature (state:EXPECT_FRAG). Patched,./gradlew test -PdisableSigning=trueis green.Note on naming: the existing
DICOMSerializerTestsis not picked up by the**/*Test.classfilter (and referencestests/test-dicom-*.dcmfixtures that are not in the repo), so the new class follows the*Testconvention the build actually runs.