changeset 4151:8c559dd5034b

Fix possible crash in HttpClient if sending multipart body (can occur in STOW-RS)
author Sebastien Jodogne <s.jodogne@gmail.com>
date Wed, 19 Aug 2020 11:59:02 +0200
parents b56f3a37a4a1
children 36257d6f348f
files NEWS OrthancFramework/Sources/ChunkedBuffer.cpp OrthancFramework/Sources/HttpClient.cpp OrthancFramework/UnitTestsSources/RestApiTests.cpp
diffstat 4 files changed, 83 insertions(+), 48 deletions(-) [+]
line wrap: on
line diff
--- a/NEWS	Wed Aug 19 11:18:55 2020 +0200
+++ b/NEWS	Wed Aug 19 11:59:02 2020 +0200
@@ -1,10 +1,18 @@
 Pending changes in the mainline
 ===============================
 
+Plugins
+-------
+
+* New config option "Worklist.LimitAnswers" for the sample modality worklist plugin
+
+Maintenance
+-----------
+
 * Add missing tag "Retrieve AE Title (0008,0054)" in C-FIND SCP responses
 * Fix DICOM SCP filters if some query tag has more than 256 characters
 * "/series/.../ordered-slices" supports spaces in Image Position/Orientation Patient tags
-* New config option "Worklist.LimitAnswers" for the sample modality worklist plugin
+* Fix possible crash in HttpClient if sending multipart body (can occur in STOW-RS)
 
 
 Version 1.7.2 (2020-07-08)
--- a/OrthancFramework/Sources/ChunkedBuffer.cpp	Wed Aug 19 11:18:55 2020 +0200
+++ b/OrthancFramework/Sources/ChunkedBuffer.cpp	Wed Aug 19 11:59:02 2020 +0200
@@ -101,7 +101,7 @@
       // Optimization if Orthanc >= 1.7.3, to speed up in the presence of many small chunks
       if (pendingPos_ + chunkSize <= pendingBuffer_.size())
       {
-        // There remain enough place in the pending buffer
+        // There remains enough place in the pending buffer
         memcpy(&pendingBuffer_[pendingPos_], chunkData, chunkSize);
         pendingPos_ += chunkSize;
       }
--- a/OrthancFramework/Sources/HttpClient.cpp	Wed Aug 19 11:18:55 2020 +0200
+++ b/OrthancFramework/Sources/HttpClient.cpp	Wed Aug 19 11:59:02 2020 +0200
@@ -197,8 +197,8 @@
   {
   private:
     HttpClient::IRequestBody*  body_;
-    std::string                sourceBuffer_;
-    size_t                     sourceBufferTransmittedSize_;
+    std::string                pending_;
+    size_t                     pendingPos_;
 
     size_t CallbackInternal(char* curlBuffer,
                             size_t curlBufferSize)
@@ -213,65 +213,63 @@
         throw OrthancException(ErrorCode_InternalError);
       }
 
-      // Read chunks from the body stream so as to fill the target buffer
-      size_t curlBufferFilledSize = 0;
-      size_t sourceRemainingSize = sourceBuffer_.size() - sourceBufferTransmittedSize_;
-      bool hasMore = true;
-      
-      while (sourceRemainingSize < curlBufferSize && hasMore)
+      if (pendingPos_ + curlBufferSize <= pending_.size())
+      {
+        assert(sizeof(char) == 1);
+        memcpy(curlBuffer, &pending_[pendingPos_], curlBufferSize);
+        pendingPos_ += curlBufferSize;
+        return curlBufferSize;
+      }
+      else
       {
-        if (sourceRemainingSize > 0)
+        ChunkedBuffer buffer;
+        buffer.SetPendingBufferSize(curlBufferSize);
+
+        if (pendingPos_ < pending_.size())
         {
-          // transmit the end of current source buffer
-          memcpy(curlBuffer + curlBufferFilledSize,
-                 sourceBuffer_.data() + sourceBufferTransmittedSize_, sourceRemainingSize);
-
-          curlBufferFilledSize += sourceRemainingSize;
+          buffer.AddChunk(&pending_[pendingPos_], pending_.size() - pendingPos_);
+        }
+        
+        // Read chunks from the body stream so as to fill the target buffer
+        std::string chunk;
+        
+        while (buffer.GetNumBytes() < curlBufferSize &&
+               body_->ReadNextChunk(chunk))
+        {
+          buffer.AddChunk(chunk);
         }
 
-        // start filling a new source buffer
-        sourceBufferTransmittedSize_ = 0;
-        sourceBuffer_.clear();
-
-        hasMore = body_->ReadNextChunk(sourceBuffer_);
-
-        sourceRemainingSize = sourceBuffer_.size();
-      }
+        buffer.Flatten(pending_);
+        pendingPos_ = std::min(pending_.size(), curlBufferSize);
 
