From f7d2d40b3057f5bc249c88784b35443aad8de7aa Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Sun, 29 Oct 2023 11:56:17 -0400 Subject: DRTVWR-587: Fix LL::apply(function, LLSD array). We define a specialization of LLSDParam to support passing an LLSD object to a const char* function parameter. Needless to remark, passing object.asString().c_str() would be Bad: destroying the temporary std::string returned by asString() would immediately invalidate the pointer returned by its c_str(). But when you pass LLSDParam(object) as the parameter, that specialization itself stores the std::string so the c_str() pointer remains valid as long as the LLSDParam object does. Then there's LLSDParam, used when we don't have the parameter type available to select the LLSDParam specialization. LLSDParam defines a templated conversion operator T() that constructs an LLSDParam to provide the actual parameter value. So far, so good. The trouble was with the implementation of LLSDParam: it constructed a _temporary_ LLSDParam, implicitly called its operator T() and immediately destroyed it. Destroying LLSDParam destroyed its stored string, thus invalidating the c_str() pointer before the target function was entered. Instead, make LLSDParam::operator T() capture each LLSDParam it constructs, extending its lifespan to the lifespan of the LLSDParam instance. For this, derive each LLSDParam specialization from LLSDParamBase, a trivial base class that simply establishes the virtual destructor. We can then capture any specialization as a pointer to LLSDParamBase. Also restore LazyEventAPI tests on Mac. --- indra/llcommon/CMakeLists.txt | 8 +-- indra/llcommon/llsdutil.h | 75 ++++++++++++++++++------- indra/llcommon/tests/lleventdispatcher_test.cpp | 15 ++--- 3 files changed, 60 insertions(+), 38 deletions(-) (limited to 'indra') diff --git a/indra/llcommon/CMakeLists.txt b/indra/llcommon/CMakeLists.txt index 0df113e2c8..e02f69126e 100644 --- a/indra/llcommon/CMakeLists.txt +++ b/indra/llcommon/CMakeLists.txt @@ -298,13 +298,7 @@ if (LL_TESTS) LL_ADD_INTEGRATION_TEST(bitpack "" "${test_libs}") LL_ADD_INTEGRATION_TEST(classic_callback "" "${test_libs}") LL_ADD_INTEGRATION_TEST(commonmisc "" "${test_libs}") - if (WINDOWS) - # As of 2023-07-27, lazyeventapi.h triggers a bug in older clang, - # unfortunately the version we run on our TeamCity Mac build agent. As we - # move forward, either with an updated TC agent or GitHub builds, remove - # this 'if'. - LL_ADD_INTEGRATION_TEST(lazyeventapi "" "${test_libs}") - endif () + LL_ADD_INTEGRATION_TEST(lazyeventapi "" "${test_libs}") LL_ADD_INTEGRATION_TEST(llbase64 "" "${test_libs}") LL_ADD_INTEGRATION_TEST(llcond "" "${test_libs}") LL_ADD_INTEGRATION_TEST(lldate "" "${test_libs}") diff --git a/indra/llcommon/llsdutil.h b/indra/llcommon/llsdutil.h index cd68f938ea..ad54d1b0be 100644 --- a/indra/llcommon/llsdutil.h +++ b/indra/llcommon/llsdutil.h @@ -34,7 +34,9 @@ #include "llsd.h" #include #include +#include // std::shared_ptr #include +#include // U32 LL_COMMON_API LLSD ll_sd_from_U32(const U32); @@ -302,6 +304,11 @@ LLSD map(Ts&&... vs) /***************************************************************************** * LLSDParam *****************************************************************************/ +struct LLSDParamBase +{ + virtual ~LLSDParamBase() {} +}; + /** * LLSDParam is a customization point for passing LLSD values to function * parameters of more or less arbitrary type. LLSD provides a small set of @@ -319,7 +326,7 @@ LLSD map(Ts&&... vs) * @endcode */ template -class LLSDParam +class LLSDParam: public LLSDParamBase { public: /** @@ -327,13 +334,13 @@ public: * value for later retrieval */ LLSDParam(const LLSD& value): - _value(value) + value_(value) {} - operator T() const { return _value; } + operator T() const { return value_; } private: - T _value; + T value_; }; /** @@ -343,10 +350,30 @@ private: * specialization. */ template <> -class LLSDParam +class LLSDParam: public LLSDParamBase { private: LLSD value_; + // LLSDParam::operator T() works by instantiating an LLSDParam on + // demand. Returning that engages LLSDParam::operator T(), producing + // the desired result. But LLSDParam owns a std::string whose + // c_str() is returned by its operator const char*(). If we return a temp + // LLSDParam, the compiler can destroy it right away, as soon + // as we've called operator const char*(). That's a problem! That + // invalidates the const char* we've just passed to the subject function. + // This LLSDParam is presumably guaranteed to survive until the + // subject function has returned, so we must ensure that any constructed + // LLSDParam lives just as long as this LLSDParam does. Putting + // each LLSDParam on the heap and capturing a smart pointer in a vector + // works. We would have liked to use std::unique_ptr, but vector entries + // must be copyable. + // (Alternatively we could assume that every instance of LLSDParam + // will be asked for at most ONE conversion. We could store a scalar + // std::unique_ptr and, when constructing an new LLSDParam, assert that + // the unique_ptr is empty. But some future change in usage patterns, and + // consequent failure of that assertion, would be very mysterious. Instead + // of explaining how to fix it, just fix it now.) + mutable std::vector> converters_; public: LLSDParam(const LLSD& value): value_(value) {} @@ -358,7 +385,15 @@ public: /// otherwise, instantiate a more specific LLSDParam to convert; that /// preserves the existing customization mechanism template - operator T() const { return LLSDParam>(value_); } + operator T() const + { + // capture 'ptr' with the specific subclass type because converters_ + // only stores LLSDParamBase pointers + auto ptr{ std::make_shared>>(value_) }; + // keep the new converter alive until we ourselves are destroyed + converters_.push_back(ptr); + return *ptr; + } }; /** @@ -375,17 +410,17 @@ public: */ #define LLSDParam_for(T, AS) \ template <> \ -class LLSDParam \ +class LLSDParam: public LLSDParamBase \ { \ public: \ LLSDParam(const LLSD& value): \ - _value((T)value.AS()) \ + value_((T)value.AS()) \ {} \ \ - operator T() const { return _value; } \ + operator T() const { return value_; } \ \ private: \ - T _value; \ + T value_; \ } LLSDParam_for(float, asReal); @@ -401,31 +436,31 @@ LLSDParam_for(LLSD::Binary, asBinary); * safely pass an LLSDParam(yourLLSD). */ template <> -class LLSDParam +class LLSDParam: public LLSDParamBase { private: // The difference here is that we store a std::string rather than a const // char*. It's important that the LLSDParam object own the std::string. - std::string _value; + std::string value_; // We don't bother storing the incoming LLSD object, but we do have to - // distinguish whether _value is an empty string because the LLSD object + // distinguish whether value_ is an empty string because the LLSD object // contains an empty string or because it's isUndefined(). - bool _undefined; + bool undefined_; public: LLSDParam(const LLSD& value): - _value(value), - _undefined(value.isUndefined()) + value_(value), + undefined_(value.isUndefined()) {} - // The const char* we retrieve is for storage owned by our _value member. + // The const char* we retrieve is for storage owned by our value_ member. // That's how we guarantee that the const char* is valid for the lifetime // of this LLSDParam object. Constructing your LLSDParam in the argument // list should ensure that the LLSDParam object will persist for the // duration of the function call. operator const char*() const { - if (_undefined) + if (undefined_) { // By default, an isUndefined() LLSD object's asString() method // will produce an empty string. But for a function accepting @@ -435,7 +470,7 @@ public: // case, though, no LLSD value could pass NULL. return NULL; } - return _value.c_str(); + return value_.c_str(); } }; @@ -592,7 +627,7 @@ namespace LL * apply(function, LLSD array) *****************************************************************************/ // validate incoming LLSD blob, and return an LLSD array suitable to pass to -// apply_impl() +// the function of interest LLSD apply_llsd_fix(size_t arity, const LLSD& args); // Derived from https://stackoverflow.com/a/20441189 diff --git a/indra/llcommon/tests/lleventdispatcher_test.cpp b/indra/llcommon/tests/lleventdispatcher_test.cpp index 40643172ee..b0c532887c 100644 --- a/indra/llcommon/tests/lleventdispatcher_test.cpp +++ b/indra/llcommon/tests/lleventdispatcher_test.cpp @@ -178,6 +178,7 @@ struct Vars /*-------- Arbitrary-params (non-const, const, static) methods ---------*/ void methodna(NPARAMSa) { + DEBUG; // Because our const char* param cp might be NULL, and because we // intend to capture the value in a std::string, have to distinguish // between the NULL value and any non-NULL value. Use a convention @@ -189,7 +190,7 @@ struct Vars else vcp = std::string("'") + cp + "'"; - debug()("methodna(", b, + this->debug()("methodna(", b, ", ", i, ", ", f, ", ", d, @@ -227,7 +228,8 @@ struct Vars void cmethodna(NPARAMSa) const { - debug()('c', NONL); + DEBUG; + this->debug()('c', NONL); const_cast(this)->methodna(NARGSa); } @@ -1200,9 +1202,6 @@ namespace tut void object::test<20>() { set_test_name("call array-style functions with right-size arrays"); -#if defined(_MSC_VER) - skip("This test fails on VS"); -#endif std::vector binary; for (size_t h(0x01), i(0); i < 5; h+= 0x22, ++i) { @@ -1241,9 +1240,6 @@ namespace tut void object::test<21>() { set_test_name("verify that passing LLSD() to const char* sends NULL"); -#if defined(_MSC_VER) - skip("This test fails on VS"); -#endif ensure_equals("Vars::cp init", v.cp, ""); work("methodna_map_mdft", LLSDMap("cp", LLSD())); @@ -1257,9 +1253,6 @@ namespace tut template<> template<> void object::test<22>() { -#if defined(_MSC_VER) - skip("This test fails on VS"); -#endif set_test_name("call map-style functions with (full | oversized) (arrays | maps)"); const char binary[] = "\x99\x88\x77\x66\x55"; LLSD array_full(LLSDMap -- cgit v1.2.3