changeset 2134:ddc75c6c712d

Avoid hard crash if not enough memory
author Sebastien Jodogne <s.jodogne@gmail.com>
date Wed, 09 Nov 2016 12:04:09 +0100
parents 15ae532af70e
children cadfe0a2a393
files Core/HttpServer/MongooseServer.cpp Core/Logging.cpp Core/Logging.h Core/MultiThreading/RunnableWorkersPool.cpp NEWS OrthancServer/Internals/CommandDispatcher.cpp OrthancServer/Internals/CommandDispatcher.h OrthancServer/ServerContext.cpp
diffstat 8 files changed, 259 insertions(+), 166 deletions(-) [+]
line wrap: on
line diff
--- a/Core/HttpServer/MongooseServer.cpp	Wed Nov 09 10:21:37 2016 +0100
+++ b/Core/HttpServer/MongooseServer.cpp	Wed Nov 09 12:04:09 2016 +0100
@@ -575,16 +575,14 @@
   }
 
 
-  static void InternalCallback(struct mg_connection *connection,
+  static void InternalCallback(HttpOutput& output /* out */,
+                               HttpMethod& method /* out */,
+                               MongooseServer& server,
+                               struct mg_connection *connection,
                                const struct mg_request_info *request)
   {
-    MongooseServer* that = reinterpret_cast<MongooseServer*>(request->user_data);
-
-    MongooseOutputStream stream(connection);
-    HttpOutput output(stream, that->IsKeepAliveEnabled());
-
     // Check remote calls
-    if (!that->IsRemoteAccessAllowed() &&
+    if (!server.IsRemoteAccessAllowed() &&
         request->remote_ip != LOCALHOST)
     {
       output.SendUnauthorized(ORTHANC_REALM);
@@ -604,7 +602,7 @@
       VLOG(1) << "HTTP header: [" << name << "]: [" << value << "]";
     }
 
-    if (that->IsHttpCompressionEnabled())
+    if (server.IsHttpCompressionEnabled())
     {
       ConfigureHttpCompression(output, headers);
     }
@@ -619,7 +617,7 @@
 
 
     // Compute the HTTP method, taking method faking into consideration
-    HttpMethod method = HttpMethod_Get;
+    method = HttpMethod_Get;
     if (!ExtractMethod(method, request, headers, argumentsGET))
     {
       output.SendStatus(HttpStatus_400_BadRequest);
@@ -628,7 +626,8 @@
 
 
     // Authenticate this connection
-    if (that->IsAuthenticationEnabled() && !IsAccessGranted(*that, headers))
+    if (server.IsAuthenticationEnabled() && 
+        !IsAccessGranted(server, headers))
     {
       output.SendUnauthorized(ORTHANC_REALM);
       return;
@@ -645,7 +644,7 @@
 
     std::string username = GetAuthenticatedUsername(headers);
 
-    const IIncomingHttpRequestFilter *filter = that->GetIncomingHttpRequestFilter();
+    const IIncomingHttpRequestFilter *filter = server.GetIncomingHttpRequestFilter();
     if (filter != NULL)
     {
       if (!filter->IsAllowed(method, request->uri, remoteIp, username.c_str(), headers))
@@ -679,7 +678,7 @@
         if (contentType.size() >= multipartLength &&
             !memcmp(contentType.c_str(), multipart, multipartLength))
         {
-          status = ParseMultipartPost(body, connection, headers, contentType, that->GetChunkStore());
+          status = ParseMultipartPost(body, connection, headers, contentType, server.GetChunkStore());
         }
         else
         {
@@ -713,7 +712,7 @@
     {
       Toolbox::SplitUriComponents(uri, request->uri);
     }
-    catch (OrthancException)
+    catch (OrthancException&)
     {
       output.SendStatus(HttpStatus_400_BadRequest);
       return;
@@ -722,60 +721,100 @@
 
     LOG(INFO) << EnumerationToString(method) << " " << Toolbox::FlattenUri(uri);
 
+    bool found = false;
 
+    if (server.HasHandler())
+    {
+      found = server.GetHandler().Handle(output, RequestOrigin_RestApi, remoteIp, username.c_str(), 
+                                         method, uri, headers, argumentsGET, body.c_str(), body.size());
+    }
+
+    if (!found)
+    {
+      throw OrthancException(ErrorCode_UnknownResource);
+    }
+  }
+
+
+  static void ProtectedCallback(struct mg_connection *connection,
+                                const struct mg_request_info *request)
+  {
     try
     {
-      bool found = false;
+      MongooseServer* server = reinterpret_cast<MongooseServer*>(request->user_data);
+      
+      if (server == NULL)
+      {
+        MongooseOutputStream stream(connection);
+        HttpOutput output(stream, false /* assume no keep-alive */);
+        output.SendStatus(HttpStatus_500_InternalServerError);
+        return;
+      }
+      
+      MongooseOutputStream stream(connection);
+      HttpOutput output(stream, server->IsKeepAliveEnabled());
+      HttpMethod method = HttpMethod_Get;
 
       try
       {
-        if (that->HasHandler())
+        try
+        {
+          InternalCallback(output, method, *server, connection, request);
+        }
+        catch (OrthancException&)
+        {
+          throw;  // Pass the exception to the main handler below
+        }
+        // Now convert native exceptions as OrthancException
+        catch (boost::bad_lexical_cast&)
         {
-          found = that->GetHandler().Handle(output, RequestOrigin_RestApi, remoteIp, username.c_str(), 
-                                            method, uri, headers, argumentsGET, body.c_str(), body.size());
+          LOG(ERROR) << "Syntax error in some user-supplied data";
+          throw OrthancException(ErrorCode_BadParameterType);
+        }
+        catch (std::runtime_error&)
+        {
+          // Presumably an error while parsing the JSON body
+          throw OrthancException(ErrorCode_BadRequest);
+        }
+        catch (std::bad_alloc&)
+        {
+          LOG(ERROR) << "The server hosting Orthanc is running out of memory";
+          throw OrthancException(ErrorCode_NotEnoughMemory);
+        }
+        catch (...)
+        {
+          LOG(ERROR) << "An unhandled exception was generated inside the HTTP server";
+          throw OrthancException(ErrorCode_InternalError);
         }
       }
-      catch (boost::bad_lexical_cast&)
-      {
-        throw OrthancException(ErrorCode_BadParameterType);
-      }
-      catch (std::runtime_error&)
+      catch (OrthancException& e)
       {
-        // Presumably an error while parsing the JSON body
-        throw OrthancException(ErrorCode_BadRequest);
-      }
-      /*catch (std::bad_alloc&)
-      {
-        throw OrthancException(ErrorCode_NotEnoughMemory);
-        }*/
+        assert(server != NULL);
 
-      if (!found)
-      {
-        throw OrthancException(ErrorCode_UnknownResource);
+        // Using this candidate handler results in an exception
+        try
+        {
+          if (server->GetExceptionFormatter() == NULL)
+          {
+            LOG(ERROR) << "Exception in the HTTP handler: " << e.What();
+            output.SendStatus(e.GetHttpStatus());
+          }
+          else
+          {
+            server->GetExceptionFormatter()->Format(output, e, method, request->uri);
+          }
+        }
+        catch (OrthancException&)
+        {
+          // An exception here reflects the fact that the status code
+          // was already set by the HTTP handler.
+        }
       }
     }
-    catch (OrthancException& e)
+    catch (...)
     {
-      // Using this candidate handler results in an exception
-      try
-      {
-        if (that->GetExceptionFormatter() == NULL)
-        {
-          LOG(ERROR) << "Exception in the HTTP handler: " << e.What();
-          output.SendStatus(e.GetHttpStatus());
-        }
-        else
-        {
-          that->GetExceptionFormatter()->Format(output, e, method, request->uri);
-        }
-      }
-      catch (OrthancException&)
-      {
-        // An exception here reflects the fact that the status code
-        // was already set by the HTTP handler.
-      }
-
-      return;
+      // We should never arrive at this point, where it is even impossible to send an answer
+      LOG(ERROR) << "Catastrophic error inside the HTTP server, giving up";
     }
   }
 
@@ -787,7 +826,7 @@
   {
     if (event == MG_NEW_REQUEST) 
     {
-      InternalCallback(connection, request);
+      ProtectedCallback(connection, request);
 
       // Mark as processed
       return (void*) "";
@@ -803,7 +842,7 @@
   {
     struct mg_request_info *request = mg_get_request_info(connection);
 
-    InternalCallback(connection, request);
+    ProtectedCallback(connection, request);
 
     return 1;  // Do not let Mongoose handle the request by itself
   }
--- a/Core/Logging.cpp	Wed Nov 09 10:21:37 2016 +0100
+++ b/Core/Logging.cpp	Wed Nov 09 12:04:09 2016 +0100
@@ -318,113 +318,123 @@
         return;
       }
 
-      LogLevel l = StringToLogLevel(level);
-      
-      if ((l == LogLevel_Info  && !loggingContext_->infoEnabled_) ||
-          (l == LogLevel_Trace && !loggingContext_->traceEnabled_))
+      try
       {
-        // This logging level is disabled, directly exit and unlock
-        // the mutex to speed-up things. The stream is set to "/dev/null"
-        lock_.unlock();
-        return;
-      }
+        LogLevel l = StringToLogLevel(level);
+      
+        if ((l == LogLevel_Info  && !loggingContext_->infoEnabled_) ||
+            (l == LogLevel_Trace && !loggingContext_->traceEnabled_))
+        {
+          // This logging level is disabled, directly exit and unlock
+          // the mutex to speed-up things. The stream is set to "/dev/null"
+          lock_.unlock();
+          return;
+        }
 
-      // Compute the header of the line, temporary release the lock as
-      // this is a time-consuming operation
-      lock_.unlock();
-      std::string header;
+        // Compute the header of the line, temporary release the lock as
+        // this is a time-consuming operation
+        lock_.unlock();
+        std::string header;
 
-      {
-        boost::filesystem::path path(file);
-        boost::posix_time::ptime now = boost::posix_time::microsec_clock::local_time();
-        boost::posix_time::time_duration duration = now.time_of_day();
+        {
+          boost::filesystem::path path(file);
+          boost::posix_time::ptime now = boost::posix_time::microsec_clock::local_time();
+          boost::posix_time::time_duration duration = now.time_of_day();
 
-        /**
-           From Google Log documentation:
+          /**
+             From Google Log documentation:
 
-           "Log lines have this form:
+             "Log lines have this form:
 
-           Lmmdd hh:mm:ss.uuuuuu threadid file:line] msg...
+             Lmmdd hh:mm:ss.uuuuuu threadid file:line] msg...
 
-           where the fields are defined as follows:
+             where the fields are defined as follows:
 
-           L                A single character, representing the log level (eg 'I' for INFO)
-           mm               The month (zero padded; ie May is '05')
-           dd               The day (zero padded)
-           hh:mm:ss.uuuuuu  Time in hours, minutes and fractional seconds
-           threadid         The space-padded thread ID as returned by GetTID() (this matches the PID on Linux)
-           file             The file name
-           line             The line number
-           msg              The user-supplied message"
+             L                A single character, representing the log level (eg 'I' for INFO)
+             mm               The month (zero padded; ie May is '05')
+             dd               The day (zero padded)
+             hh:mm:ss.uuuuuu  Time in hours, minutes and fractional seconds
+             threadid         The space-padded thread ID as returned by GetTID() (this matches the PID on Linux)
+             file             The file name
+             line             The line number
+             msg              The user-supplied message"
 
-           In this implementation, "threadid" is not printed.
-         **/
+             In this implementation, "threadid" is not printed.
+          **/
 
-        char date[32];
-        sprintf(date, "%c%02d%02d %02d:%02d:%02d.%06d ",
-                level[0],
-                now.date().month().as_number(),
-                now.date().day().as_number(),
-                duration.hours(),
-                duration.minutes(),
-                duration.seconds(),
-                static_cast<int>(duration.fractional_seconds()));
+          char date[32];
+          sprintf(date, "%c%02d%02d %02d:%02d:%02d.%06d ",
+                  level[0],
+                  now.date().month().as_number(),
+                  now.date().day().as_number(),
+                  duration.hours(),
+                  duration.minutes(),
+                  duration.seconds(),
+                  static_cast<int>(duration.fractional_seconds()));
 
-        header = std::string(date) + path.filename().string() + ":" + boost::lexical_cast<std::string>(line) + "] ";
-      }
+          header = std::string(date) + path.filename().string() + ":" + boost::lexical_cast<std::string>(line) + "] ";
+        }
 
 
-      // The header is computed, we now re-lock the mutex to access
-      // the stream objects. Pay attention that "loggingContext_",
-      // "infoEnabled_" or "traceEnabled_" might have changed while
-      // the mutex was unlocked.
-      lock_.lock();
+        // The header is computed, we now re-lock the mutex to access
+        // the stream objects. Pay attention that "loggingContext_",
+        // "infoEnabled_" or "traceEnabled_" might have changed while
+        // the mutex was unlocked.
+        lock_.lock();
+
+        if (loggingContext_.get() == NULL)
+        {
+          fprintf(stderr, "ERROR: Trying to log a message after the finalization of the logging engine\n");
+          return;
+        }
 
-      if (loggingContext_.get() == NULL)
-      {
-        fprintf(stderr, "ERROR: Trying to log a message after the finalization of the logging engine\n");
-        return;
-      }
+        switch (l)
+        {
+          case LogLevel_Error:
+            stream_ = loggingContext_->error_;
+            break;
 
-      switch (l)
-      {
-        case LogLevel_Error:
-          stream_ = loggingContext_->error_;
-          break;
+          case LogLevel_Warning:
+            stream_ = loggingContext_->warning_;
+            break;
 
-        case LogLevel_Warning:
-          stream_ = loggingContext_->warning_;
-          break;
+          case LogLevel_Info:
+            if (loggingContext_->infoEnabled_)
+            {
+              stream_ = loggingContext_->info_;
+            }
+
+            break;
 
-        case LogLevel_Info:
-          if (loggingContext_->infoEnabled_)
-          {
-            stream_ = loggingContext_->info_;
-          }
+          case LogLevel_Trace:
+            if (loggingContext_->traceEnabled_)
+            {
+              stream_ = loggingContext_->info_;
+            }
 
-          break;
+            break;
 
-        case LogLevel_Trace:
-          if (loggingContext_->traceEnabled_)
-          {
-            stream_ = loggingContext_->info_;
-          }
+          default:
+            throw OrthancException(ErrorCode_InternalError);
+        }
 
-          break;
-
-        default:
-          throw OrthancException(ErrorCode_InternalError);
-      }
+        if (stream_ == &null_)
+        {
+          // The logging is disabled for this level. The stream is the
+          // "null_" member of this object, so we can release the global
+          // mutex.
+          lock_.unlock();
+        }
 
-      if (stream_ == &null_)
-      {
-        // The logging is disabled for this level. The stream is the
-        // "null_" member of this object, so we can release the global
-        // mutex.
-        lock_.unlock();
+        (*stream_) << header;
       }
-
-      (*stream_) << header;
+      catch (...)
+      { 
+        // Something is going really wrong, probably running out of
+        // memory. Fallback to a degraded mode.
+        stream_ = loggingContext_->error_;
+        (*stream_) << "E???? ??:??:??.?????? ] ";
+      }
     }
 
 
--- a/Core/Logging.h	Wed Nov 09 10:21:37 2016 +0100
+++ b/Core/Logging.h	Wed Nov 09 12:04:09 2016 +0100
@@ -110,6 +110,12 @@
       {
         return (*stream_) << message;
       }
+
+      // This overload fixes build problems with Visual Studio 2015
+      std::ostream& operator<< (const char* message)
+      {
+        return (*stream_) << message;
+      }
     };
   }
 }
--- a/Core/MultiThreading/RunnableWorkersPool.cpp	Wed Nov 09 10:21:37 2016 +0100
+++ b/Core/MultiThreading/RunnableWorkersPool.cpp	Wed Nov 09 12:04:09 2016 +0100
@@ -52,25 +52,33 @@
       {
         while (that->continue_)
         {
-          std::auto_ptr<IDynamicObject>  obj(that->queue_.Dequeue(100));
-          if (obj.get() != NULL)
+          try
           {
-            try
+            std::auto_ptr<IDynamicObject>  obj(that->queue_.Dequeue(100));
+            if (obj.get() != NULL)
             {
               IRunnableBySteps& runnable = *dynamic_cast<IRunnableBySteps*>(obj.get());
-            
+              
               bool wishToContinue = runnable.Step();
-
+              
               if (wishToContinue)
               {
                 // The runnable wishes to continue, reinsert it at the beginning of the queue
                 that->queue_.Enqueue(obj.release());
               }
             }
-            catch (OrthancException& e)
-            {
-              LOG(ERROR) << "Exception in a pool of working threads: " << e.What();
-            }
+          }
+          catch (OrthancException& e)
+          {
+            LOG(ERROR) << "Exception while handling some runnable object: " << e.What();
+          }
+          catch (std::bad_alloc&)
+          {
+            LOG(ERROR) << "Not enough memory to handle some runnable object";
+          }
+          catch (...)
+          {
+            LOG(ERROR) << "Native exception while handling some runnable object";
           }
         }
       }
--- a/NEWS	Wed Nov 09 10:21:37 2016 +0100
+++ b/NEWS	Wed Nov 09 12:04:09 2016 +0100
@@ -25,6 +25,7 @@
 Maintenance
 -----------
 
+* Avoid hard crash if not enough memory (handling of std::bad_alloc)
 * Improved robustness of Orthanc Explorer wrt. query/retrieve (maybe fix issue 24)
 * Fix serious performance issue with C-FIND
 * Fix extraction of the symbolic name of the private tags
--- a/OrthancServer/Internals/CommandDispatcher.cpp	Wed Nov 09 10:21:37 2016 +0100
+++ b/OrthancServer/Internals/CommandDispatcher.cpp	Wed Nov 09 12:04:09 2016 +0100
@@ -381,7 +381,6 @@
 
     OFCondition AssociationCleanup(T_ASC_Association *assoc)
     {
-      OFString temp_str;
       OFCondition cond = ASC_dropSCPAssociation(assoc);
       if (cond.bad())
       {
@@ -695,6 +694,38 @@
       return new CommandDispatcher(server, assoc, remoteIp, remoteAet, calledAet, filter);
     }
 
+
+    CommandDispatcher::CommandDispatcher(const DicomServer& server,
+                                         T_ASC_Association* assoc,
+                                         const std::string& remoteIp,
+                                         const std::string& remoteAet,
+                                         const std::string& calledAet,
+                                         IApplicationEntityFilter* filter) :
+      server_(server),
+      assoc_(assoc),
+      remoteIp_(remoteIp),
+      remoteAet_(remoteAet),
+      calledAet_(calledAet),
+      filter_(filter)
+    {
+      associationTimeout_ = server.GetAssociationTimeout();
+      elapsedTimeSinceLastCommand_ = 0;
+    }
+
+
+    CommandDispatcher::~CommandDispatcher()
+    {
+      try
+      {
+        AssociationCleanup(assoc_);
+      }
+      catch (...)
+      {
+        LOG(ERROR) << "Some association was not cleanly aborted";
+      }
+    }
+
+
     bool CommandDispatcher::Step()
     /*
      * This function receives DIMSE commmands over the network connection
--- a/OrthancServer/Internals/CommandDispatcher.h	Wed Nov 09 10:21:37 2016 +0100
+++ b/OrthancServer/Internals/CommandDispatcher.h	Wed Nov 09 12:04:09 2016 +0100
@@ -61,22 +61,9 @@
                         const std::string& remoteIp,
                         const std::string& remoteAet,
                         const std::string& calledAet,
-                        IApplicationEntityFilter* filter) :
-        server_(server),
-        assoc_(assoc),
-        remoteIp_(remoteIp),
-        remoteAet_(remoteAet),
-        calledAet_(calledAet),
-        filter_(filter)
-      {
-        associationTimeout_ = server.GetAssociationTimeout();
-        elapsedTimeSinceLastCommand_ = 0;
-      }
+                        IApplicationEntityFilter* filter);
 
-      virtual ~CommandDispatcher()
-      {
-        AssociationCleanup(assoc_);
-      }
+      virtual ~CommandDispatcher();
 
       virtual bool Step();
     };
--- a/OrthancServer/ServerContext.cpp	Wed Nov 09 10:21:37 2016 +0100
+++ b/OrthancServer/ServerContext.cpp	Wed Nov 09 12:04:09 2016 +0100
@@ -86,7 +86,18 @@
         {
           try
           {
-            it->GetListener().SignalChange(change);
+            try
+            {
+              it->GetListener().SignalChange(change);
+            }
+            catch (std::bad_alloc&)
+            {
+              LOG(ERROR) << "Not enough memory while signaling a change";
+            }
+            catch (...)
+            {
+              throw OrthancException(ErrorCode_InternalError);
+            }
           }
           catch (OrthancException& e)
           {