From b99c63040dd97de2fb6006723d25cce44c0147ea Mon Sep 17 00:00:00 2001 From: Jesse Dowell Date: Wed, 2 Sep 2026 12:48:53 -0600 Subject: [PATCH] fix(dicom): EXPECT_FRAG crash on undefined-length UN sequences in fromXML 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 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 children contain renamed 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 --- .../datatypes/dicom/DICOMSerializer.java | 35 ++++ .../dicom/DICOMSerializerUnSequenceTest.java | 152 ++++++++++++++++++ 2 files changed, 187 insertions(+) create mode 100644 server/src/test/java/com/mirth/connect/plugins/datatypes/dicom/DICOMSerializerUnSequenceTest.java diff --git a/server/src/main/java/com/mirth/connect/plugins/datatypes/dicom/DICOMSerializer.java b/server/src/main/java/com/mirth/connect/plugins/datatypes/dicom/DICOMSerializer.java index 9b16c4bbb2..81c8326809 100644 --- a/server/src/main/java/com/mirth/connect/plugins/datatypes/dicom/DICOMSerializer.java +++ b/server/src/main/java/com/mirth/connect/plugins/datatypes/dicom/DICOMSerializer.java @@ -122,6 +122,31 @@ public String fromXML(String source) throws MessageSerializerException { } } + /* + * dcm4che2's XML round trip is asymmetric for sequences read from implicit-VR private + * tags: SAXWriter emits the wire VR (vr="UN" len="-1") with structured + * children, but ContentHandlerAdapter only enters its sequence state for vr="SQ" and + * otherwise expects fragments, throwing IllegalStateException("state:EXPECT_FRAG") + * when a dataset item arrives. DicomInputStream already treats such elements as SQ in + * memory, so present them to the parser the same way. Only items holding renamed + * children mark a dataset sequence; fragment containers (encapsulated pixel + * data, whose items hold only base64/hex text) are left untouched. + */ + 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"); + } + } + } + } + // find the charset String charset = null; Element charsetElement = (Element) document.getElementsByTagName("tag00080005").item(0); @@ -209,6 +234,16 @@ private void renameAttrToTag(Document document, Node node) throws DOMException { } } + private boolean hasAttrElementChild(Node node) { + for (Node child = node.getFirstChild(); child != null; child = child.getNextSibling()) { + if (child.getNodeType() == Node.ELEMENT_NODE && child.getNodeName().equals("attr")) { + return true; + } + } + + return false; + } + private void renameTagToAttr(Document document, Node node) throws DOMException { NamedNodeMap attr = node.getAttributes(); diff --git a/server/src/test/java/com/mirth/connect/plugins/datatypes/dicom/DICOMSerializerUnSequenceTest.java b/server/src/test/java/com/mirth/connect/plugins/datatypes/dicom/DICOMSerializerUnSequenceTest.java new file mode 100644 index 0000000000..246e63140a --- /dev/null +++ b/server/src/test/java/com/mirth/connect/plugins/datatypes/dicom/DICOMSerializerUnSequenceTest.java @@ -0,0 +1,152 @@ +// SPDX-License-Identifier: MPL-2.0 +// SPDX-FileCopyrightText: Saga IT, LLC + +package com.mirth.connect.plugins.datatypes.dicom; + +import static org.junit.Assert.assertArrayEquals; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertTrue; + +import java.io.ByteArrayInputStream; +import java.io.ByteArrayOutputStream; +import java.nio.charset.StandardCharsets; + +import javax.xml.parsers.DocumentBuilderFactory; + +import org.apache.commons.codec.binary.Base64; +import org.dcm4che2.data.BasicDicomObject; +import org.dcm4che2.data.DicomElement; +import org.dcm4che2.data.DicomObject; +import org.dcm4che2.data.Tag; +import org.dcm4che2.data.UID; +import org.dcm4che2.data.VR; +import org.dcm4che2.io.DicomOutputStream; +import org.junit.Test; +import org.w3c.dom.Document; +import org.w3c.dom.Element; +import org.w3c.dom.Node; +import org.w3c.dom.NodeList; + +import com.mirth.connect.model.converters.DICOMConverter; + +/** + * Round-trip tests for undefined-length VR=UN sequences (private implicit-VR tags, e.g. FujiFILM + * (0029,E131) / Siemens MEDCOM (0029,1140)). dcm4che2's SAXWriter emits these with their wire VR + * while ContentHandlerAdapter requires vr="SQ" for dataset items, so before the fromXML fix the + * round trip threw IllegalStateException("state:EXPECT_FRAG"). + */ +public class DICOMSerializerUnSequenceTest { + + private static final int PRIVATE_SEQUENCE_TAG = 0x0029E131; + private static final int PRIVATE_BLOB_TAG = 0x00291001; + private static final byte[] BLOB_BYTES = new byte[] { 1, 2, 3, 4 }; + + @Test + public void testUndefinedLengthUnSequenceRoundTrip() throws Exception { + BasicDicomObject dcm = new BasicDicomObject(); + dcm.putString(Tag.SOPClassUID, VR.UI, UID.SecondaryCaptureImageStorage); + dcm.putString(Tag.SOPInstanceUID, VR.UI, "1.2.3.4.5.6.7.8.9"); + dcm.putString(Tag.PatientName, VR.PN, "TEST^PATIENT"); + dcm.putBytes(PRIVATE_BLOB_TAG, VR.UN, BLOB_BYTES); + + BasicDicomObject item = new BasicDicomObject(); + item.putString(Tag.CodeValue, VR.SH, "VALUE1"); + dcm.putNestedDicomObject(PRIVATE_SEQUENCE_TAG, item); + + String base64 = writeDicomFile(dcm, UID.ImplicitVRLittleEndian); + + DICOMSerializer serializer = new DICOMSerializer(); + String xml = serializer.toXML(base64); + + // Guard against a vacuous pass: the XML must actually carry the broken wire shape + // (vr="UN" with a structured child) or this test isn't exercising the fix. + Element sequenceElement = findElementByTag(xml, "0029E131"); + assertNotNull("private sequence element missing from XML", sequenceElement); + assertEquals("UN", sequenceElement.getAttribute("vr")); + NodeList sequenceItems = sequenceElement.getElementsByTagName("item"); + assertTrue("expected an child", sequenceItems.getLength() > 0); + assertTrue("expected the to hold dataset elements", hasElementChild(sequenceItems.item(0))); + + // Pre-fix this threw MessageSerializerException caused by + // IllegalStateException("state:EXPECT_FRAG"). + String roundTripped = serializer.fromXML(xml); + + DicomObject result = DICOMConverter.byteArrayToDicomObject(Base64.decodeBase64(roundTripped), false); + assertEquals("TEST^PATIENT", result.getString(Tag.PatientName)); + + DicomObject resultItem = result.getNestedDicomObject(PRIVATE_SEQUENCE_TAG); + assertNotNull("private sequence lost in round trip", resultItem); + assertEquals("VALUE1", resultItem.getString(Tag.CodeValue)); + + // Defined-length UN blobs have no items and must pass through untouched. + assertArrayEquals(BLOB_BYTES, result.getBytes(PRIVATE_BLOB_TAG)); + } + + @Test + public void testEncapsulatedPixelDataFragmentsUntouched() throws Exception { + BasicDicomObject dcm = new BasicDicomObject(); + dcm.putString(Tag.SOPClassUID, VR.UI, UID.SecondaryCaptureImageStorage); + dcm.putString(Tag.SOPInstanceUID, VR.UI, "1.2.3.4.5.6.7.8.10"); + + byte[] fragment = new byte[] { 9, 8, 7, 6, 5, 4 }; + DicomElement pixelData = dcm.putFragments(Tag.PixelData, VR.OB, false); + pixelData.addFragment(new byte[0]); // basic offset table + pixelData.addFragment(fragment); + + String base64 = writeDicomFile(dcm, UID.JPEGBaseline1); + + DICOMSerializer serializer = new DICOMSerializer(); + String xml = serializer.toXML(base64); + + // The fragment container must keep its wire VR: its items hold encoded text + // (backslash-hex for OB/OW), not elements, so the SQ rewrite must not touch it. + Element pixelDataElement = findElementByTag(xml, "7FE00010"); + assertNotNull("pixel data element missing from XML", pixelDataElement); + assertEquals("OB", pixelDataElement.getAttribute("vr")); + + String roundTripped = serializer.fromXML(xml); + + DicomObject result = DICOMConverter.byteArrayToDicomObject(Base64.decodeBase64(roundTripped), false); + DicomElement resultPixelData = result.get(Tag.PixelData); + assertNotNull("pixel data lost in round trip", resultPixelData); + assertTrue("pixel data no longer encapsulated", resultPixelData.hasFragments()); + assertArrayEquals(fragment, resultPixelData.getFragment(1)); + } + + 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()); + } + + private boolean hasElementChild(Node node) { + for (Node child = node.getFirstChild(); child != null; child = child.getNextSibling()) { + if (child.getNodeType() == Node.ELEMENT_NODE) { + return true; + } + } + + return false; + } + + private Element findElementByTag(String xml, String tag) throws Exception { + DocumentBuilderFactory factory = DocumentBuilderFactory.newInstance(); + factory.setFeature("http://apache.org/xml/features/disallow-doctype-decl", true); + Document document = factory.newDocumentBuilder().parse(new ByteArrayInputStream(xml.getBytes(StandardCharsets.UTF_8))); + NodeList elements = document.getElementsByTagName("*"); + + for (int i = 0; i < elements.getLength(); i++) { + Element element = (Element) elements.item(i); + + if (tag.equalsIgnoreCase(element.getAttribute("tag"))) { + return element; + } + } + + return null; + } +}