Bug 1474369 - Part 1: Clean up value initialization codepaths in XPConnect, r=mccr8

Summary:
A goal of the Sequence<T> work is to allow using more complex types within lists
in XPConnect. For example, we ideally want to support `Sequence<AString>`,
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

@@ -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<JS::Value*>(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;
+    }
+}

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;
     }
 
+    // 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];
+
+        if (i == mJSContextIndex) {
             // Fill in the JSContext argument
-    if (wantsJSContext) {
-        nsXPTCVariant* dp = &mDispatchParams[mJSContextIndex];
-        dp->type = nsXPTType::T_VOID;
-        dp->val.p = mCallContext;
+            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<uint32_t>(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();
             }
 
-    // Fill in the optional_argc argument
-    if (wantsOptArgc) {
-        nsXPTCVariant* dp = &mDispatchParams[mOptArgcIndex];
-        dp->type = nsXPTType::T_U8;
-        dp->val.u8 = std::min<uint32_t>(mArgc, paramCount) - requiredArgs;
+            // 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;
         }
 

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 {

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() { }
 };