# HG changeset patch # User Sebastien Jodogne # Date 1478689449 -3600 # Node ID ddc75c6c712d40dd258d01753f30f7b0bc9b8db6 # Parent 15ae532af70efc181ae1edcb27951d9b18cd8d76 Avoid hard crash if not enough memory diff -r 15ae532af70e -r ddc75c6c712d Core/HttpServer/MongooseServer.cpp --- 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(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(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 } diff -r 15ae532af70e -r ddc75c6c712d Core/Logging.cpp --- 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(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(duration.fractional_seconds())); - header = std::string(date) + path.filename().string() + ":" + boost::lexical_cast(line) + "] "; - } + header = std::string(date) + path.filename().string() + ":" + boost::lexical_cast(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???? ??:??:??.?????? ] "; + } } diff -r 15ae532af70e -r ddc75c6c712d Core/Logging.h --- 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; + } }; } } diff -r 15ae532af70e -r ddc75c6c712d Core/MultiThreading/RunnableWorkersPool.cpp --- 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 obj(that->queue_.Dequeue(100)); - if (obj.get() != NULL) + try { - try + std::auto_ptr obj(that->queue_.Dequeue(100)); + if (obj.get() != NULL) { IRunnableBySteps& runnable = *dynamic_cast(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"; } } } diff -r 15ae532af70e -r ddc75c6c712d NEWS --- 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 diff -r 15ae532af70e -r ddc75c6c712d OrthancServer/Internals/CommandDispatcher.cpp --- 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 diff -r 15ae532af70e -r ddc75c6c712d OrthancServer/Internals/CommandDispatcher.h --- 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(); }; diff -r 15ae532af70e -r ddc75c6c712d OrthancServer/ServerContext.cpp --- 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) {