Mercurial > hg > orthanc-stone

changeset 975:e75fd08d6c75 toa2019083101

Find changesets by keywords (author, files, the commit message), revision number or hash, or revset expression.
Cleaning in ICallable + changed fingerprint to plain char array to allow for dead object examination + additional check in FetchContext callback to avoid the unexplained rogue callbacks I have seen + protection in LoaderStateMachine::HandleSuccessMessage in case things go wrong anyway
author Benjamin Golinvaux <bgo@osimis.io>
date Sat, 31 Aug 2019 13:45:04 +0200
parents e4b028c1ede1
children 3abc47e051c8
files Framework/Loaders/LoaderStateMachine.cpp Framework/Messages/ICallable.h Framework/Messages/IObserver.h Framework/Oracle/WebAssemblyOracle.cpp
diffstat 4 files changed, 71 insertions(+), 51 deletions(-) [+]
line wrap: on
line diff
--- a/Framework/Loaders/LoaderStateMachine.cpp	Thu Aug 29 18:08:48 2019 +0200
+++ b/Framework/Loaders/LoaderStateMachine.cpp	Sat Aug 31 13:45:04 2019 +0200
@@ -139,19 +139,21 @@
     LOG(TRACE) << "LoaderStateMachine(" << std::hex << this << std::dec << ")::HandleSuccessMessage()";
     if (activeCommands_ <= 0) {
       LOG(ERROR) << "LoaderStateMachine(" << std::hex << this << std::dec << ")::HandleSuccessMessage : activeCommands_ should be > 0 but is: " << activeCommands_;
+      LOG(ERROR) << "LoaderStateMachine(" << std::hex << this << std::dec << ") fingerprint = " << GetFingerprint();
     }
-    activeCommands_--;
-
-    try
-    {
-      dynamic_cast<State&>(message.GetOrigin().GetPayload()).Handle(message);
-      Step();
-    }
-    catch (Orthanc::OrthancException& e)
-    {
-      LOG(ERROR) << "Error in the state machine, stopping all processing: " << 
-        e.What() << " Details: " << e.GetDetails();
-      Clear();
+    else {
+      activeCommands_--;
+      try
+      {
+        dynamic_cast<State&>(message.GetOrigin().GetPayload()).Handle(message);
+        Step();
+      }
+      catch (Orthanc::OrthancException& e)
+      {
+        LOG(ERROR) << "Error in the state machine, stopping all processing: " <<
+          e.What() << " Details: " << e.GetDetails();
+        Clear();
+      }
     }
   }
 
--- a/Framework/Messages/ICallable.h	Thu Aug 29 18:08:48 2019 +0200
+++ b/Framework/Messages/ICallable.h	Sat Aug 31 13:45:04 2019 +0200
@@ -78,19 +78,15 @@
 
     void ApplyInternal(const TMessage& message)
     {
-#if 0
-      (observer_.*function_) (message);
-#else
       if (observerFingerprint_ != observer_.GetFingerprint())
       {
-        LOG(WARNING) << "The observer at address " << std::hex << &observer_ << std::dec << ") has a different fingerprint than the one recorded at callback registration time. Callback will NOT be sent!";
-        LOG(WARNING) << " recorded fingerprint = " << observerFingerprint_ << " current fingerprint = " << observer_.GetFingerprint();
+        LOG(TRACE) << "The observer at address " << std::hex << &observer_ << std::dec << ") has a different fingerprint than the one recorded at callback registration time. This means that it is not the same object as the one recorded, even though their addresses are the same.  Callback will NOT be sent!";
+        LOG(TRACE) << " recorded fingerprint = " << observerFingerprint_ << " current fingerprint = " << observer_.GetFingerprint();
       }
       else
       {
         (observer_.*function_) (message);
       }
-#endif
     }
 
     virtual void Apply(const IMessage& message)
--- a/Framework/Messages/IObserver.h	Thu Aug 29 18:08:48 2019 +0200
+++ b/Framework/Messages/IObserver.h	Sat Aug 31 13:45:04 2019 +0200
@@ -34,12 +34,20 @@
     MessageBroker&  broker_;
     // the following is a UUID that is used to disambiguate different observers
     // that may have the same address
-    std::string     fingerprint_;
+    char     fingerprint_[37];
   public:
     IObserver(MessageBroker& broker)
       : broker_(broker)
-      , fingerprint_(Orthanc::Toolbox::GenerateUuid())
+      , fingerprint_()
     {
+      // we store the fingerprint_ as a char array to avoid problems when
+      // reading it in a deceased object.
+      // remember this is panic-level code to track zombie object usage
+      std::string fingerprint = Orthanc::Toolbox::GenerateUuid();
+      const char* fingerprintRaw = fingerprint.c_str();
+      ORTHANC_ASSERT(strlen(fingerprintRaw) == 36);
+      ORTHANC_ASSERT(fingerprintRaw[36] == 0);
+      memcpy(fingerprint_, fingerprintRaw, 37);
       LOG(TRACE) << "IObserver(" << std::hex << this << std::dec << ")::IObserver : fingerprint_ == " << fingerprint_;
       broker_.Register(*this);
     }
