From 5ee2c612913f7dc82a33f8187b38ed49f7337f4d Mon Sep 17 00:00:00 2001 From: Nika Layzell Date: Tue, 10 Jul 2018 15:47:30 -0400 Subject: [PATCH] Bug 1474369 - Part 1: Clean up value initialization codepaths in XPConnect, r=mccr8 Summary: A goal of the Sequence work is to allow using more complex types within lists in XPConnect. For example, we ideally want to support `Sequence`, rather than requiring people to use the unergonomic 'wstring' type. These types require initialization before they can be read into. Currently this initialization for parameters is directly handled by XPCWrappedNative's CallMethodHelper object. This patch introduces a new function to the `xpc` namespace to initialize a specific value from an uninitialized state to a safe state. Reviewers: mccr8! Tags: #secure-revision Bug #: 1474369 Differential Revision: https://phabricator.services.mozilla.com/D2105 --- js/xpconnect/src/XPCConvert.cpp | 35 ++++++++ js/xpconnect/src/XPCWrappedNative.cpp | 113 ++++++++++---------------- js/xpconnect/src/xpcprivate.h | 11 +++ xpcom/reflect/xptcall/xptcall.h | 23 ++---- 4 files changed, 96 insertions(+), 86 deletions(-) diff --git a/js/xpconnect/src/XPCConvert.cpp b/js/xpconnect/src/XPCConvert.cpp index 467f81a3df77..769f94ab5b96 100644 --- a/js/xpconnect/src/XPCConvert.cpp +++ b/js/xpconnect/src/XPCConvert.cpp @@ -1668,6 +1668,11 @@ xpc::InnerCleanupValue(const nsXPTType& aType, void* aValue, uint32_t aArrayLen) free(elements); break; } + + // Clear the JS::Value to `undefined` + case nsXPTType::T_JSVAL: + ((JS::Value*)aValue)->setUndefined(); + break; } // Null out the pointer if we have it. @@ -1675,3 +1680,33 @@ xpc::InnerCleanupValue(const nsXPTType& aType, void* aValue, uint32_t aArrayLen) *(void**)aValue = nullptr; } } + +/***************************************************************************/ + +// Implementation of xpc::InitializeValue. + +void +xpc::InitializeValue(const nsXPTType& aType, void* aValue) +{ + switch (aType.Tag()) { + // Types which require custom, specific initialization. + case nsXPTType::T_JSVAL: + new (aValue) JS::Value(); + MOZ_ASSERT(reinterpret_cast(aValue)->isUndefined()); + break; + + case nsXPTType::T_ASTRING: + case nsXPTType::T_DOMSTRING: + new (aValue) nsString(); + break; + case nsXPTType::T_CSTRING: + case nsXPTType::T_UTF8STRING: + new (aValue) nsCString(); + break; + + // The remaining types all have valid states where all bytes are '0'. + default: + memset(aValue, 0, aType.Stride()); + break; + } +} diff --git a/js/xpconnect/src/XPCWrappedNative.cpp b/js/xpconnect/src/XPCWrappedNative.cpp index 8c43cdc1b801..1a42638b8469 100644 --- a/js/xpconnect/src/XPCWrappedNative.cpp +++ b/js/xpconnect/src/XPCWrappedNative.cpp @@ -1245,10 +1245,6 @@ CallMethodHelper::Call() CallMethodHelper::~CallMethodHelper() { for (nsXPTCVariant& param : mDispatchParams) { - // Only clean up values which need cleanup. - if (!param.DoesValNeedCleanup()) - continue; - uint32_t arraylen = 0; if (!GetArraySizeFromParam(param.type, UndefinedHandleValue, &arraylen)) continue; @@ -1477,25 +1473,39 @@ CallMethodHelper::InitializeDispatchParams() mJSContextIndex = mMethodInfo->IndexOfJSContext(); - // iterate through the params to clear flags (for safe cleanup later) - for (uint8_t i = 0; i < paramCount + wantsJSContext + wantsOptArgc; i++) { - nsXPTCVariant* dp = mDispatchParams.AppendElement(); - dp->ClearFlags(); - dp->val.p = nullptr; + // Allocate enough space in mDispatchParams up-front. + if (!mDispatchParams.AppendElements(paramCount + wantsJSContext + wantsOptArgc)) { + Throw(NS_ERROR_OUT_OF_MEMORY, mCallContext); + return false; } - // Fill in the JSContext argument - if (wantsJSContext) { - nsXPTCVariant* dp = &mDispatchParams[mJSContextIndex]; - dp->type = nsXPTType::T_VOID; - dp->val.p = mCallContext; - } + // Initialize each parameter to a valid state (for safe cleanup later). + for (uint8_t i = 0, paramIdx = 0; i < mDispatchParams.Length(); i++) { + nsXPTCVariant& dp = mDispatchParams[i]; - // Fill in the optional_argc argument - if (wantsOptArgc) { - nsXPTCVariant* dp = &mDispatchParams[mOptArgcIndex]; - dp->type = nsXPTType::T_U8; - dp->val.u8 = std::min(mArgc, paramCount) - requiredArgs; + if (i == mJSContextIndex) { + // Fill in the JSContext argument + dp.type = nsXPTType::T_VOID; + dp.val.p = mCallContext; + } else if (i == mOptArgcIndex) { + // Fill in the optional_argc argument + dp.type = nsXPTType::T_U8; + dp.val.u8 = std::min(mArgc, paramCount) - requiredArgs; + } else { + // Initialize normal arguments. + const nsXPTParamInfo& param = mMethodInfo->Param(paramIdx); + dp.type = param.Type(); + xpc::InitializeValue(dp.type, &dp.val); + + // Specify the correct storage/calling semantics. This will also set + // the `ptr` field to be self-referential. + if (param.IsIndirect()) { + dp.SetIndirect(); + } + + // Advance to the next normal parameter. + paramIdx++; + } } return true; @@ -1522,40 +1532,8 @@ bool CallMethodHelper::ConvertIndependentParam(uint8_t i) { const nsXPTParamInfo& paramInfo = mMethodInfo->GetParam(i); - const nsXPTType& type = paramInfo.GetType(); + const nsXPTType& type = paramInfo.Type(); nsXPTCVariant* dp = GetDispatchParam(i); - dp->type = type; - MOZ_ASSERT(!paramInfo.IsShared(), "[shared] implies [noscript]!"); - - // Specify the correct storage/calling semantics. - if (paramInfo.IsIndirect()) - dp->SetIndirect(); - - // Some types are always stored within the nsXPTCVariant, and passed - // indirectly, regardless of in/out-ness. These types are stored in the - // nsXPTCVariant's extended value. - switch (type.Tag()) { - // Ensure that the jsval has a valid value. - case nsXPTType::T_JSVAL: - new (&dp->ext.jsval) JS::Value(); - MOZ_ASSERT(dp->ext.jsval.isUndefined()); - break; - - // Initialize our temporary string class values so they can be assigned - // to by the XPCConvert logic. - case nsXPTType::T_ASTRING: - case nsXPTType::T_DOMSTRING: - new (&dp->ext.nsstr) nsString(); - break; - case nsXPTType::T_CSTRING: - case nsXPTType::T_UTF8STRING: - new (&dp->ext.nscstr) nsCString(); - break; - } - - // Flag cleanup for anything that isn't self-contained. - if (!type.IsArithmetic()) - dp->SetValNeedsCleanup(); // Even if there's nothing to convert, we still need to examine the // JSObject container for out-params. If it's null or otherwise invalid, @@ -1641,18 +1619,8 @@ bool CallMethodHelper::ConvertDependentParam(uint8_t i) { const nsXPTParamInfo& paramInfo = mMethodInfo->GetParam(i); - const nsXPTType& type = paramInfo.GetType(); - + const nsXPTType& type = paramInfo.Type(); nsXPTCVariant* dp = GetDispatchParam(i); - dp->type = type; - - // Specify the correct storage/calling semantics. - if (paramInfo.IsIndirect()) - dp->SetIndirect(); - - // Make sure we clean up all of our dependent types. All of them require - // allocations of some kind. - dp->SetValNeedsCleanup(); // Even if there's nothing to convert, we still need to examine the // JSObject container for out-params. If it's null or otherwise invalid, @@ -1712,14 +1680,18 @@ TraceParam(JSTracer* aTrc, void* aVal, const nsXPTType& aType, if (aType.Tag() == nsXPTType::T_JSVAL) { JS::UnsafeTraceRoot(aTrc, (JS::Value*)aVal, "XPCWrappedNative::CallMethod param"); + } else if (aType.Tag() == nsXPTType::T_SEQUENCE) { + auto* sequence = (xpt::detail::UntypedSequence*)aVal; + + const nsXPTType& elty = aType.ArrayElementType(); + for (uint32_t i = 0; i < sequence->Length(); ++i) { + TraceParam(aTrc, sequence->ElementAt(elty, i), elty); + } } else if (aType.Tag() == nsXPTType::T_ARRAY && *(void**)aVal) { const nsXPTType& elty = aType.ArrayElementType(); - if (elty.Tag() != nsXPTType::T_JSVAL) { - return; - } for (uint32_t i = 0; i < aArrayLen; ++i) { - TraceParam(aTrc, elty.ElementPtr(aVal, i), elty); + TraceParam(aTrc, elty.ElementPtr(*(void**)aVal, i), elty); } } } @@ -1729,9 +1701,8 @@ CallMethodHelper::trace(JSTracer* aTrc) { // We need to note each of our initialized parameters which contain jsvals. for (nsXPTCVariant& param : mDispatchParams) { - if (!param.DoesValNeedCleanup()) { - MOZ_ASSERT(param.type.Tag() != nsXPTType::T_JSVAL, - "JSVals are marked as needing cleanup (even though they don't)"); + // We only need to trace parameters which have an innermost JSVAL. + if (param.type.InnermostType().Tag() != nsXPTType::T_JSVAL) { continue; } diff --git a/js/xpconnect/src/xpcprivate.h b/js/xpconnect/src/xpcprivate.h index 658622bb670e..e73c8ede7845 100644 --- a/js/xpconnect/src/xpcprivate.h +++ b/js/xpconnect/src/xpcprivate.h @@ -3060,6 +3060,17 @@ void InnerCleanupValue(const nsXPTType& aType, void* aValue, uint32_t aArrayLen); +// In order to be able to safely call CleanupValue on a generated value, the +// data behind it needs to be initialized to a safe value. This method handles +// initializing the backing data to a safe value to use as an argument to +// XPCConvert methods, or xpc::CleanupValue. +// +// The pointer `aValue` must point to a block of memory at least aType.Stride() +// bytes large, and correctly aligned. +// +// This method accepts the same types as xpc::CleanupValue. +void InitializeValue(const nsXPTType& aType, void* aValue); + } // namespace xpc namespace mozilla { diff --git a/xpcom/reflect/xptcall/xptcall.h b/xpcom/reflect/xptcall/xptcall.h index 2085f3e9443d..d6ddd7ead9b5 100644 --- a/xpcom/reflect/xptcall/xptcall.h +++ b/xpcom/reflect/xptcall/xptcall.h @@ -45,8 +45,6 @@ static_assert(offsetof(nsXPTCMiniVariant, val) == 0, struct nsXPTCVariant { -// No ctors or dtors so that we can use arrays of these on the stack with no -// penalty. union ExtendedVal { // ExtendedVal is an extension on nsXPTCMiniVariant. It contains types @@ -79,6 +77,12 @@ struct nsXPTCVariant nsXPTType type; uint8_t flags; + // Clear to a valid, null state. + nsXPTCVariant() { + memset(this, 0, sizeof(nsXPTCVariant)); + type = nsXPTType::T_VOID; + } + enum { // @@ -88,22 +92,12 @@ struct nsXPTCVariant // Indicates that we &val.p should be passed n the stack, i.e. that // val should be passed by reference. IS_INDIRECT = 0x1, - - // Indicates that the value we hold requires some sort of cleanup (memory - // deallocation, interface release, JS::Value unrooting, etc). The precise - // cleanup that is performed depends on the 'type' field above. - // If the value is an array, this flag specifies whether the elements - // within the array require cleanup (we always clean up the array itself, - // so this flag would be redundant for that purpose). - VAL_NEEDS_CLEANUP = 0x2 }; void ClearFlags() {flags = 0;} void SetIndirect() {flags |= IS_INDIRECT;} - void SetValNeedsCleanup() {flags |= VAL_NEEDS_CLEANUP;} bool IsIndirect() const {return 0 != (flags & IS_INDIRECT);} - bool DoesValNeedCleanup() const {return 0 != (flags & VAL_NEEDS_CLEANUP);} // Implicitly convert to nsXPTCMiniVariant. operator nsXPTCMiniVariant&() { @@ -113,9 +107,8 @@ struct nsXPTCVariant return *(const nsXPTCMiniVariant*) &val; } - // As this type contains an anonymous union, we need to provide explicit - // constructors & destructors. - nsXPTCVariant() { } + // As this type contains an anonymous union, we need to provide an explicit + // destructor. ~nsXPTCVariant() { } };