changeset 3953:620e87e9e816 c-get

c-get: fixing memory with failedUIDs_
author Sebastien Jodogne <s.jodogne@gmail.com>
date Wed, 20 May 2020 09:19:35 +0200
parents 4f09a6a2b369
children 67b457283499
files Core/DicomNetworking/IGetRequestHandler.h Core/DicomNetworking/Internals/CommandDispatcher.cpp Core/DicomNetworking/Internals/GetScp.cpp Core/DicomNetworking/Internals/GetScp.h OrthancServer/OrthancGetRequestHandler.cpp OrthancServer/OrthancGetRequestHandler.h
diffstat 6 files changed, 157 insertions(+), 158 deletions(-) [+]
line wrap: on
line diff
--- a/Core/DicomNetworking/IGetRequestHandler.h	Wed May 20 08:41:54 2020 +0200
+++ b/Core/DicomNetworking/IGetRequestHandler.h	Wed May 20 09:19:35 2020 +0200
@@ -66,15 +66,15 @@
     
     virtual Status DoNext(T_ASC_Association *) = 0;
     
-    virtual unsigned int nRemaining() = 0;
+    virtual unsigned int nRemaining() const = 0;
     
-    virtual unsigned int nCompleted() = 0;
+    virtual unsigned int nCompleted() const = 0;
     
-    virtual unsigned int warningCount() = 0;
+    virtual unsigned int warningCount() const = 0;
     
-    virtual const char * failedUids() = 0;
+    virtual const std::string& failedUids() const = 0;
     
-    virtual unsigned int nFailed() = 0;
+    virtual unsigned int nFailed() const = 0;
   };
 
 }
--- a/Core/DicomNetworking/Internals/CommandDispatcher.cpp	Wed May 20 08:41:54 2020 +0200
+++ b/Core/DicomNetworking/Internals/CommandDispatcher.cpp	Wed May 20 09:19:35 2020 +0200
@@ -885,7 +885,7 @@
                 
                 if (handler.get() != NULL)
                 {
-                  cond = Internals::getScp(assoc_, &msg, presID, *handler, remoteIp_, remoteAet_, calledAet_);
+                  cond = Internals::getScp(assoc_, &msg, presID, *handler, remoteIp_, remoteAet_, calledAet_, associationTimeout_);
                 }
               }
               break;
--- a/Core/DicomNetworking/Internals/GetScp.cpp	Wed May 20 08:41:54 2020 +0200
+++ b/Core/DicomNetworking/Internals/GetScp.cpp	Wed May 20 09:19:35 2020 +0200
@@ -40,44 +40,44 @@
   Program: DCMTK 3.6.0
   Module:  http://dicom.offis.de/dcmtk.php.en
 
-Copyright (C) 1994-2011, OFFIS e.V.
-All rights reserved.
+  Copyright (C) 1994-2011, OFFIS e.V.
+  All rights reserved.
 
-This software and supporting documentation were developed by
+  This software and supporting documentation were developed by
 
   OFFIS e.V.
   R&D Division Health
   Escherweg 2
   26121 Oldenburg, Germany
 
-Redistribution and use in source and binary forms, with or without
-modification, are permitted provided that the following conditions
-are met:
+  Redistribution and use in source and binary forms, with or without
+  modification, are permitted provided that the following conditions
+  are met:
 
-- Redistributions of source code must retain the above copyright
+  - Redistributions of source code must retain the above copyright
   notice, this list of conditions and the following disclaimer.
 
-- Redistributions in binary form must reproduce the above copyright
+  - Redistributions in binary form must reproduce the above copyright
   notice, this list of conditions and the following disclaimer in the
   documentation and/or other materials provided with the distribution.
 
-- Neither the name of OFFIS nor the names of its contributors may be
+  - Neither the name of OFFIS nor the names of its contributors may be
   used to endorse or promote products derived from this software
   without specific prior written permission.
 
-THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
-"AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
-LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
-A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
-HOLDER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
-SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
-LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
-DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
-THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
-(INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
-OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+  THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+  "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+  LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+  A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
+  HOLDER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+  SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+  LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
+  DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
+  THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+  (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+  OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
 
-=========================================================================*/
+  =========================================================================*/
 
 
 #include "../../PrecompiledHeaders.h"
@@ -123,23 +123,27 @@
       };
     };
       
