# HG changeset patch # User Benjamin Golinvaux # Date 1567251904 -7200 # Node ID e75fd08d6c7594f3a94b480cfef69c01f7090a41 # Parent e4b028c1ede1eee606035e286849bccb4fe0271d 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 diff -r e4b028c1ede1 -r e75fd08d6c75 Framework/Loaders/LoaderStateMachine.cpp --- 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(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(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(); + } } } diff -r e4b028c1ede1 -r e75fd08d6c75 Framework/Messages/ICallable.h --- 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) diff -r e4b028c1ede1 -r e75fd08d6c75 Framework/Messages/IObserver.h --- 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_; diff -r e4b028c1ede1 -r e75fd08d6c75 Framework/Oracle/WebAssemblyOracle.cpp --- 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 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 context(reinterpret_cast(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(); } }