From 4c44e744835657afa45920fb8be710e6c64e8e9f Mon Sep 17 00:00:00 2001 From: 100029962 Date: Sun, 16 Apr 2023 20:18:58 +0200 Subject: [PATCH 1/6] [dcmnet] Extend DUL_ASSOCIATESERVICEPARAMETERS with parameters needed for cancelation of TCP connect attempts Here we extend the DUL_ASSOCIATESERVICEPARAMETERS struct with members for: * A cancelation callback function that can be implemented by client to cancel ongoing TCP connect attempts * A poll/select interval that is used to chop the poll/select timeout into shorter intervals to enable checking for cancelation. --- dcmnet/include/dcmtk/dcmnet/dul.h | 5 ++++- dcmnet/libsrc/assoc.cc | 3 +++ 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/dcmnet/include/dcmtk/dcmnet/dul.h b/dcmnet/include/dcmtk/dcmnet/dul.h index 010806719e..b01f6db8bb 100644 --- a/dcmnet/include/dcmtk/dcmnet/dul.h +++ b/dcmnet/include/dcmtk/dcmnet/dul.h @@ -202,7 +202,10 @@ typedef struct { UserIdentityNegotiationSubItemAC *ackUserIdentNeg; OFBool useSecureLayer; - Sint32 tcpConnectTimeout; + Sint32 tcpConnectTimeout; // TCP connect timeout in seconds when requesting associations + Sint32 tcpPollInterval; // Duration in seconds of each socket poll/select call during TCP connection phase. Maximum valid value is tcpConnectTimeout. + void* tcpCancelContext; // User defined data that is passed on to tcpConnectCancelled() + bool (*tcpConnectCanceled)(void*); // Called after each socket poll/select attempt to enable canceling ongoing TCP connect before the TCP connect timeout has elapsed } DUL_ASSOCIATESERVICEPARAMETERS; /** Enum describing the possible role settings for role negotiation sub items. diff --git a/dcmnet/libsrc/assoc.cc b/dcmnet/libsrc/assoc.cc index 5e250e7730..d2ed68413f 100644 --- a/dcmnet/libsrc/assoc.cc +++ b/dcmnet/libsrc/assoc.cc @@ -336,6 +336,9 @@ ASC_createAssociationParameters(T_ASC_Parameters ** params, (*params)->DULparams.useSecureLayer = OFFalse; (*params)->DULparams.tcpConnectTimeout = tcpConnectTimeout; + (*params)->DULparams.tcpPollInterval = -1; + (*params)->DULparams.tcpCancelContext = NULL; + (*params)->DULparams.tcpConnectCanceled = NULL; return EC_Normal; } From b592cdb1746415f93a9e6313a4464747c0bd0a1a Mon Sep 17 00:00:00 2001 From: 100029962 Date: Sun, 16 Apr 2023 20:45:34 +0200 Subject: [PATCH 2/6] [dcmnet] Enable cancelation of connect attempts in requestAssociationTCP This is implemented using a cancellation callback (not a cancellation flag). The reason for using a cancellation callback function is that this relieves the dul implementation from having to deal with potential data races related to a cancellation flag. It is now up to the caller to ensure appropriate synchronization of any data being involved in cancellation. --- dcmnet/libsrc/dulfsm.cc | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/dcmnet/libsrc/dulfsm.cc b/dcmnet/libsrc/dulfsm.cc index b932fbef7e..37436d95a0 100644 --- a/dcmnet/libsrc/dulfsm.cc +++ b/dcmnet/libsrc/dulfsm.cc @@ -116,6 +116,8 @@ END_EXTERN_C #include "dcmtk/dcmnet/diutil.h" #include "dcmtk/dcmnet/helpers.h" #include "dcmtk/ofstd/ofsockad.h" /* for class OFSockAddr */ +#include "dcmtk/ofstd/oftimer.h" + #include @@ -2303,6 +2305,9 @@ requestAssociationTCP(PRIVATE_NETWORKKEY ** network, if (rc < 0 && errno == EINPROGRESS) #endif { + rc = 0; + OFTimer connectTimer; + do { #ifndef DCMTK_HAVE_POLL // we're in non-blocking mode. Prepare to wait for timeout. fd_set fdSet; @@ -2316,9 +2321,18 @@ requestAssociationTCP(PRIVATE_NETWORKKEY ** network, #endif /* DCMTK_HAVE_POLL */ struct timeval timeout; - timeout.tv_sec = connectTimeout; + if (params->tcpPollInterval == -1) + timeout.tv_sec = connectTimeout; + else + timeout.tv_sec = params->tcpPollInterval; timeout.tv_usec = 0; + if (params->tcpConnectCanceled && params->tcpConnectCanceled(params->tcpCancelContext)) { + // TCP connect attempt was canceled. Flag connection as timed out + rc = 0; + break; + } + do { #ifdef DCMTK_HAVE_POLL struct pollfd pfd[] = @@ -2331,6 +2345,7 @@ requestAssociationTCP(PRIVATE_NETWORKKEY ** network, rc = select(OFstatic_cast(int, s + 1), NULL, &fdSet, NULL, &timeout); #endif } while (rc == -1 && OFStandard::getLastNetworkErrorCode().value() == DCMNET_EINTR); + } while (rc == 0 && connectTimer.getDiff() < connectTimeout); if (DCM_dcmnetLogger.isEnabledFor(OFLogger::DEBUG_LOG_LEVEL)) { From 0ea4d91d59847c4419c57ffd957b93a9ef086148 Mon Sep 17 00:00:00 2001 From: 100029962 Date: Mon, 17 Apr 2023 08:57:14 +0200 Subject: [PATCH 3/6] [dcmnet] Fix indentation after previous change --- dcmnet/libsrc/dulfsm.cc | 42 ++++++++++++++++++++--------------------- 1 file changed, 21 insertions(+), 21 deletions(-) diff --git a/dcmnet/libsrc/dulfsm.cc b/dcmnet/libsrc/dulfsm.cc index 37436d95a0..1e6c6f2e4a 100644 --- a/dcmnet/libsrc/dulfsm.cc +++ b/dcmnet/libsrc/dulfsm.cc @@ -2309,23 +2309,23 @@ requestAssociationTCP(PRIVATE_NETWORKKEY ** network, OFTimer connectTimer; do { #ifndef DCMTK_HAVE_POLL - // we're in non-blocking mode. Prepare to wait for timeout. - fd_set fdSet; - FD_ZERO(&fdSet); + // we're in non-blocking mode. Prepare to wait for timeout. + fd_set fdSet; + FD_ZERO(&fdSet); #ifdef __MINGW32__ - // on MinGW, FD_SET expects an unsigned first argument - FD_SET((unsigned int) s, &fdSet); + // on MinGW, FD_SET expects an unsigned first argument + FD_SET((unsigned int) s, &fdSet); #else - FD_SET(s, &fdSet); + FD_SET(s, &fdSet); #endif /* __MINGW32__ */ #endif /* DCMTK_HAVE_POLL */ - struct timeval timeout; - if (params->tcpPollInterval == -1) - timeout.tv_sec = connectTimeout; - else - timeout.tv_sec = params->tcpPollInterval; - timeout.tv_usec = 0; + struct timeval timeout; + if (params->tcpPollInterval == -1) + timeout.tv_sec = connectTimeout; + else + timeout.tv_sec = params->tcpPollInterval; + timeout.tv_usec = 0; if (params->tcpConnectCanceled && params->tcpConnectCanceled(params->tcpCancelContext)) { // TCP connect attempt was canceled. Flag connection as timed out @@ -2333,18 +2333,18 @@ requestAssociationTCP(PRIVATE_NETWORKKEY ** network, break; } - do { + do { #ifdef DCMTK_HAVE_POLL - struct pollfd pfd[] = - { - { s, POLLOUT, 0 } - }; - rc = poll(pfd, 1, timeout.tv_sec*1000+(timeout.tv_usec/1000)); + struct pollfd pfd[] = + { + { s, POLLOUT, 0 } + }; + rc = poll(pfd, 1, timeout.tv_sec*1000+(timeout.tv_usec/1000)); #else - // the typecast is safe because Windows ignores the first select() parameter anyway - rc = select(OFstatic_cast(int, s + 1), NULL, &fdSet, NULL, &timeout); + // the typecast is safe because Windows ignores the first select() parameter anyway + rc = select(OFstatic_cast(int, s + 1), NULL, &fdSet, NULL, &timeout); #endif - } while (rc == -1 && OFStandard::getLastNetworkErrorCode().value() == DCMNET_EINTR); + } while (rc == -1 && OFStandard::getLastNetworkErrorCode().value() == DCMNET_EINTR); } while (rc == 0 && connectTimer.getDiff() < connectTimeout); if (DCM_dcmnetLogger.isEnabledFor(OFLogger::DEBUG_LOG_LEVEL)) From 109ec547386e567c5782d7242ac549d65a2b63df Mon Sep 17 00:00:00 2001 From: 100029962 Date: Sun, 16 Apr 2023 21:11:44 +0200 Subject: [PATCH 4/6] [dcmnet] Add support for setting/getting tcp polling interval from DCMScu --- dcmnet/include/dcmtk/dcmnet/scu.h | 18 ++++++++++++++++++ dcmnet/libsrc/scu.cc | 11 +++++++++++ 2 files changed, 29 insertions(+) diff --git a/dcmnet/include/dcmtk/dcmnet/scu.h b/dcmnet/include/dcmtk/dcmnet/scu.h index 5237dfdc21..8c0a14259d 100644 --- a/dcmnet/include/dcmtk/dcmnet/scu.h +++ b/dcmnet/include/dcmtk/dcmnet/scu.h @@ -690,6 +690,15 @@ class DCMTK_DCMNET_EXPORT DcmSCU */ void setConnectionTimeout(const Sint32 connectionTimeout); + /** Set the poll interval (short timeout) used when connecting to the SCP. + * While requesting an association, the TCP connection will be checked for successful + * connection with this timeout until the full connection timeout has been reached, or + * until the connection attempt has been cancelled. + * @param pollInterval [in] poll interval in seconds, <= connectionTimeout). + * set to -1 to disable connect cancellation + */ + void setTcpPollInterval(const Sint32 pollInterval); + /** Set an association configuration file and profile to be used * @param filename [in] File name of the association configuration file * @param profile [in] Profile inside the association negotiation file @@ -786,6 +795,12 @@ class DCMTK_DCMNET_EXPORT DcmSCU */ Sint32 getConnectionTimeout() const; + /** Gets the poll interval (short timeout) that defines how frequently the SCU will check + * for cancelled connect attempt. + * @return The poll interval (in seconds) + */ + Sint32 getTcpPollInterval() const; + /** Returns the storage directory used for storing objects received with C-STORE requests * in the context of C-GET sessions. Default is empty string which refers to the current * working directory. @@ -1099,6 +1114,9 @@ class DCMTK_DCMNET_EXPORT DcmSCU /// TCP connection timeout (default: value of global dcmConnectionTimeout) Sint32 m_tcpConnectTimeout; + /// TCP poll interval (default: -1 (disabled)) + Sint32 m_tcpPollInterval; + /// Storage directory for objects received with C-STORE due to a running /// C-GET session. By default, the received objects are stored in the current /// working directory. diff --git a/dcmnet/libsrc/scu.cc b/dcmnet/libsrc/scu.cc index d32261f77f..9a0c74ed67 100644 --- a/dcmnet/libsrc/scu.cc +++ b/dcmnet/libsrc/scu.cc @@ -50,6 +50,7 @@ DcmSCU::DcmSCU() , m_dimseTimeout(0) , m_acseTimeout(30) , m_tcpConnectTimeout(dcmConnectionTimeout.get()) + , m_tcpPollInterval(-1) , m_storageDir() , m_storageMode(DCMSCU_STORAGE_DISK) , m_verbosePCMode(OFFalse) @@ -2562,6 +2563,11 @@ void DcmSCU::setConnectionTimeout(const Sint32 connectionTimeout) m_tcpConnectTimeout = connectionTimeout; } +void DcmSCU::setTcpPollInterval(const Sint32 pollInterval) +{ + m_tcpPollInterval = pollInterval; +} + void DcmSCU::setAssocConfigFileAndProfile(const OFString& filename, const OFString& profile) { m_assocConfigFilename = filename; @@ -2650,6 +2656,11 @@ Sint32 DcmSCU::getConnectionTimeout() const return m_tcpConnectTimeout; } +Sint32 DcmSCU::getTcpPollInterval() const +{ + return m_tcpPollInterval; +} + OFString DcmSCU::getStorageDir() const { return m_storageDir; From 1bd0e8b5a785213c21b2207450759d74201c83dd Mon Sep 17 00:00:00 2001 From: 100029962 Date: Mon, 17 Apr 2023 10:06:26 +0200 Subject: [PATCH 5/6] [dcmnet] Extend DcmSCU::negotiateAssociation to take a cancel token as argument We'll use this cancel token to cancel ongoing TCP connect attempts. A cancel token allows the client to decide how a request should be cancelled, for example using std::stop_token in the implementation of the ICancelToken. --- dcmnet/include/dcmtk/dcmnet/scu.h | 9 ++++++++- dcmnet/libsrc/scu.cc | 2 +- 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/dcmnet/include/dcmtk/dcmnet/scu.h b/dcmnet/include/dcmtk/dcmnet/scu.h index 8c0a14259d..9620c3c925 100644 --- a/dcmnet/include/dcmtk/dcmnet/scu.h +++ b/dcmnet/include/dcmtk/dcmnet/scu.h @@ -185,6 +185,13 @@ class DCMTK_DCMNET_EXPORT RetrieveResponse : public QRResponse RetrieveResponse& operator=(const RetrieveResponse& other); }; +class IDcmCancelToken +{ +public: + virtual ~IDcmCancelToken() {} + virtual bool IsCanceled() const = 0; +}; + /** Base class for implementing DICOM Service Class User functionality. The class offers * support for negotiating associations and sending and receiving arbitrary DIMSE messages * on that connection. DcmSCU has built-in C-ECHO support so derived classes do not have to @@ -226,7 +233,7 @@ class DCMTK_DCMNET_EXPORT DcmSCU * @return EC_Normal if negotiation was successful, otherwise error code. * NET_EC_AlreadyConnected if SCU is already connected. */ - virtual OFCondition negotiateAssociation(); + virtual OFCondition negotiateAssociation(IDcmCancelToken* tcpCancelToken = NULL); /** After negotiation association, this call returns the first usable presentation context * given the desired abstract syntax and transfer syntax diff --git a/dcmnet/libsrc/scu.cc b/dcmnet/libsrc/scu.cc index 9a0c74ed67..0d66d50cce 100644 --- a/dcmnet/libsrc/scu.cc +++ b/dcmnet/libsrc/scu.cc @@ -259,7 +259,7 @@ OFCondition DcmSCU::initNetwork() return cond; } -OFCondition DcmSCU::negotiateAssociation() +OFCondition DcmSCU::negotiateAssociation(IDcmCancelToken* tcpCancelToken) { /* Return error if SCU is already connected */ if (isConnected()) From d32854ff4fc6a0fa4a0ae3bae7a35aa3cf28da83 Mon Sep 17 00:00:00 2001 From: 100029962 Date: Mon, 17 Apr 2023 10:56:38 +0200 Subject: [PATCH 6/6] [dcmnet] Implement TCP connect cancellation support in DcmSCU --- dcmnet/include/dcmtk/dcmnet/scu.h | 8 ++- dcmnet/libsrc/scu.cc | 15 ++++ dcmnet/tests/tests.cc | 3 + dcmnet/tests/tscuscp.cc | 112 ++++++++++++++++++++++++++++++ 4 files changed, 137 insertions(+), 1 deletion(-) diff --git a/dcmnet/include/dcmtk/dcmnet/scu.h b/dcmnet/include/dcmtk/dcmnet/scu.h index 9620c3c925..6a0c55727a 100644 --- a/dcmnet/include/dcmtk/dcmnet/scu.h +++ b/dcmnet/include/dcmtk/dcmnet/scu.h @@ -230,8 +230,14 @@ class DCMTK_DCMNET_EXPORT DcmSCU /** Negotiate association by using presentation contexts and parameters as defined by * earlier function calls. If negotiation fails, there is no need to close the association * or to do anything else with this class. + * @param tcpCancelToken [in] Optional cancellation token which is checked periodically + * for cancellation. If the token is cancelled during TCP + * connect phase, the TCP connect attempts are stopped and this + * function returns failure. * @return EC_Normal if negotiation was successful, otherwise error code. - * NET_EC_AlreadyConnected if SCU is already connected. + * NET_EC_AlreadyConnected if SCU is already connected. DULC_TCPINITERROR is returned + * if the TCP connect attempt was cancelled before the connection was established + * */ virtual OFCondition negotiateAssociation(IDcmCancelToken* tcpCancelToken = NULL); diff --git a/dcmnet/libsrc/scu.cc b/dcmnet/libsrc/scu.cc index 0d66d50cce..9a515273de 100644 --- a/dcmnet/libsrc/scu.cc +++ b/dcmnet/libsrc/scu.cc @@ -32,6 +32,15 @@ #include /* for zlibVersion() */ #endif +static bool CancelTcpConnect(void* context) +{ + if (!context) + return false; + + const IDcmCancelToken* token = OFstatic_cast(const IDcmCancelToken*, context); + return token->IsCanceled(); +} + DcmSCU::DcmSCU() : m_assoc(NULL) , m_net(NULL) @@ -129,6 +138,9 @@ OFCondition DcmSCU::initNetwork() return cond; } + m_params->DULparams.tcpPollInterval = m_tcpPollInterval; + m_params->DULparams.tcpConnectCanceled = CancelTcpConnect; + /* sets this application's title and the called application's title in the params */ /* structure. The default values are "ANY-SCU" and "ANY-SCP". */ ASC_setAPTitles(m_params, m_ourAETitle.c_str(), m_peerAETitle.c_str(), NULL); @@ -272,6 +284,9 @@ OFCondition DcmSCU::negotiateAssociation(IDcmCancelToken* tcpCancelToken) else DCMNET_DEBUG("Request Parameters:" << OFendl << ASC_dumpParameters(tempStr, m_params, ASC_ASSOC_RQ)); + + m_params->DULparams.tcpCancelContext = tcpCancelToken; + /* create association, i.e. try to establish a network connection to another */ /* DICOM application. This call creates an instance of T_ASC_Association*. */ DCMNET_INFO("Requesting Association"); diff --git a/dcmnet/tests/tests.cc b/dcmnet/tests/tests.cc index e7a2b438ba..aa04e6ec17 100644 --- a/dcmnet/tests/tests.cc +++ b/dcmnet/tests/tests.cc @@ -51,6 +51,9 @@ OFTEST_REGISTER(dcmnet_scu_sendNSETRequest_fails_when_requestedsopinstance_is_em OFTEST_REGISTER(dcmnet_scu_sendNSETRequest_succeeds_and_modifies_instance_when_scp_has_instance); OFTEST_REGISTER(dcmnet_scu_sendNSETRequest_succeeds_and_sets_responsestatuscode_from_scp_when_scp_sets_error_status); +OFTEST_REGISTER(dcmnet_scu_negotiateAssociation_fails_when_token_is_initially_cancelled); +OFTEST_REGISTER(dcmnet_scu_negotiateAssociation_fails_when_token_gets_cancelled_after_3_calls); +OFTEST_REGISTER(dcmnet_scu_negotiateAssociation_fails_when_connectionTimeout_elapses); #endif // WITH_THREADS OFTEST_MAIN("dcmnet") diff --git a/dcmnet/tests/tscuscp.cc b/dcmnet/tests/tscuscp.cc index 0e1d986a38..c5525dae19 100644 --- a/dcmnet/tests/tscuscp.cc +++ b/dcmnet/tests/tscuscp.cc @@ -505,6 +505,118 @@ OFTEST(dcmnet_scu_getConectionTimeout_returns_scu_tcp_connection_timeout) OFCHECK(scu.getConnectionTimeout() == 42); } +struct CancelToken : IDcmCancelToken +{ + CancelToken(Uint32 cancelAfterNumCalls) + : m_canceled(false) + , m_cancelAfterNumCalls(cancelAfterNumCalls) + , m_callCount(0) + {} + + bool IsCanceled() const /* override */ + { + ++m_callCount; + if (m_callCount > m_cancelAfterNumCalls) + m_canceled = true; + + return m_canceled; + } + + mutable bool m_canceled; + const Uint32 m_cancelAfterNumCalls; + mutable Uint32 m_callCount; +}; + + +struct TcpCancelFixture +{ + TcpCancelFixture() + { + m_scu.setPeerAETitle("ACCEPTOR"); + m_scu.setAETitle("REQUESTOR"); + m_scu.setPeerHostName("localhost"); + m_scu.setPeerPort(GetUnusedPort()); + + OFList ts; + ts.push_back(UID_LittleEndianImplicitTransferSyntax); + OFCHECK(m_scu.addPresentationContext(UID_VerificationSOPClass, ts, ASC_SC_ROLE_DEFAULT).good()); + } + + // Temporarily start an SCP to get an unused port + // Note: The port is not reserved after this call returns + static Uint16 GetUnusedPort() + { + TestSCP m_scp; + DcmSCPConfig& config = m_scp.getConfig(); + configure_scp_for_echo(config, 0, ASC_SC_ROLE_SCP); + config.setAETitle("ACCEPTOR"); + config.setConnectionBlockingMode(DUL_NOBLOCK); + config.setConnectionTimeout(4); + OFCHECK(m_scp.openListenPort().good()); + const Uint16 port = config.getPort(); + m_scp.join(); + return port; + } + + DcmSCU m_scu; + +}; + +OFTEST(dcmnet_scu_negotiateAssociation_fails_when_token_is_initially_cancelled) +{ + TcpCancelFixture m_fixture; + DcmSCU& scu = m_fixture.m_scu; + + scu.setTcpPollInterval(1); + scu.setConnectionTimeout(60); + + OFCHECK(scu.initNetwork().good()); + + CancelToken tok(0); + tok.m_canceled = true; + + const OFCondition result = scu.negotiateAssociation(&tok); + + OFCHECK(result.code() == DULC_TCPINITERROR); + OFCHECK(tok.m_callCount == 1); +} + +OFTEST(dcmnet_scu_negotiateAssociation_fails_when_token_gets_cancelled_after_3_calls) +{ + TcpCancelFixture m_fixture; + DcmSCU& scu = m_fixture.m_scu; + + scu.setTcpPollInterval(1); + scu.setConnectionTimeout(60); + + OFCHECK(scu.initNetwork().good()); + + CancelToken tok(3); + + const OFCondition result = scu.negotiateAssociation(&tok); + + OFCHECK(result.code() == DULC_TCPINITERROR); + OFCHECK(tok.m_callCount == 4); +} + +OFTEST(dcmnet_scu_negotiateAssociation_fails_when_connectionTimeout_elapses) +{ + + TcpCancelFixture m_fixture; + DcmSCU& scu = m_fixture.m_scu; + + scu.setTcpPollInterval(1); + scu.setConnectionTimeout(5); + + CancelToken tok(15); // Should never be called 15 times since connection timeout is reached first + + OFCHECK(scu.initNetwork().good()); + const OFCondition result = scu.negotiateAssociation(&tok); + + OFCHECK(result.code() == DULC_TCPINITERROR); + OFCHECK(!tok.m_canceled); +} + /** Helper function for testing role selection, test "dcmnet_scp_role_selection". * @param r_req The role selection setting from the association requestor