fix(DicomMessage): parse UN-encoded sequences using Implicit VR LE per PS3.5 §6.2.2#492
fix(DicomMessage): parse UN-encoded sequences using Implicit VR LE per PS3.5 §6.2.2#492jimOnAir wants to merge 1 commit into
Conversation
…r PS3.5 §6.2.2
When an Explicit VR file encodes a sequence element with VR=UN and a
defined length, DICOM PS3.5 §6.2.2 requires that its items be parsed
using Implicit VR Little Endian regardless of the file's transfer syntax.
Without this fix, dcmjs tried to interpret the item bytes as Explicit VR,
emitted 'Invalid vr type ( - using UN', stored the value as [{}], and then
on write inflated the element length to ~843 MB (the 843 MB inflation bug
reported for files containing UDISequence (0018,100A)).
The fix records the literal wire VR before any dictionary override
(wireVrType), then after reading the length checks whether the payload
starts with the FFFE item-delimiter group bytes (FE FF in LE). If so,
the element is re-typed as SQ and the recursive item read is forced to
IMPLICIT_LITTLE_ENDIAN.
Adds a regression test that builds a hand-crafted Explicit VR buffer with
UDISequence (0018,100A) encoded as UN and verifies the inner
UniqueDeviceIdentifier (0018,1009) value is correctly recovered.
✅ Deploy Preview for dcmjs2 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Thanks for the nice PR, thanks. I was totally unaware of this particular scenario. I assume this arose in practice with a real datafile? I'm guessing you can't share it, but maybe you can describe the equipment / vendor that it came from? |
|
Hi. Here is data from the image: |
|
@wayfarer3130 When you have time, could you please review this PR? |
wayfarer3130
left a comment
wayfarer3130
left a comment
There was a problem hiding this comment.
I have some additional tests on this, plus the fix for the AsyncDicomReader side of things. Could you give me write access as a maintainer to push to your PR so that I can incorporate the other changes? Then I can go ahead and approve it after pushing the additional fixes.
|
I added some additional changes in #496 to fix it more broadly and have a bit mroe comprehensive tests. |
3 participants
Problem
When a DICOM file encodes a sequence element with
VR=UNand a defined length (valid per DICOM PS3.5 §6.2.2), dcmjs fails to parse it correctly.Root cause
DICOM PS3.5 §6.2.2 requires that when a sequence is stored with
VR=UNand a defined length, its items must be parsed using Implicit VR Little Endian, regardless of the file's overall transfer syntax.dcmjs did not implement this rule. It attempted to read the item bytes as Explicit VR, encountered
28 00as the VR string (the character(+ NUL), emittedInvalid vr type ( - using UN, and stored the value as[{}].Consequence
On write-back, the corrupt
[{}]payload caused a ~843 MB length field to be emitted for the element (e.g. files containingUDISequence (0018,100A)), inflating an 788 KB file to 843 MB.Fix
In
DicomMessage._readTag():wireVrType) immediately afterstream.readVR(), before any dictionary override changesvrType.wireVrType === "UN"and the payload starts withFE FF(the little-endian group bytes of the DICOM item-start tag0xFFFEE000), promote the element toSQand switch the recursive read toIMPLICIT_LITTLE_ENDIAN.The
wireVrTypeapproach correctly handles both:vr.typestays"UN"UDISequence (0018,100A)— where the existing dictionary-override already promotesvrTypeto"SQ"before our check runsTest
Adds a regression test that constructs a hand-crafted Explicit VR Little Endian buffer encoding
UDISequence (0018,100A)asVR=UNwith a defined length, containing one Implicit VR item withUniqueDeviceIdentifier (0018,1009). Verifies:SQ[{}])