storescp crash while receiving a corrupt dataset

[Yves Neumann]

Hello OFFIS team,

We are currently facing a problem in a storescp-derivate (store SCP of K-PACS/iQ-VIEW) and files created by a FUJI CR (device model unknown). First of all, I know the file are corrupt and this should be corrected by the FUJI guys. Anyway ...

The server crashes abnormal during receiving the dataset. The SCP runs under Windows (XP in this case). The crash is reproducable and the call stack before the termination is:

• >IQSERVER.exe!operator new(unsigned int size=12) Line 64
  >IQSERVER.exe!DcmList::insert(DcmObject * obj=0x014178e8, E_ListPos pos=ELP_next) Line 175 + 0x7 bytes
  >IQSERVER.exe!DcmItem::insert(DcmElement * elem=0x014178e8, bool replaceOld=false, bool checkInsertOrder=true) Line 1249
  >IQSERVER.exe!DcmItem::readSubElement(DcmInputStream & inStream={...}, DcmTag & newTag={...}, const unsigned long newLength=4294967295, E_TransferSyntax xfer=EXS_LittleEndianImplicit, E_GrpLenEncoding glenc=EGL_noChange, const unsigned long maxReadLength=4096) Line 883 + 0x19 bytes
  >IQSERVER.exe!DcmItem::read(DcmInputStream & inStream={...}, E_TransferSyntax xfer=EXS_LittleEndianImplicit, E_GrpLenEncoding glenc=EGL_noChange, const unsigned long maxReadLength=4096) Line 974 + 0x24 bytes
  >IQSERVER.exe!DcmDataset::read(DcmInputStream & inStream={...}, E_TransferSyntax xfer=EXS_LittleEndianImplicit, E_GrpLenEncoding glenc=EGL_noChange, const unsigned long maxReadLength=4096) Line 288 + 0x1f bytes
  >IQSERVER.exe!DIMSE_receiveDataSetInMemory(T_ASC_Association * assoc=0x01414ed8, T_DIMSE_BlockingMode blocking=DIMSE_BLOCKING, int timeout=0, unsigned char * presID=0x013dde03, DcmDataset * * dataObject=0x013de01c, void (void *, unsigned long)* callback=0x0077a060, void * callbackData=0x013dddcc) Line 1695 + 0x26 bytes

Debugging the case shows that reading the sub-element in DcmItem::readSubElement ( the call to subElem->read(..) before returns ECC_StreamNotifyClient - "I/O suspension or premature end of stream") tries to insert the element into the Element-List but crashes within the DcmList when allocating memory for the new element. The length of the element is FFFFFFFF (4294967295) which obviously is wrong. By the way, a hex dump on the dataset shows it is having FFFF FFFF in the length field of element (0008,1070).

The problem here is not that the stream content is corrupt (this should lead to an abortion) but that the implementation does not handle the situation and crashes.

Just wanted to report this ... I can also provide a sample dataset if you want. Maybe this report leads to a more robust implementation.

[Jörg Riesmeier]

Thank you for the report. I've added this issue to our to-do list ...

[Per]

Could you please upload a sample data set somewhere, or send it to me directly?

[Shaeto]

i fixed similar bug by this patch:

==============================================

