search

pydicom / pynetdicom

A Python implementation of the DICOM networking protocol
https://pydicom.github.io/pynetdicom
MIT License
511 stars 180 forks source link

Large number of DIMSE sub-operations leads to integer overflow #982

Closed denesd closed 2 days ago

denesd commented 1 week ago

Describe the bug A large number of sub-operations can cause an integer overflow when the values are returned.

Example code block:

@property
def _NumberOfCompletedSuboperations(self) -> Optional[int]:
    """Return the *Number of Completed Suboperations*."""
    return self._number_of_completed_suboperations

This happens also for NumberOfFailedSuboperations, NumberOfRemainingSuboperations, NumberOfWarningSuboperations

Reference link

Expected behavior Returning sub-operations should not cause integer overflow error.

Current workaround is using a fork with the following fix:

def _fix_num_of_subops(num_of_subops):
    if num_of_subops:
        return num_of_subops % 65535
    return num_of_subops

@property
def NumberOfCompletedSuboperations(self) -> Optional[int]:
    """Get or set the *Number of Completed Suboperations* as :class:`int`."""
    return _fix_num_of_subops(self._NumberOfCompletedSuboperations)

Steps To Reproduce C-Move with a large Study

scaramallion commented 5 days ago

Can you post the traceback please? I'm fairly certain as to the cause but would like to confirm.

This is with you acting as the Move SCP, correct?

denesd commented 5 days ago 
{"message": "With tag (0000, 1020) got exception: ushort format requires 0 <= number <= (0x7fff * 2 + 1)\nfor data_element:\n(0000, 1020) Number of Remaining Sub-operations  US: 170251\nTraceback (most recent call last):\n  File \"/usr/local/lib/python3.7/site-packages/pydicom/filewriter.py\", line 235, in write_numbers\n    value.append  # works only if list, not if string or number\nAttributeError: 'int' object has no attribute 'append'\n\nDuring handling of the above exception, another exception occurred:\n\nTraceback (most recent call last):\n  File \"/usr/local/lib/python3.7/site-packages/pydicom/filewriter.py\", line 237, in write_numbers\n    fp.write(pack(format_string, value))\nstruct.error: ushort format requires 0 <= number <= (0x7fff * 2 + 1)\n\nDuring handling of the above exception, another exception occurred:\n\nTraceback (most recent call last):\n  File \"/usr/local/lib/python3.7/site-packages/pydicom/tag.py\", line 27, in tag_in_exception\n    yield\n  File \"/usr/local/lib/python3.7/site-packages/pydicom/filewriter.py\", line 552, in write_dataset\n    write_data_element(fp, dataset.get_item(tag), dataset_encoding)\n  File \"/usr/local/lib/python3.7/site-packages/pydicom/filewriter.py\", line 481, in write_data_element\n    writer_function(buffer, data_element, writer_param)\n  File \"/usr/local/lib/python3.7/site-packages/pydicom/filewriter.py\", line 243, in write_numbers\n    \"{0}\\nfor data_element:\\n{1}\".format(str(e), str(data_element)))\nOSError: ushort format requires 0 <= number <= (0x7fff * 2 + 1)\nfor data_element:\n(0000, 1020) Number of Remaining Sub-operations  US: 170251\n", "severity": "ERROR", "timestamp": "2020-10-26 15:32:59.119647", "process": 1, "filename": "dsutils.py", "lineno": 105, "thread": 140118512953088, "exc_info": "Traceback (most recent call last):\n  File \"/usr/local/lib/python3.7/site-packages/pydicom/filewriter.py\", line 235, in write_numbers\n    value.append  # works only if list, not if string or number\nAttributeError: 'int' object has no attribute 'append'\n\nDuring handling of the above exception, another exception occurred:\n\nTraceback (most recent call last):\n  File \"/usr/local/lib/python3.7/site-packages/pydicom/filewriter.py\", line 237, in write_numbers\n    fp.write(pack(format_string, value))\nstruct.error: ushort format requires 0 <= number <= (0x7fff * 2 + 1)\n\nDuring handling of the above exception, another exception occurred:\n\nTraceback (most recent call last):\n  File \"/usr/local/lib/python3.7/site-packages/pydicom/tag.py\", line 27, in tag_in_exception\n    yield\n  File \"/usr/local/lib/python3.7/site-packages/pydicom/filewriter.py\", line 552, in write_dataset\n    write_data_element(fp, dataset.get_item(tag), dataset_encoding)\n  File \"/usr/local/lib/python3.7/site-packages/pydicom/filewriter.py\", line 481, in write_data_element\n    writer_function(buffer, data_element, writer_param)\n  File \"/usr/local/lib/python3.7/site-packages/pydicom/filewriter.py\", line 243, in write_numbers\n    \"{0}\\nfor data_element:\\n{1}\".format(str(e), str(data_element)))\nOSError: ushort format requires 0 <= number <= (0x7fff * 2 + 1)\nfor data_element:\n(0000, 1020) Number of Remaining Sub-operations  US: 170251\n\nDuring handling of the above exception, another exception occurred:\n\nTraceback (most recent call last):\n  File \"/usr/local/lib/python3.7/site-packages/pynetdicom/dsutils.py\", line 102, in encode\n    write_dataset(fp, ds)\n  File \"/usr/local/lib/python3.7/site-packages/pydicom/filewriter.py\", line 552, in write_dataset\n    write_data_element(fp, dataset.get_item(tag), dataset_encoding)\n  File \"/usr/local/lib/python3.7/contextlib.py\", line 130, in __exit__\n    self.gen.throw(type, value, traceback)\n  File \"/usr/local/lib/python3.7/site-packages/pydicom/tag.py\", line 34, in tag_in_exception\n    raise type(ex)(msg)\nOSError: With tag (0000, 1020) got exception: ushort format requires 0 <= number <= (0x7fff * 2 + 1)\nfor data_element:\n(0000, 1020) Number of Remaining Sub-operations  US: 170251\nTraceback (most recent call last):\n  File \"/usr/local/lib/python3.7/site-packages/pydicom/filewriter.py\", line 235, in write_numbers\n    value.append  # works only if list, not if string or number\nAttributeError: 'int' object has no attribute 'append'\n\nDuring handling of the above exception, another exception occurred:\n\nTraceback (most recent call last):\n  File \"/usr/local/lib/python3.7/site-packages/pydicom/filewriter.py\", line 237, in write_numbers\n    fp.write(pack(format_string, value))\nstruct.error: ushort format requires 0 <= number <= (0x7fff * 2 + 1)\n\nDuring handling of the above exception, another exception occurred:\n\nTraceback (most recent call last):\n  File \"/usr/local/lib/python3.7/site-packages/pydicom/tag.py\", line 27, in tag_in_exception\n    yield\n  File \"/usr/local/lib/python3.7/site-packages/pydicom/filewriter.py\", line 552, in write_dataset\n    write_data_element(fp, dataset.get_item(tag), dataset_encoding)\n  File \"/usr/local/lib/python3.7/site-packages/pydicom/filewriter.py\", line 481, in write_data_element\n    writer_function(buffer, data_element, writer_param)\n  File \"/usr/local/lib/python3.7/site-packages/pydicom/filewriter.py\", line 243, in write_numbers\n    \"{0}\\nfor data_element:\\n{1}\".format(str(e), str(data_element)))\nOSError: ushort format requires 0 <= number <= (0x7fff * 2 + 1)\nfor data_element:\n(0000, 1020) Number of Remaining Sub-operations  US: 170251\n"}

