Mercurial > hg > orthanc
changeset 7156:8c8be0a58e46
Now rejecting Storage Commitment Report (N-EVENT-REPORT) if they don't perfectly match the request initiated by Orthanc (N-ACTION)
| author | Alain Mazy <am@orthanc.team> |
|---|---|
| date | Thu, 24 Sep 2026 14:56:56 +0200 |
| parents | 12bcfedb323a |
| children | 0da07ce09f9d |
| files | NEWS OrthancFramework/Sources/DicomNetworking/Internals/CommandDispatcher.cpp OrthancServer/Plugins/Samples/MultitenantDicom/OrthancFrameworkDependencies.cpp OrthancServer/Sources/OrthancRestApi/OrthancRestModalities.cpp OrthancServer/Sources/StorageCommitmentReports.cpp OrthancServer/Sources/StorageCommitmentReports.h OrthancServer/Sources/main.cpp |
| diffstat | 7 files changed, 138 insertions(+), 16 deletions(-) [+] |
line wrap: on
line diff
--- a/NEWS Wed Sep 23 15:47:45 2026 +0200 +++ b/NEWS Thu Sep 24 14:56:56 2026 +0200 @@ -18,6 +18,8 @@ ----------- * Fixed a TCP socket leak when a DICOM association is received with an invalid calling AET. +* Now rejecting Storage Commitment Report (N-EVENT-REPORT) if they don't perfectly match the + request initiated by Orthanc (N-ACTION) * Added more tolerance to invalid OW value representations * Fix use of the static runtime under Visual Studio (/MT), which broke in 1.12.10 * New CMake options:
--- a/OrthancFramework/Sources/DicomNetworking/Internals/CommandDispatcher.cpp Wed Sep 23 15:47:45 2026 +0200 +++ b/OrthancFramework/Sources/DicomNetworking/Internals/CommandDispatcher.cpp Thu Sep 24 14:56:56 2026 +0200 @@ -87,6 +87,7 @@ #include "GetScp.h" #include "MoveScp.h" #include "StoreScp.h" +#include "../DimseErrorPayload.h" #include <dcmtk/dcmdata/dcdeftag.h> /* for storage commitment */ #include <dcmtk/dcmdata/dcsequen.h> /* for class DcmSequenceOfItems */ @@ -1363,8 +1364,16 @@ { CLOG(ERROR, DICOM) << "Error while processing an incoming storage commitment report: " << e.What(); - // Code 0x0110 - "General failure in processing the operation was encountered" - dimseStatus = STATUS_N_ProcessingFailure; + if (e.GetPayload().HasContent() && + e.GetPayload().GetType() == ErrorPayloadType_Dimse) + { + dimseStatus = GetDimseErrorStatusFromPayload(e.GetPayload()); + } + else + { + // Code 0x0110 - "General failure in processing the operation was encountered" + dimseStatus = STATUS_N_ProcessingFailure; + } }
--- a/OrthancServer/Plugins/Samples/MultitenantDicom/OrthancFrameworkDependencies.cpp Wed Sep 23 15:47:45 2026 +0200 +++ b/OrthancServer/Plugins/Samples/MultitenantDicom/OrthancFrameworkDependencies.cpp Thu Sep 24 14:56:56 2026 +0200 @@ -52,6 +52,7 @@ #include "../../../../OrthancFramework/Sources/DicomNetworking/DicomAssociationParameters.cpp" #include "../../../../OrthancFramework/Sources/DicomNetworking/DicomFindAnswers.cpp" #include "../../../../OrthancFramework/Sources/DicomNetworking/DicomServer.cpp" +#include "../../../../OrthancFramework/Sources/DicomNetworking/DimseErrorPayload.cpp" #include "../../../../OrthancFramework/Sources/DicomNetworking/Internals/CommandDispatcher.cpp" #include "../../../../OrthancFramework/Sources/DicomNetworking/Internals/FindScp.cpp" #include "../../../../OrthancFramework/Sources/DicomNetworking/Internals/GetScp.cpp"
--- a/OrthancServer/Sources/OrthancRestApi/OrthancRestModalities.cpp Wed Sep 23 15:47:45 2026 +0200 +++ b/OrthancServer/Sources/OrthancRestApi/OrthancRestModalities.cpp Thu Sep 24 14:56:56 2026 +0200 @@ -2517,9 +2517,22 @@ // Create a "pending" storage commitment report BEFORE the // actual SCU call in order to avoid race conditions + std::unique_ptr<StorageCommitmentReports::Report> request(new StorageCommitmentReports::Report(remoteAet)); + + std::list<std::string>::const_iterator itInstanceUid = sopInstanceUids.begin(); + std::list<std::string>::const_iterator itClassUid = sopClassUids.begin(); + + while (itInstanceUid != sopInstanceUids.end() && itClassUid != sopClassUids.end()) + { + request->AddRequestedInstance(*itClassUid, *itInstanceUid); + + ++itInstanceUid; + ++itClassUid; + } + context.GetStorageCommitmentReports().Store( - transactionUid, new StorageCommitmentReports::Report(remoteAet)); - + transactionUid, request.release()); + DicomAssociationParameters parameters(localAet, remote); InjectAssociationTimeout(parameters, json);
--- a/OrthancServer/Sources/StorageCommitmentReports.cpp Wed Sep 23 15:47:45 2026 +0200 +++ b/OrthancServer/Sources/StorageCommitmentReports.cpp Thu Sep 24 14:56:56 2026 +0200 @@ -41,6 +41,18 @@ } } + void StorageCommitmentReports::Report::AddRequestedInstance(const std::string& sopClassUid, + const std::string& sopInstanceUid) + { + if (isComplete_) + { + throw OrthancException(ErrorCode_BadSequenceOfCalls); + } + + requetsedInstances_[sopInstanceUid] = sopClassUid; + } + + void StorageCommitmentReports::Report::AddSuccess(const std::string& sopClassUid, const std::string& sopInstanceUid) { @@ -180,6 +192,12 @@ } + const std::map<std::string, std::string>& StorageCommitmentReports::Report::GetRequestedInstancesAndSopClasses() const + { + return requetsedInstances_; + } + + StorageCommitmentReports::~StorageCommitmentReports() { while (!content_.IsEmpty())
--- a/OrthancServer/Sources/StorageCommitmentReports.h Wed Sep 23 15:47:45 2026 +0200 +++ b/OrthancServer/Sources/StorageCommitmentReports.h Thu Sep 24 14:56:56 2026 +0200 @@ -35,6 +35,8 @@ class Report : public boost::noncopyable { public: + typedef std::map<std::string, std::string> RequestedInstancesAndSopClasses; + enum Status { Status_Success, @@ -60,6 +62,7 @@ std::list<Success> success_; std::list<Failure> failures_; std::string remoteAet_; + RequestedInstancesAndSopClasses requetsedInstances_; public: explicit Report(const std::string& remoteAet) : @@ -75,6 +78,9 @@ void MarkAsComplete(); + void AddRequestedInstance(const std::string& sopClassUid, + const std::string& sopInstanceUid); + void AddSuccess(const std::string& sopClassUid, const std::string& sopInstanceUid); @@ -87,6 +93,8 @@ void Format(Json::Value& json) const; void GetSuccessSopInstanceUids(std::vector<std::string>& target) const; + + const RequestedInstancesAndSopClasses& GetRequestedInstancesAndSopClasses() const; }; private:
--- a/OrthancServer/Sources/main.cpp Wed Sep 23 15:47:45 2026 +0200 +++ b/OrthancServer/Sources/main.cpp Thu Sep 24 14:56:56 2026 +0200 @@ -31,6 +31,7 @@ #include "../../OrthancFramework/Sources/DicomFormat/DicomArray.h" #include "../../OrthancFramework/Sources/DicomNetworking/DicomAssociationParameters.h" #include "../../OrthancFramework/Sources/DicomNetworking/DicomServer.h" +#include "../../OrthancFramework/Sources/DicomNetworking/DimseErrorPayload.h" #include "../../OrthancFramework/Sources/DicomParsing/FromDcmtkBridge.h" #include "../../OrthancFramework/Sources/FileStorage/MemoryStorageArea.h" #include "../../OrthancFramework/Sources/FileStorage/PluginStorageAreaAdapter.h" @@ -174,21 +175,91 @@ THROW_WITH_FILE_AND_LINE_INFO(ErrorCode_InternalError); } - std::unique_ptr<StorageCommitmentReports::Report> report( - new StorageCommitmentReports::Report(connection.GetRemoteAet())); - - for (size_t i = 0; i < successSopClassUids.size(); i++) + std::unique_ptr<StorageCommitmentReports::Report> report; + { - report->AddSuccess(successSopClassUids[i], successSopInstanceUids[i]); + Orthanc::StorageCommitmentReports::Accessor requestAccessor(context_.GetStorageCommitmentReports(), transactionUid); + if (!requestAccessor.IsValid()) + { + throw OrthancException(ErrorCode_UnknownResource, + std::string("This Storage Commitment transaction UID has not been initiated by this Orthanc or is too old: ") + transactionUid + ).SetPayload(MakeDimseErrorStatusPayload(STATUS_N_UnrecognizedOperation)); + } + + if (requestAccessor.GetReport().GetRemoteAet() != connection.GetRemoteAet()) + { + throw OrthancException(ErrorCode_UnknownResource, + std::string("This Storage Commitment transaction is invalid: the remote AET the N-EVENT-REPORT orginates from (") + connection.GetRemoteAet() + ") does not match the AET the request was sent to (" + requestAccessor.GetReport().GetRemoteAet() + ")" + ); + } + + const StorageCommitmentReports::Report::RequestedInstancesAndSopClasses& requestedInstances = requestAccessor.GetReport().GetRequestedInstancesAndSopClasses(); + // make sure all requestedInstances have been answered: store all requested instances ids in a set and remove it from the set when they have been answered. + std::set<std::string> remainingRequestedInstancesIds; + StorageCommitmentReports::Report::RequestedInstancesAndSopClasses::const_iterator it = requestedInstances.begin(); + for (; it != requestedInstances.end(); ++it) + { + remainingRequestedInstancesIds.insert(it->first); + } + + report.reset(new StorageCommitmentReports::Report(connection.GetRemoteAet())); + + for (size_t i = 0; i < successSopClassUids.size(); i++) + { + StorageCommitmentReports::Report::RequestedInstancesAndSopClasses::const_iterator it = requestedInstances.find(successSopInstanceUids[i]); + + if (it == requestedInstances.end()) + { + throw OrthancException(ErrorCode_UnknownResource, + std::string("This (success) SOPInstanceUID (") + successSopInstanceUids[i] + ") was not requested in the Storage Commitment transaction " + transactionUid + ).SetPayload(MakeDimseErrorStatusPayload(STATUS_N_InvalidSOPInstance)); + } + + if (it->second != successSopClassUids[i]) + { + throw OrthancException(ErrorCode_UnknownResource, + std::string("This (success) SOPInstanceUID (") + successSopInstanceUids[i] + ") was not requested with this SOPClassUID (" + successSopClassUids[i] + ") in the Storage Commitment transaction " + transactionUid + ).SetPayload(MakeDimseErrorStatusPayload(STATUS_N_ClassInstanceConflict)); + } + + report->AddSuccess(successSopClassUids[i], successSopInstanceUids[i]); + remainingRequestedInstancesIds.erase(successSopInstanceUids[i]); + } + + for (size_t i = 0; i < failedSopClassUids.size(); i++) + { + StorageCommitmentReports::Report::RequestedInstancesAndSopClasses::const_iterator it = requestedInstances.find(successSopInstanceUids[i]); + + if (it == requestedInstances.end()) + { + throw OrthancException(ErrorCode_UnknownResource, + std::string("This (failed) SOPInstanceUID (") + successSopInstanceUids[i] + ") was not requested in the Storage Commitment transaction " + transactionUid + ).SetPayload(MakeDimseErrorStatusPayload(STATUS_N_InvalidSOPInstance)); + } + + report->AddFailure(failedSopClassUids[i], failedSopInstanceUids[i], failureReasons[i]); + remainingRequestedInstancesIds.erase(failedSopInstanceUids[i]); + } + + if (remainingRequestedInstancesIds.size() > 0) // make sure all requestedInstances have been answered + { + throw OrthancException(ErrorCode_InexistentItem, + std::string("Not all requested SOPInstanceUID have been answered in the Storage Commitment transaction ") + transactionUid + ).SetPayload(MakeDimseErrorStatusPayload(STATUS_N_ProcessingFailure)); + } + + // now that we are sure that all requestedInstances have been answered, we can reject any size mismatch (if more instances have been answered) + if (failedSopClassUids.size() + successSopClassUids.size() > requestedInstances.size()) + { + throw OrthancException(ErrorCode_UnknownResource, + std::string("This Storage Commitment transaction UID contains more instances than requested: ") + transactionUid + ).SetPayload(MakeDimseErrorStatusPayload(STATUS_N_UnrecognizedOperation)); + } + + + report->MarkAsComplete(); } - for (size_t i = 0; i < failedSopClassUids.size(); i++) - { - report->AddFailure(failedSopClassUids[i], failedSopInstanceUids[i], failureReasons[i]); - } - - report->MarkAsComplete(); - context_.GetStorageCommitmentReports().Store(transactionUid, report.release()); } };