-    void BuildFailedInstanceList(const char* failedUIDs, DcmDataset ** rspIds)
+    static DcmDataset *BuildFailedInstanceList(const std::string& failedUIDs)
     {
-      OFBool ok;
-      
-      if (failedUIDs != NULL) 
+      if (failedUIDs.empty())
+      {
+        return NULL;
+      }
+      else
       {
-        *rspIds = new DcmDataset();
-        ok = DU_putStringDOElement(*rspIds, DCM_FailedSOPInstanceUIDList, failedUIDs);
-        if (!ok) 
+        std::unique_ptr<DcmDataset> rspIds(new DcmDataset());
+        
+        if (!DU_putStringDOElement(rspIds.get(), DCM_FailedSOPInstanceUIDList, failedUIDs.c_str()))
         {
-          LOG (ERROR) << "getSCP: failed to build DCM_FailedSOPInstanceUIDList";
+          throw OrthancException(ErrorCode_InternalError,
+                                 "getSCP: failed to build DCM_FailedSOPInstanceUIDList");
         }
-        
+
+        return rspIds.release();
       }
     }
 
-    void GetScpCallback(
+    static void GetScpCallback(
       /* in */ 
       void *callbackData,  
       OFBool cancelled, 
@@ -164,7 +168,7 @@
         try
         {
           if(!data.handler_->Handle(input, data.remoteIp_, data.remoteAet_,
-                                                     data.calledAet_))
+                                    data.calledAet_))
           {
             response->DimseStatus = STATUS_GET_Failed_UnableToProcess;
             return;
@@ -223,21 +227,24 @@
         {
           response->DimseStatus = STATUS_GET_Failed_UnableToProcess;
           
-          if (data.handler_->nFailed() > 0 || data.handler_->warningCount() > 0) 
+          if (data.handler_->nFailed() > 0 ||
+              data.handler_->warningCount() > 0) 
           {
             response->DimseStatus = STATUS_GET_Warning_SubOperationsCompleteOneOrMoreFailures;
           }
           /*
-           * if all the sub-operations failed then we need to generate a failed or refused status.
-           * cf. DICOM part 4, C.4.3.3.1
-           * we choose to generate a "Refused - Out of Resources - Unable to perform suboperations" status.
+           * if all the sub-operations failed then we need to generate
+           * a failed or refused status.  cf. DICOM part 4, C.4.3.3.1
+           * we choose to generate a "Refused - Out of Resources -
+           * Unable to perform suboperations" status.
            */
-          if ((data.handler_->nFailed() > 0) && ((data.handler_->nCompleted() + data.handler_->warningCount()) == 0)) 
+          if ((data.handler_->nFailed() > 0) &&
+              ((data.handler_->nCompleted() + data.handler_->warningCount()) == 0)) 
           {
             response->DimseStatus = STATUS_GET_Refused_OutOfResourcesSubOperations;
           }
           
-          BuildFailedInstanceList(data.handler_->failedUids(), responseIdentifiers);
+          *responseIdentifiers = BuildFailedInstanceList(data.handler_->failedUids());
         }
       }
       
@@ -246,18 +253,17 @@
       response->NumberOfFailedSubOperations = data.handler_->nFailed();
       response->NumberOfWarningSubOperations = data.handler_->warningCount();
     }
-    
   }
 
   OFCondition Internals::getScp(T_ASC_Association * assoc,
-                                 T_DIMSE_Message * msg, 
-                                 T_ASC_PresentationContextID presID,
-                                 IGetRequestHandler& handler,
-                                 std::string remoteIp,
-                                 std::string remoteAet,
-                                 std::string calledAet)
+                                T_DIMSE_Message * msg, 
+                                T_ASC_PresentationContextID presID,
+                                IGetRequestHandler& handler,
+                                std::string remoteIp,
+                                std::string remoteAet,
+                                std::string calledAet,
+                                int timeout)
   {
-    
     GetScpData data;
     data.lastRequest_ = NULL;
     data.handler_ = &handler;
@@ -266,12 +272,11 @@
     data.remoteAet_ = remoteAet;
     data.calledAet_ = calledAet;
 
-
     OFCondition cond = DIMSE_getProvider(assoc, presID, &msg->msg.CGetRQ, 
-                                          GetScpCallback, &data,
-                                          /*opt_blockMode*/ DIMSE_BLOCKING, 
-                                          /*opt_dimse_timeout*/ 0);
-
+                                         GetScpCallback, &data,
+                                         /*opt_blockMode*/ (timeout ? DIMSE_NONBLOCKING : DIMSE_BLOCKING),
+                                         /*opt_dimse_timeout*/ timeout);
+    
     // if some error occured, dump corresponding information and remove the outfile if necessary
     if (cond.bad())
     {
--- a/Core/DicomNetworking/Internals/GetScp.h	Wed May 20 08:41:54 2020 +0200
+++ b/Core/DicomNetworking/Internals/GetScp.h	Wed May 20 09:19:35 2020 +0200
@@ -37,7 +37,6 @@
 
 namespace Orthanc
 {
-  
   namespace Internals
   {
     OFCondition getScp(T_ASC_Association * assoc,
@@ -46,6 +45,7 @@
                        IGetRequestHandler& handler,
                        std::string remoteIp,
                        std::string remoteAet,
-                       std::string calledAet);
+                       std::string calledAet,
+                       int timeout);
   }
 }