Sorry about the format, this is the only way I could extract it.

Yes, this occurred on a SCP server.

scaramallion commented 2 days ago

The following elements all use a VR of US, which should be a 2-byte unsigned integer, range [0, 65535]:

The requirements for Query/Retrieve C-MOVE include this in Section C.1.4 (C-GET has similar requirements):

The SCP may optionally generate responses to the C-MOVE with status equal to Pending during the processing of the C-STORE sub-operations. These C-MOVE responses indicate the number of Remaining C-STORE sub-operations and the number of C-STORE sub-operations returning the status of Success, Warning, and Failed.

When the number of Remaining C-STORE sub-operations reaches zero, the SCP generates a final response with a status equal to Success, Warning, Failed, or Refused. This response may indicate the number of C-STORE sub-operations returning the status of Success, Warning, and Failed. If the status of a C-STORE sub-operation was Failed a UID List will be returned.

But then it also includes this requirement in Section C.4.2.3:

When the number of Remaining sub-operations reaches zero, the SCP shall generate a final response with a status equal to Success, Warning, Failure, or Refused. This response shall indicate the number of Completed sub-operations, the number of Failed sub-operations, and the number of sub-operations with Warning Status.

And in Section C.4.2.2:

The SCU shall interpret responses with a status equal to Success, Warning, Failure, or Refused as final responses. The final response shall indicate the number of Successful C-STORE sub-operations and the number of Failed C-STORE sub-operations resulting from the C-MOVE operation.

However in Section C.4.2.1.7 we have:

Canceled, Warning, Failure, or Success may contain the Number of Completed Sub-operations Attribute

In Section C.4.2.1.8:

Canceled, Warning, Failure, or Success may contain the Number of Failed Sub-operations Attribute

In Section C.4.2.1.9:

Canceled, Warning, Failure, or Success may contain the Number of Warning Sub-operations Attribute

All my emphasis. So if the number of matches is greater than 65535 then we can work around this by not sending Pending sub-operation status responses, but it's not completely clear if we must include the sub-operations summary in the final response, although I lean towards it being required given C.4.2.2 and C.4.2.3 are explicit descriptions for how a MOVE SCU and SCP are expected to behave.

Cycling the sub-operations values is not a great idea as some SCUs may rely on them if they're present.

One option is that since you're the SCP you can simply limit the number of allowed responses to less than 65535 (which I believe is what most QR SCPs do anyway to prevent resource mismanagement).

scaramallion commented 2 days ago

At this point I'm mostly leaning towards simply disallowing more than 65535 responses with a suitable exception message and failure response status.