From cd64842e338fb216f0772e371278c56b921f6f87 Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Thu, 18 Jul 2024 14:24:36 -0400 Subject: Improve viewer's defense against `LLEventAPI` failures. `LLEventAPI` is specifically intended to allow a LEAP plugin, or a Lua script, to access certain viewer functionality. Errors in external code like that cannot be addressed during viewer development. Any code path that allows external code in any form to crash the viewer opens up a potential abuse vector, if a trusting user runs external code from an untrustworthy source. `LLDispatchListener` reports exceptions back to its invoker, if the invoker provides a "reply" `LLEventPump` name. Absent "reply", though, `LLDispatchListener` is documented to let any such exception propagate. That behavior may be okay for internal use, but in the case of the `LLEventAPI` subclass, it veers into the abuse scenario described above. Make `LLEventAPI` ensure that any exception propagating from `LLDispatchListener` is caught and logged, but not propagated. Also enrich error reporting for the "batch" `LLDispatchListener` operations. --- indra/llcommon/lleventapi.cpp | 20 ++++++++++++++++++++ indra/llcommon/lleventapi.h | 1 + indra/llcommon/lleventdispatcher.cpp | 15 +++++++++------ indra/llcommon/lleventdispatcher.h | 4 +++- 4 files changed, 33 insertions(+), 7 deletions(-) (limited to 'indra/llcommon') diff --git a/indra/llcommon/lleventapi.cpp b/indra/llcommon/lleventapi.cpp index 8b724256b8..4672371b4f 100644 --- a/indra/llcommon/lleventapi.cpp +++ b/indra/llcommon/lleventapi.cpp @@ -55,6 +55,26 @@ LLEventAPI::~LLEventAPI() { } +bool LLEventAPI::process(const LLSD& event) const +{ + // LLDispatchListener is documented to let DispatchError propagate if the + // incoming request has no "reply" key. That may be fine for internal-only + // use, but LLEventAPI opens the door for external requests. It should NOT + // be possible for any external requester to crash the viewer with an + // unhandled exception, especially not by something as simple as omitting + // the "reply" key. + try + { + return LLDispatchListener::process(event); + } + catch (const std::exception& err) + { + // log the exception, but otherwise ignore it + LL_WARNS("LLEventAPI") << LLError::Log::classname(err) << ": " << err.what() << LL_ENDL; + return false; + } +} + LLEventAPI::Response::Response(const LLSD& seed, const LLSD& request, const LLSD::String& replyKey): mResp(seed), mReq(request), diff --git a/indra/llcommon/lleventapi.h b/indra/llcommon/lleventapi.h index 3ae820db51..da7c58e6f0 100644 --- a/indra/llcommon/lleventapi.h +++ b/indra/llcommon/lleventapi.h @@ -157,6 +157,7 @@ protected: LLEventAPI(const LL::LazyEventAPIParams&); private: + bool process(const LLSD& event) const override; std::string mDesc; }; diff --git a/indra/llcommon/lleventdispatcher.cpp b/indra/llcommon/lleventdispatcher.cpp index 9dd6864cff..12b966fadf 100644 --- a/indra/llcommon/lleventdispatcher.cpp +++ b/indra/llcommon/lleventdispatcher.cpp @@ -800,7 +800,7 @@ void LLDispatchListener::call_one(const LLSD& name, const LLSD& event) const { result = (*this)(event); } - catch (const DispatchError& err) + catch (const std::exception& err) { if (! event.has(mReplyKey)) { @@ -810,7 +810,10 @@ void LLDispatchListener::call_one(const LLSD& name, const LLSD& event) const // Here there was an error and the incoming event has mReplyKey. Reply // with a map containing an "error" key explaining the problem. - return reply(llsd::map("error", err.what()), event); + return reply(llsd::map("error", + stringize(LLError::Log::classname(err), + ": ", err.what())), + event); } // We seem to have gotten a valid result. But we don't know whether the @@ -846,7 +849,7 @@ void LLDispatchListener::call_map(const LLSD& reqmap, const LLSD& event) const { // in case of errors, tell user the dispatch key, the fact that // we're processing a request map and the current key in that map - SetState(this, '[', key, '[', name, "]]"); + SetState transient(this, '[', key, '[', name, "]]"); // With this form, capture return value even if undefined: // presence of the key in the response map can be used to detect // which request keys succeeded. @@ -869,7 +872,7 @@ void LLDispatchListener::call_map(const LLSD& reqmap, const LLSD& event) const if (! event.has(mReplyKey)) { // can't send reply, throw - sCallFail(error); + callFail(error); } else { @@ -923,7 +926,7 @@ void LLDispatchListener::call_array(const LLSD& reqarray, const LLSD& event) con // in case of errors, tell user the dispatch key, the fact that // we're processing a request array, the current entry in that // array and the corresponding callable name - SetState(this, '[', key, '[', i, "]=", name, ']'); + SetState transient(this, '[', key, '[', i, "]=", name, ']'); // With this form, capture return value even if undefined results.append((*this)(name, args)); } @@ -944,7 +947,7 @@ void LLDispatchListener::call_array(const LLSD& reqarray, const LLSD& event) con if (! event.has(mReplyKey)) { // can't send reply, throw - sCallFail(error); + callFail(error); } else { diff --git a/indra/llcommon/lleventdispatcher.h b/indra/llcommon/lleventdispatcher.h index 5adaa3ebae..698412fdb4 100644 --- a/indra/llcommon/lleventdispatcher.h +++ b/indra/llcommon/lleventdispatcher.h @@ -853,8 +853,10 @@ public: std::string getPumpName() const { return getName(); } +protected: + virtual bool process(const LLSD& event) const; + private: - bool process(const LLSD& event) const; void call_one(const LLSD& name, const LLSD& event) const; void call_map(const LLSD& reqmap, const LLSD& event) const; void call_array(const LLSD& reqarray, const LLSD& event) const; -- cgit v1.2.3