--- dcmdata/libsrc/dcitem.cc 2005-12-08 16:41:16.000000000 +0300
+++ dcmdata/libsrc/dcitem.cc 2009-01-21 16:11:16.859375000 +0300
@@ -778,6 +778,17 @@
inStream.read(&valueLength, 4); //length field is 4 bytes wide
swapIfNecessary(gLocalByteOrder, byteOrder, &valueLength, 4, 4);
bytesRead += 4;
+
+ /* Shaeto: fix for some strange cases with SQ elements in private groups. */
+ /* for example (0507,0001) from 'Orex PcCR 1417' modality */
+ /* by default dcmtk recognizes it as PrivateGroupLengthToEnd but it is sequence. */
+ /* dcmtk failed later on reading */
+ /* as temp solution you can modify dicom.dic and add that bogus private tag to list */
+ /* of standard items */
+ if (nxtobj != EVR_SQ && (valueLength == OFstatic_cast(Uint32, -1)) && (newTag.getGroup() & 1)) {
+ newTag.setVR(EVR_UN);
+ nxtobj = newTag.getEVR();
+ }
} else { //the transfer syntax is explicit VR
DcmVR vr(newTag.getEVR());
if (vr.usesExtendedLengthEncoding())

==============================================

[Michael Onken]

Hi Shaeto,

could you provide an example dataset to dicom (at) offis (dot) de? That helps for including and testing the proposed patch.

Thank you, Regards,
Michael

[Shaeto]

sent...

[Michael Onken]

Hi,

thank you for sending. It's actually about the PrivateGroupLengthToEnd attribute which is part of the dicom.dic. As another team member (Marco) already pointed out in our TODO list, this attribute (and also IllegalGroupLengthToEnd and GenericGroupLengthToEnd) will probably removed from dicom.dic, because they don't have been ever part of DICOM or ACR/NEMA as it seems. ( - I have no idea then, how they got into dicom.dic )

Anyway, the easiest thing you could do is just to remove those attributes from the dictionary yourself meanwhile. I think will remove the attributes next days but I want to talk about this with Marco first, I guess he did not already do that because he wanted to re-check this assumption or he just did not have the time.

Regards,
Michael

[Shaeto]

as i see someone removed lines
(0009-o-ffff,0001) UL PrivateGroupLengthToEnd 1 PRIVATE
(0001-o-0007,0001) UL IllegalGroupLengthToEnd 1 ILLEGAL

from dicom.dic in the latest snapshot

what do you think about
(0009-o-ffff,0010-u-00ff) LO PrivateCreator 1 PRIVATE

if private creator will place SQ to that range instead of LO ? for example
(0507, 0011)

[Michael Onken]

Hi,

oops, then my colleague just forgot to mark that in the TODO list But ok, even better that it's already done.

what do you think about
(0009-o-ffff,0010-u-00ff) LO PrivateCreator 1 PRIVATE

if private creator will place SQ to that range instead of LO ? for example
(0507, 0011)

Thats illegal in DICOM I guess, private creator elements (00gg,00ee) with gg being odd and ee betwen 10 and FF are always of VR=LO. But I did not check the parser's behaviour in that case. Did you already do ?

Regards,
Michael

[Shaeto]

yes, it will read 0xffffffff as LO (which really SQ) length and fail later on memory allocation and reading... take a look at topic starter message, stack dump will be very similar

[Shaeto]

btw if we checking private.dic

(0027,"SVISION",13) DT NewestStudy 1

definitely not a LO
etc etc alot examples in the private dictionary

(0009,"CARDIO-D.R. 1.0",40) SQ AlternateImageSequence 1

candidate for crash

[Michael Onken]

Hi Shaeto,

btw if we checking private.dic

(0027,"SVISION",13) DT NewestStudy 1

These are normal private tags (in that case of creator "SVISION"); of course not every private tag has to have a VR of LO. Reading part 5 of the standard about the reservation mechanisms using private creator attributes might shed some light on this topic.

Regards,
Michael

[Shaeto]

agree! but if dcmtk finds "unknown tag" in 10-ff range and try to load it from dataset it will crash because trying to load LO with length 0xffffffff

[Michael Onken]

Yes, that is still a problem - but i dont understand the the connection between the normal private (non-reservation) tags you posted above. But maybe I'm just too confused at the moment.

By the way, I already corrected dcmdata routines so far, that they do not crash when reading such datasets. I'm now adding an parser flag which allows for handling private, maximum length attributes as sequences. That already works for reading (e. g. dcmdump), but still must be completed for writing such datasets (tested with dcmconv). However, the crashes are gone. I will publish a snapshot as far I have something worth to share.

Regards,
Michael

[Michael Onken]

Hi Shaeto, Per, Yve,

I've uploaded a snapshot (to the well-know location) which includes two enhancements:

1) It offers a new global parsing flag which can be set to allow for parsing private attributes, in implicit VR, with maximum length value as a sequence, regardless of what the dictionary says.

2) It prevents dcmdata's parsing from crashing even if the above flag is not activated. However, then an error is reported if the (incorrectly encoded) file cannot be parsed succesfully.

For testing and looking how the global flag can be enabled, take a look at dcmconv and dcmdump, both having a new option for this purpose. I would be happy if you could do some testing on your data and provide errors in case there are some.

Please note that when successfully reading a file with dcmdata (which contains a sequence that only could be read by enabling the above flag), and you then force to dcmdata to write the file in implicit VR using _explicit_ length encoding, then after writing dcmdump (e. g. ) has no chance to recognize the sequence (instead of the attribute being defined in the dictionary) because the undefined length is gone. dcmdump then _must_ rely on the dictionary and will try parsing and printing the value as it is not of type sequence. I have not altered the writing routines by adding a flag to write only explicit length for that sequence, because that would be result in pretty ugly code which - I think - the issue is not worth. Instead one can expect from the "user" to change the dictionary accordlingly or to write with undefined length.

Regards,
Michael