-      if (sourceRemainingSize > 0 &&
-          curlBufferSize > curlBufferFilledSize)
-      {
-        size_t s = std::min(sourceRemainingSize, curlBufferSize - curlBufferFilledSize);
+        if (pendingPos_ != 0)
+        {
+          memcpy(curlBuffer, pending_.c_str(), pendingPos_);
+        }
 
-        memcpy(curlBuffer + curlBufferFilledSize,
-               sourceBuffer_.data() + sourceBufferTransmittedSize_, s);
-
-        sourceBufferTransmittedSize_ += s;
-        curlBufferFilledSize += s;
+        return pendingPos_;
       }
-
-      return curlBufferFilledSize;
     }
     
   public:
     CurlRequestBody() :
       body_(NULL),
-      sourceBufferTransmittedSize_(0)
+      pendingPos_(0)
     {
     }
 
     void SetBody(HttpClient::IRequestBody& body)
     {
       body_ = &body;
-      sourceBufferTransmittedSize_ = 0;
-      sourceBuffer_.clear();
+      pending_.clear();
+      pendingPos_ = 0;
     }
 
     void Clear()
     {
       body_ = NULL;
-      sourceBufferTransmittedSize_ = 0;
-      sourceBuffer_.clear();
+      pending_.clear();
+      pendingPos_ = 0;
     }
 
     bool IsValid() const
--- a/OrthancFramework/UnitTestsSources/RestApiTests.cpp	Wed Aug 19 11:18:55 2020 +0200
+++ b/OrthancFramework/UnitTestsSources/RestApiTests.cpp	Wed Aug 19 11:59:02 2020 +0200
@@ -1034,7 +1034,7 @@
                         const void* bodyData,
                         size_t bodySize)
     {
-      printf("received %llu\n", bodySize);
+      printf("received %lu\n", bodySize);
 
       const uint8_t* b = reinterpret_cast<const uint8_t*>(bodyData);
       
@@ -1055,7 +1055,37 @@
 
 #include "../Sources/HttpServer/HttpServer.h"
 
-TEST(Toto, DISABLED_Toto)
+TEST(HttpClient, DISABLED_Issue156_Slow)
+{
+  // https://bugs.orthanc-server.com/show_bug.cgi?id=156
+  
+  TotoServer handler;
+  HttpServer server;
+  server.SetPortNumber(5000);
+  server.Register(handler);
+  server.Start();
+  
+  WebServiceParameters w;
+  w.SetUrl("http://localhost:5000");
+
+  // This is slow in Orthanc <= 1.5.8 (issue 156)
+  TotoBody body(600 * 1024 * 1024, 6 * 1024 * 1024 - 17);
+  
+  HttpClient c(w, "toto");
+  c.SetMethod(HttpMethod_Post);
+  c.AddHeader("Expect", "");
+  c.AddHeader("Transfer-Encoding", "chunked");
+  c.SetBody(body);
+  
+  std::string s;
+  ASSERT_TRUE(c.Apply(s));
+  ASSERT_EQ("ok", s);
+
+  server.Stop();
+}
+
+
+TEST(HttpClient, DISABLED_Issue156_Crash)
 {
   TotoServer handler;
   HttpServer server;
@@ -1065,20 +1095,19 @@
   
   WebServiceParameters w;
   w.SetUrl("http://localhost:5000");
-  
-  //TotoBody body(600 * 1024 * 1024, 6 * 1024 * 1024 - 17);
-  TotoBody body(32 * 1024, 1);  // This crashes Orthanc 1.6.0 to 1.7.2 
+
+  // This crashes Orthanc 1.6.0 to 1.7.2 
+  TotoBody body(32 * 1024, 1);  
   
   HttpClient c(w, "toto");
   c.SetMethod(HttpMethod_Post);
   c.AddHeader("Expect", "");
   c.AddHeader("Transfer-Encoding", "chunked");
   c.SetBody(body);
-
+  
   std::string s;
   ASSERT_TRUE(c.Apply(s));
-
-  printf(">> [%s]\n", s.c_str());
+  ASSERT_EQ("ok", s);
 
   server.Stop();
 }