--- a/OrthancServer/OrthancGetRequestHandler.cpp	Wed May 20 08:41:54 2020 +0200
+++ b/OrthancServer/OrthancGetRequestHandler.cpp	Wed May 20 09:19:35 2020 +0200
@@ -71,36 +71,28 @@
     }
     
     const std::string& id = instances_[position_++];
-    
+
     std::string dicom;
     context_.ReadDicom(dicom, id);
     
-    if(dicom.size() <= 0)
+    if (dicom.size() <= 0)
     {
       return Status_Failure;
     }
-    
-    DcmInputBufferStream is;
-    is.setBuffer(&dicom[0], dicom.size());
-    is.setEos();
-    
-    DcmFileFormat dcmff;
-    OFCondition cond = dcmff.read(is, EXS_Unknown, EGL_noChange, DCM_MaxReadLength);
-    if(cond.bad())
-    {
-      return Status_Failure;
-    }
+
+    std::unique_ptr<DcmFileFormat> parsed(
+      FromDcmtkBridge::LoadFromMemoryBuffer(dicom.c_str(), dicom.size()));
     
     // Determine the storage SOP class UID for this instance
     DIC_UI sopClass;
     DIC_UI sopInstance;
     
 #if DCMTK_VERSION_NUMBER >= 364
-    if (!DU_findSOPClassAndInstanceInDataSet(static_cast<DcmItem *> (dcmff.getDataset()),
+    if (!DU_findSOPClassAndInstanceInDataSet(static_cast<DcmItem *> (parsed->getDataset()),
                                              sopClass, sizeof(sopClass),
                                              sopInstance, sizeof(sopInstance)))
 #else
-      if (!DU_findSOPClassAndInstanceInDataSet(dcmff.getDataset(), sopClass, sopInstance))
+      if (!DU_findSOPClassAndInstanceInDataSet(parsed->getDataset(), sopClass, sopInstance))
 #endif
       {
         throw OrthancException(ErrorCode_NoSopClassOrInstance,
@@ -108,42 +100,30 @@
                                originatorAet_);
       }
     
-    
-    
-    
-    cond = performGetSubOp(assoc, sopClass, sopInstance, dcmff.getDataset());
-    if (getCancelled_) {
-      LOG (INFO) << "Get SCP: Received C-Cancel RQ";
+    OFCondition cond = performGetSubOp(assoc, sopClass, sopInstance, parsed->getDataset());
+    if (getCancelled_)
+    {
+      LOG(INFO) << "Get SCP: Received C-Cancel RQ";
     }
     
-    if(cond.bad() || getCancelled_)
+    if (cond.bad() || getCancelled_)
     {
       return Status_Failure;
     }
     
     return Status_Success;
   }
+
   
   void OrthancGetRequestHandler::addFailedUIDInstance(const char *sopInstance)
   {
-    size_t len;
-    
-    if (failedUIDs_ == NULL) {
-      if ((failedUIDs_ = (char*)malloc(DIC_UI_LEN+1)) == NULL) {
-        LOG (ERROR) << "malloc failure: addFailedUIDInstance";
-        return;
-      }
-      strcpy(failedUIDs_, sopInstance);
-    } else {
-      len = strlen(failedUIDs_);
-      if ((failedUIDs_ = (char*)realloc(failedUIDs_,
-                                        (len+strlen(sopInstance)+2))) == NULL) {
-        LOG (ERROR) << "realloc failure: addFailedUIDInstance";
-        return;
-      }
-      // tag sopInstance onto end of old with '\' between
-      strcat(failedUIDs_, "\\");
-      strcat(failedUIDs_, sopInstance);
+    if (failedUIDs_.empty())
+    {
+      failedUIDs_ = sopInstance;
+    }
+    else
+    {
+      failedUIDs_ += "\\" + std::string(sopInstance);
     }
   }
 
@@ -160,19 +140,21 @@
     T_ASC_PresentationContextID presId;
     DcmDataset *stDetail = NULL;
     
-    
     msgId = assoc->nextMsgID++;
     
     // which presentation context should be used
-    presId = ASC_findAcceptedPresentationContextID(assoc,
-                                                   sopClass);
-    if (presId == 0) {
+    presId = ASC_findAcceptedPresentationContextID(assoc, sopClass);
+    
+    if (presId == 0)
+    {
       nFailed_++;
       addFailedUIDInstance(sopInstance);
-      LOG (ERROR) << "Get SCP: storeSCU: No presentation context for: ("
-      << dcmSOPClassUIDToModality(sopClass, "OT") << ") " << sopClass;
+      LOG(ERROR) << "Get SCP: storeSCU: No presentation context for: ("
+                 << dcmSOPClassUIDToModality(sopClass, "OT") << ") " << sopClass;
       return DIMSE_NOVALIDPRESENTATIONCONTEXTID;
-    } else {
+    }
+    else
+    {
       // make sure that we can send images in this presentation context
       T_ASC_PresentationContext pc;
       ASC_findAcceptedPresentationContext(assoc->params, presId, &pc);
@@ -182,8 +164,8 @@
         // the role is not appropriate
         nFailed_++;
         addFailedUIDInstance(sopInstance);
-        LOG (ERROR) <<"Get SCP: storeSCU: [No presentation context with requestor SCP role for: ("
-        << dcmSOPClassUIDToModality(sopClass, "OT") << ") " << sopClass;
+        LOG(ERROR) <<"Get SCP: storeSCU: [No presentation context with requestor SCP role for: ("
+                   << dcmSOPClassUIDToModality(sopClass, "OT") << ") " << sopClass;
         return DIMSE_NOVALIDPRESENTATIONCONTEXTID;
       }
     }
@@ -195,8 +177,8 @@
     req.Priority = priority_;
     req.opts = 0;
     
-    LOG (INFO) << "Store SCU RQ: MsgID " << msgId << ", ("
-    << dcmSOPClassUIDToModality(sopClass, "OT") << ")";
+    LOG(INFO) << "Store SCU RQ: MsgID " << msgId << ", ("
+              << dcmSOPClassUIDToModality(sopClass, "OT") << ")";
     
     T_DIMSE_DetectedCancelParameters cancelParameters;
     
@@ -204,47 +186,63 @@
                            NULL, dataset, getSubOpProgressCallback, this, DIMSE_BLOCKING, 0,
                            &rsp, &stDetail, &cancelParameters);
     
-    if (cond.good()) {
-      if (cancelParameters.cancelEncountered) {
+    if (cond.good())
+    {
+      if (cancelParameters.cancelEncountered)
+      {
         if (origPresId == cancelParameters.presId &&
-            origMsgId == cancelParameters.req.MessageIDBeingRespondedTo) {
+            origMsgId == cancelParameters.req.MessageIDBeingRespondedTo)
+        {
           getCancelled_ = OFTrue;
-        } else {
-          LOG (ERROR) << "Get SCP: Unexpected C-Cancel-RQ encountered: pid=" << (int)cancelParameters.presId
-          << ", mid=" << (int)cancelParameters.req.MessageIDBeingRespondedTo;
+        }
+        else
+        {
+          LOG(ERROR) << "Get SCP: Unexpected C-Cancel-RQ encountered: pid=" << (int)cancelParameters.presId
+                     << ", mid=" << (int)cancelParameters.req.MessageIDBeingRespondedTo;
         }
       }
       
-      if (rsp.DimseStatus == STATUS_Success) {
+      if (rsp.DimseStatus == STATUS_Success)
+      {
         // everything ok
         nCompleted_++;
-      } else if ((rsp.DimseStatus & 0xf000) == 0xb000) {
+      }
+      else if ((rsp.DimseStatus & 0xf000) == 0xb000)
+      {
         // a warning status message
         warningCount_++;
-        LOG (ERROR) << "Get SCP: Store Warning: Response Status: " << DU_cstoreStatusString(rsp.DimseStatus);
-      } else {
+        LOG(ERROR) << "Get SCP: Store Warning: Response Status: "
+                   << DU_cstoreStatusString(rsp.DimseStatus);
+      }
+      else
+      {
         nFailed_++;
         addFailedUIDInstance(sopInstance);
         // print a status message
-        LOG (ERROR) << "Get SCP: Store Failed: Response Status: "
-        << DU_cstoreStatusString(rsp.DimseStatus);
+        LOG(ERROR) << "Get SCP: Store Failed: Response Status: "
+                   << DU_cstoreStatusString(rsp.DimseStatus);
       }
-    } else {
+    }
+    else
+    {
       nFailed_++;
       addFailedUIDInstance(sopInstance);
       OFString temp_str;
-      LOG (ERROR) << "Get SCP: storeSCU: Store Request Failed: " << DimseCondition::dump(temp_str, cond);
+      LOG(ERROR) << "Get SCP: storeSCU: Store Request Failed: " << DimseCondition::dump(temp_str, cond);
     }
-    if (stDetail) {
-      LOG (INFO) << "  Status Detail:" << OFendl << DcmObject::PrintHelper(*stDetail);
+    
+    if (stDetail)
+    {
+      LOG(INFO) << "  Status Detail:" << OFendl << DcmObject::PrintHelper(*stDetail);
       delete stDetail;
     }
+    
     return cond;
   }
 
   bool OrthancGetRequestHandler::LookupIdentifiers(std::vector<std::string>& publicIds,
-                                                    ResourceType level,
-                                                    const DicomMap& input)
+                                                   ResourceType level,
+                                                   const DicomMap& input)
   {
     DicomTag tag(0, 0);   // Dummy initialization
 
@@ -291,6 +289,17 @@
   }
 
 
+  OrthancGetRequestHandler::OrthancGetRequestHandler(ServerContext& context) :
+    context_(context)
+  {
+    position_ = 0;
+    nRemaining_ = 0;
+    nCompleted_  = 0;
+    warningCount_ = 0;
+    nFailed_ = 0;
+  }
+
+
   bool OrthancGetRequestHandler::Handle(const DicomMap& input,
                                         const std::string& originatorIp,
                                         const std::string& originatorAet,
@@ -342,7 +351,7 @@
     for (size_t i = 0; i < publicIds.size(); i++)
     {
       LOG(INFO) << "Sending resource " << publicIds[i] << " to modality \""
-      << originatorAet << "\" in synchronous mode";
+                << originatorAet << "\" in synchronous mode";
       
       std::list<std::string> tmp;
       context_.GetIndex().GetChildInstances(tmp, publicIds[i]);
@@ -353,7 +362,8 @@
         instances_.push_back(*it);
       }
     }
-    failedUIDs_ = NULL;
+
+    failedUIDs_.clear();
     getCancelled_ = OFFalse;
     
     nRemaining_ = GetSubOperationCount();
@@ -361,8 +371,6 @@
     nFailed_ = 0;
     warningCount_ = 0;
 
-    return retVal;
-
-    
+    return retVal;    
   }
 };
--- a/OrthancServer/OrthancGetRequestHandler.h	Wed May 20 08:41:54 2020 +0200
+++ b/OrthancServer/OrthancGetRequestHandler.h	Wed May 20 09:19:35 2020 +0200
@@ -55,7 +55,7 @@
     unsigned int nCompleted_;
     unsigned int warningCount_;
     unsigned int nFailed_;
-    char *failedUIDs_;
+    std::string failedUIDs_;
     
     T_DIMSE_Priority priority_;
     DIC_US origMsgId;
@@ -75,57 +75,43 @@
     void addFailedUIDInstance(const char *sopInstance);
 
   public:
-    OrthancGetRequestHandler(ServerContext& context) :
-      context_(context)
-    {
-      position_ = 0;
-
-      nRemaining_ = 0;
-      nCompleted_  = 0;
-      warningCount_ = 0;
-      nFailed_ = 0;
-
-      failedUIDs_ = NULL;
-    }
-
+    OrthancGetRequestHandler(ServerContext& context);
+    
     bool Handle(const DicomMap& input,
                 const std::string& originatorIp,
                 const std::string& originatorAet,
                 const std::string& calledAet);
     
-    virtual Status DoNext(T_ASC_Association *);
+    virtual Status DoNext(T_ASC_Association *) ORTHANC_OVERRIDE;
     
-    virtual unsigned int GetSubOperationCount() const
+    virtual unsigned int GetSubOperationCount() const ORTHANC_OVERRIDE
     {
       return (unsigned int) instances_.size();
     }
     
-    virtual unsigned int nRemaining()
+    virtual unsigned int nRemaining() const ORTHANC_OVERRIDE
     {
       return nRemaining_;
     }
     
-    virtual unsigned int nCompleted()
+    virtual unsigned int nCompleted() const ORTHANC_OVERRIDE
     {
       return nCompleted_;
     }
     
-    virtual unsigned int warningCount()
+    virtual unsigned int warningCount() const ORTHANC_OVERRIDE
     {
       return warningCount_;
     }
     
-    virtual unsigned int nFailed()
+    virtual unsigned int nFailed() const ORTHANC_OVERRIDE
     {
       return nFailed_;
     }
     
-    virtual const char * failedUids()
+    virtual const std::string& failedUids() const ORTHANC_OVERRIDE
     {
       return failedUIDs_;
-    }
-
-    
-    
+    }    
   };
 }