Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Non-conforming command sets raise an exception #554

Closed
or150 opened this issue Oct 22, 2020 · 2 comments
Closed

Non-conforming command sets raise an exception #554

or150 opened this issue Oct 22, 2020 · 2 comments

Comments

@or150
Copy link
Contributor

or150 commented Oct 22, 2020

Describe the bug
Hi,

I am working with a PACS that has a weird implementation bug. It returns two NumberOf*Suboperations fields in C-Move responses.
When parsing the command set, pydicom parses it as MultiValue, and this in turn causes pydicom to raise:
TypeError: Number of Remaining Suboperations must be an int

Expected behavior
We are trying to migrate from dcmtk/dcm4che3 cli utilities, and encountered this bug only here.

From looking at both implementations, it seems that dcmtk takes the first value:
https://github.com/DCMTK/dcmtk/blob/a9561f364d50adf5344d45e01fda0b2ae784877f/dcmnet/libsrc/dimcmd.cc#L1044
https://github.com/DCMTK/dcmtk/blob/a9561f364d50adf5344d45e01fda0b2ae784877f/dcmnet/libsrc/dimcmd.cc#L280

And if I didn't miss anything, dcm4che3 seems to completely ignore it (checks only the Status field, and takes the first value there, too):
https://github.com/dcm4che/dcm4che/blob/5b3f94cc8816490e6e0cf5dc82516901b828fecf/dcm4che-tool/dcm4che-tool-movescu/src/main/java/org/dcm4che3/tool/movescu/MoveSCU.java#L357

Since both are popular implementations, I assume that taking the first value is ok for these cases.

I have a fork that before setting any field of a DIMSEPrimitive from the command dataset, verifies the VM of the tag against dictionary_VM and takes the first value if needed. If this solution is acceptable, I can open a PR.

Steps To Reproduce

from pynetdicom.pdu_primitives import P_DATA
from pynetdicom.dimse_messages import C_MOVE_RSP

def reproduce():
    cmd_set = b'\x03\x00\x00\x00\x00\x04\x00\x00\x00\x72' \
              b'\x00\x00\x00\x00\x00\x02\x00\x1a\x00\x00' \
              b'\x00\x31\x2e\x32\x2e\x38\x34\x30\x2e\x31' \
              b'\x30\x30\x30\x38\x2e\x35\x2e\x31\x2e\x34' \
              b'\x2e\x31\x2e\x31\x2e\x32\x00\x00\x00\x00' \
              b'\x01\x02\x00\x00\x00\x21\x80\x00\x00\x20' \
              b'\x01\x02\x00\x00\x00\x05\x00\x00\x00\x00' \
              b'\x08\x02\x00\x00\x00\x01\x00\x00\x00\x00' \
              b'\x09\x02\x00\x00\x00\x00\xff\x00\x00\x20' \
              b'\x10\x04\x00\x00\x00\x03\x00\x00\x00\x00' \
              b'\x00\x21\x10\x04\x00\x00\x00\x01\x00\x00' \
              b'\x00\x00\x00\x22\x10\x04\x00\x00\x00\x02' \
              b'\x00\x00\x00\x00\x00\x23\x10\x04\x00\x00' \
              b'\x00\x04\x00\x00\x00'

    data_set = b'\x02\x08\x00\x52\x00\x08\x00\x00\x00\x50' \
               b'\x41\x54\x49\x45\x4e\x54\x20\x10\x00\x20' \
               b'\x00\x02\x00\x00\x00\x2a\x20'

    msg = C_MOVE_RSP()
    for data in [cmd_set, data_set]:
        p_data = P_DATA()
        p_data.presentation_data_value_list.append([0, data])
        msg.decode_msg(p_data)
    primitive = msg.message_to_primitive()


reproduce()

Your environment
Almost Latest pynetdicom development code (93ef29c)
pydicom 2.0.0
python 3.7

@scaramallion
Copy link
Member

scaramallion commented Oct 23, 2020

It's probably easier to assume the VM should be 1 as only Offending Element and Attribute Identifier List are 1-n

This should be faster than the dictionary_VM lookup

for elem in self.command_set:
    if hasattr(primitive, elem.keyword):
        if elem.VM > 1 and elem.tag not in [0x00000901, 0x00001005]:
            LOGGER.warning(f"Non-conformant VM {elem.VM} for '{elem.keyword}', taking the first value")
            elem.value = elem.value[0]
        setattr(primitive, elem.keyword, elem.value)

If you want to submit a PR that'd be great, otherwise I'm happy to do it.

@or150
Copy link
Contributor Author

or150 commented Oct 25, 2020

Opened a PR.
One of the tests broke, and I'm not sure that the that case is valid anymore.
I'd be glad to get feedback about it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants