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

PDU primitives reason_str() throwing KeyError #633

Closed
richard-moss opened this issue Jun 3, 2021 · 5 comments
Closed

PDU primitives reason_str() throwing KeyError #633

richard-moss opened this issue Jun 3, 2021 · 5 comments

Comments

@richard-moss
Copy link

Problem

pdu_primitives.reason_str seeming to throw a KeyError, rather than text of problem:

image004

Expected Behavior

Shouldn't throw a KeyError

(actually maybe should validate type when passing ae_title to either AE() or AE.associate().

Curiously, I hadn't had this problem using version 1.5.6, even though I was passing strings for ae_title to both AE() and associate()
...was there a change in moving from 1.5.6 to 1.5.7 that would now start failing if a string was passed instead of a bytes?

Sorry....

sorry! ...this was a bug report from a user who had just ran an updated bit of code from me. So I haven't had a chance yet to try and reproduce.

Steps To Reproduce (presumably...)

The code in question is an exe via pyinstaller, and I'd just given them a new version I'd built after I'd upgraded my side from pynetdicom 1.5.6 -> 1.5.7.

The trigger seemed to be from me calling AE() with a string instead of bytes for the ae_title and/or me subsequently likewise using a string instead of bytes when calling associate.

My code previously ...subsequently changed to pass bytes instead of strings (though haven't had a chance to hear from customer whether this resolved the problem)

    #  CALLING_AE_TITLE defined previously as string
    ae = AE(ae_title=CALLING_AE_TITLE)
    ae.add_requested_context(VerificationSOPClass, transfer_syntax)

    ae.acse_timeout = ACSE_TIMEOUT
    ae.dimse_timeout = DIMSE_TIMEOUT
    ae.network_timeout = NETWORK_TIMEOUT

    association = ae.associate(host, port, ae_title='ANY-SCP')

Environment

Sorry can't run as required, but running inside a pyinstaller exe on windows 10 with key info as:

altgraph                  0.17               pyhd8ed1ab_0    conda-forge
ansicon                   1.89.0           py38haa244fe_3    conda-forge
astroid                   2.5.7            py38haa244fe_0    conda-forge
backports                 1.0                        py_2    conda-forge
backports.functools_lru_cache 1.6.4              pyhd8ed1ab_0    conda-forge
blessed                   1.18.0           py38haa244fe_0    conda-forge
brotlipy                  0.7.0           py38h294d835_1001    conda-forge
ca-certificates           2021.5.30            h5b45459_0    conda-forge
certifi                   2021.5.30        py38haa244fe_0    conda-forge
cffi                      1.14.5           py38hd8c33c5_0    conda-forge
chardet                   4.0.0            py38haa244fe_1    conda-forge
colorama                  0.4.4              pyh9f0ad1d_0    conda-forge
cryptography              3.4.7            py38hd7da0ea_0    conda-forge
future                    0.18.2           py38haa244fe_3    conda-forge
idna                      2.10               pyh9f0ad1d_0    conda-forge
isort                     5.8.0              pyhd8ed1ab_0    conda-forge
jinxed                    1.1.0            py38haa244fe_0    conda-forge
lazy-object-proxy         1.6.0            py38h294d835_0    conda-forge
macholib                  1.14               pyh9f0ad1d_1    conda-forge
mccabe                    0.6.1                      py_1    conda-forge
openssl                   1.1.1k               h8ffe710_0    conda-forge
pefile                    2021.5.24          pyhd8ed1ab_0    conda-forge
pip                       21.1.2             pyhd8ed1ab_0    conda-forge
pycparser                 2.20               pyh9f0ad1d_2    conda-forge
pydicom                   2.1.2              pyhd3deb0d_0    conda-forge
pyinstaller               4.3              py38hd0d6af5_0    conda-forge
pyinstaller-hooks-contrib 2020.11            pyhd8ed1ab_0    conda-forge
pylint                    2.7.2            py38haa244fe_0    conda-forge
pynetdicom                1.5.7              pyhd8ed1ab_0    conda-forge
pyopenssl                 20.0.1             pyhd8ed1ab_0    conda-forge
pysocks                   1.7.1            py38haa244fe_3    conda-forge
python                    3.8.10          h7840368_1_cpython    conda-forge
python-json-logger        2.0.1              pyh9f0ad1d_0    conda-forge
python_abi                3.8                      1_cp38    conda-forge
pywin32                   300              py38h294d835_0    conda-forge
pywin32-ctypes            0.2.0           py38haa244fe_1003    conda-forge
requests                  2.25.1             pyhd3deb0d_0    conda-forge
setuptools                49.6.0           py38haa244fe_3    conda-forge
six                       1.16.0             pyh6c4a22f_0    conda-forge
sqlite                    3.35.5               h8ffe710_0    conda-forge
toml                      0.10.2             pyhd8ed1ab_0    conda-forge
urllib3                   1.26.5             pyhd8ed1ab_0    conda-forge
vc                        14.2                 hb210afc_4    conda-forge
vs2015_runtime            14.28.29325          h5e1d092_4    conda-forge
wcwidth                   0.2.5              pyh9f0ad1d_2    conda-forge
wheel                     0.36.2             pyhd3deb0d_0    conda-forge
win_inet_pton             1.1.0            py38haa244fe_2    conda-forge
wincertstore              0.2             py38haa244fe_1006    conda-forge
wrapt                     1.12.1           py38h294d835_3    conda-forge
zlib                      1.2.11            h62dcd97_1010    conda-forge    
@scaramallion
Copy link
Member

scaramallion commented Jun 3, 2021

ae_title 'should' be bytes, but v1.5.7 should handle setting using a string anyway as it gets passed through utils.validate_ae_title() when both setting the AE attribute and through associate().

The only change between 1.5.6 and 1.5.7 was a fix for the QR SCP. It's possible that in backporting one of the fixes I've forgotten to take into account Python 2's str usage. But looking through commits the only thing I see that might be related is #563 but that's for A-ABORT/A-P-ABORT.

What is the actual value that's being used for the AE title? It must be ASCII encodable, so if they've changed it to something else that could trigger weird behaviour.

@scaramallion
Copy link
Member

I don't support you could get a copy of the debugging log output?

@scaramallion
Copy link
Member

scaramallion commented Jun 3, 2021

Hmm, if the key error is 7, then it seems likely that its trying to hit the "Called AE title not recognised" diagnostic response for a rejected association, but the source must be 1 (or alternatively the source is 7, which is wildly non-conformant).

Yeah, the ACSE trigger is:

elif hasattr(rsp, 'result') and rsp.result in [0x01, 0x02]:
    # 0x01 is rejected (permanent)                
    # 0x02 is rejected (transient)                
    LOGGER.error('Association Rejected')                
    LOGGER.error(                    
        'Result: {}, Source: {}'                    
        .format(rsp.result_str, rsp.source_str)                
    )                
    LOGGER.error('Reason: {}'.format(rsp.reason_str))

I'd say the association request is rejected due to an unrecognised Called AE Title but the peer is responding with a non-conformant A-ASSOCIATE-RJ PDU with a Source of 2 but a Reason/Diagnostic value of 7.

The other mystery is why using str for the called AE title results in it being unrecognised while bytes is OK... the value is encoded to bytes here (and also passed through the validator again for good measure).

Also I should add a (better) validator for the A-ASSOCIATE-RJ's diagnostic. And make sure exceptions during ACSE negotiations are handled gracefully

@richard-moss
Copy link
Author

richard-moss commented Jun 3, 2021

Sorry - I should have said that I also traced it to a (likely) 'Called AE title not recognized' entry in the table. ...this was the reason for my assumption the problem was passing in a string instead of bytes when I specified the called title to associate(). ...but that could be a wrong assumption!

I'll do a bit more digging and let you know what I find, but some other key info:

  • I've sent an updated version to my client, with bytes specified instead of str. but I still haven't heard back if that fixed the problem.
  • I myself haven't reproduced, so it's possible my change in my code won't fix the problem
  • both the peer and client are pynetdicom.
  • I didn't yet update the peer side to use bytes instead of strings when setting things up there (which I will)
  • the called AE title I'd specified as 'AQS_gateway'. Previously as a string, but then changed to be bytes.
  • I think off the top of my head trying to remember the rules around them, both calling and called AE titles are valid

...So I'll also do a bit more digging tonight or tomorrow for you to try and nail down when and why exactly this was happening (sorry haven't done it yet - wanted to file the issue first)

@scaramallion
Copy link
Member

No problem, let me know what you find out

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