@@ -50,11 +58,27 @@
       broker_.Unregister(*this);
     }
 
-    const std::string& GetFingerprint() const
+    const char* GetFingerprint() const
     {
       return fingerprint_;
     }
 
+    bool DoesFingerprintLookGood() const
+    {
+      for (size_t i = 0; i < 36; ++i) {
+        bool ok = false;
+        if (fingerprint_[i] >= 'a' && fingerprint_[i] <= 'f')
+          ok = true;
+        if (fingerprint_[i] >= '0' && fingerprint_[i] <= '9')
+          ok = true;
+        if (fingerprint_[i] == '-')
+          ok = true;
+        if (!ok)
+          return false;
+      }
+      return fingerprint_[36] == 0;
+    }
+
     MessageBroker& GetBroker() const
     {
       return broker_;
--- a/Framework/Oracle/WebAssemblyOracle.cpp	Thu Aug 29 18:08:48 2019 +0200
+++ b/Framework/Oracle/WebAssemblyOracle.cpp	Sat Aug 31 13:45:04 2019 +0200
@@ -99,7 +99,6 @@
   private:
     Emitter                        emitter_;
     const IObserver&               receiver_;
-    std::string                    receiverFingerprint_;
     std::auto_ptr<IOracleCommand>  command_;
     std::string                    expectedContentType_;
 
@@ -110,7 +109,6 @@
                  const std::string& expectedContentType) :
       emitter_(oracle),
       receiver_(receiver),
-      receiverFingerprint_(receiver.GetFingerprint()),
       command_(command),
       expectedContentType_(expectedContentType)
     {
@@ -120,16 +118,6 @@
       }
     }
 
-    bool IsFingerprintOK() const
-    {
-      bool ok = receiverFingerprint_ == receiver_.GetFingerprint();
-      if (!ok)
-      {
-        LOG(TRACE) << "IsFingerprintOK returned false. receiverFingerprint_ = " << receiverFingerprint_ << " | receiver_(" << std::hex << (&receiver_) << std::dec << ").GetFingerprint() = " << receiver_.GetFingerprint();
-      }
-      return ok;
-    }
-
     const std::string& GetExpectedContentType() const
     {
       return expectedContentType_;
@@ -208,6 +196,18 @@
       
       std::auto_ptr<FetchContext> context(reinterpret_cast<FetchContext*>(fetch->userData));
 
+      // an UUID is 36 chars : 32 hex chars + 4 hyphens: char #0 --> char #35
+      // char #36 is \0.
+      
+      // TODO: remove this line because we are NOT allowed to call methods on GetReceiver that is maybe a dangling ref
+      if (context->GetReceiver().DoesFingerprintLookGood()) {
+        LOG(TRACE) << "SuccessCallback for object at address (" << std::hex << &(context->GetReceiver()) << std::dec << " with current fingerprint = " << context->GetReceiver().GetFingerprint();
+      }
+      else {
+        LOG(TRACE) << "SuccessCallback for object at address (" << std::hex << &(context->GetReceiver()) << std::dec << " with current fingerprint is XXXXX -- NOT A VALID FINGERPRINT! OBJECT IS READ ! CALLBACK WILL NOT BE CALLED!";
+      }
+      
+
       if (fetch->userData == NULL)
       {
         LOG(ERROR) << "WebAssemblyOracle::FetchContext::SuccessCallback fetch->userData is NULL!!!!!!!";
@@ -243,27 +243,25 @@
         DumpCommand(fetch, answer);
       }
 #endif
+      LOG(TRACE) << "About to call emscripten_fetch_close";
       emscripten_fetch_close(fetch);
+      LOG(TRACE) << "Successfully called emscripten_fetch_close";
 
       /**
        * Secondly, use the retrieved data.
-       * We only use the receiver if its fingerprint matches the one stored
-       * at command creation. 
+       * IMPORTANT NOTE: the receiver might be dead. This is prevented 
+       * by the object responsible for zombie check, later on.
        **/
-      if (!context->IsFingerprintOK())
-      {
-        LOG(WARNING) << "FetchContext::SuccessCallback -- the initial request initiator has been destroyed. Response will be discarded.";
-      }
-      else
+      try
       {
-        try
+        if (context.get() == NULL)
         {
-          if (context.get() == NULL)
-          {
-            throw Orthanc::OrthancException(Orthanc::ErrorCode_NullPointer);
-          }
-          else
-          {
+          LOG(ERROR) << "WebAssemblyOracle::FetchContext::SuccessCallback: (context.get() == NULL)";
+          throw Orthanc::OrthancException(Orthanc::ErrorCode_NullPointer);
+        }
+        else
+        {
+          if (context->GetReceiver().DoesFingerprintLookGood()) {
             switch (context->GetCommand().GetType())
             {
             case IOracleCommand::Type_OrthancRestApi:
@@ -294,10 +292,10 @@
             }
           }
         }
-        catch (Orthanc::OrthancException& e)
-        {
-          LOG(ERROR) << "Error while processing a fetch answer in the oracle: " << e.What();
-        }
+      }
+      catch (Orthanc::OrthancException& e)
+      {
+        LOG(ERROR) << "Error while processing a fetch answer in the oracle: " << e.What();
       }
     }