Bug 1475409 - Part 3: Make the different categories of types in xptinfo more explicit, r=mccr8

Summary:
This should make it more clear which types have which behaviours, and should
make it easier to add new types without forgetting to handle a special case
somewhere.

Depends On D2114

Reviewers: mccr8!

Tags: #secure-revision

Bug #: 1475409

Differential Revision: https://phabricator.services.mozilla.com/D2115

js/xpconnect/src/XPCConvert.cpp

@@ -1637,9 +1637,9 @@ xpc::InnerCleanupValue(const nsXPTType& aType, void* aValue, uint32_t aArrayLen)
             MOZ_CRASH("Unknown Type!");
     }
 
-    // Null out the pointer if we have it.
+    // Clear any non-complex values to the valid '0' state.
-    if (aType.HasPointerRepr()) {
+    if (!aType.IsComplex()) {
-        *(void**)aValue = nullptr;
+        aType.ZeroValue(aValue);
     }
 }
 
@@ -1672,7 +1672,7 @@ xpc::InitializeValue(const nsXPTType& aType, void* aValue)
 
         // The remaining types all have valid states where all bytes are '0'.
         default:
-            memset(aValue, 0, aType.Stride());
+            aType.ZeroValue(aValue);
             break;
     }
 }

js/xpconnect/src/XPCInlines.h

@@ -530,7 +530,10 @@ xpc::CleanupValue(const nsXPTType& aType,
     // Check if we can do a cheap early return, and only perform the inner call
     // if we can't. We never have to clean up null pointer types or arithmetic
     // types.
-    if (aType.IsArithmetic() || (aType.HasPointerRepr() && !*(void**)aValue)) {
+    //
+    // NOTE: We can skip zeroing arithmetic types in CleanupValue, as they are
+    // already in a valid state.
+    if (aType.IsArithmetic() || (aType.IsPointer() && !*(void**)aValue)) {
         return;
     }
     xpc::InnerCleanupValue(aType, aValue, aArrayLen);

js/xpconnect/src/XPCWrappedJSClass.cpp

@@ -745,25 +745,27 @@ nsXPCWrappedJSClass::CleanupOutparams(const nsXPTMethodInfo* info,
         if (!param.IsOut())
             continue;
 
-        // Extract the array length so we can use it in CleanupValue.
+        MOZ_ASSERT(param.IsIndirect(), "Outparams are always indirect");
+
+        // Call 'CleanupValue' on parameters which we know to be initialized:
+        //  1. Complex parameters (initialized by caller)
+        //  2. 'inout' parameters (initialized by caller)
+        //  3. 'out' parameters when 'inOutOnly' is 'false' (initialized by us)
+        //
+        // We skip non-complex 'out' parameters before the call, as they may
+        // contain random junk.
+        if (param.Type().IsComplex() || param.IsIn() || !inOutOnly) {
             uint32_t arrayLen = 0;
             if (!GetArraySizeFromParam(info, param.Type(), nativeParams, &arrayLen))
                 continue;
 
-        MOZ_ASSERT(param.IsIndirect(), "Outparams are always indirect");
-
-        // The inOutOnly flag is necessary because full outparams may contain
-        // uninitialized junk before the call is made, and we don't want to try
-        // to clean up uninitialized junk.
-        if (!inOutOnly || param.IsIn()) {
             xpc::CleanupValue(param.Type(), nativeParams[i].val.p, arrayLen);
         }
 
-        // Even if we didn't call CleanupValue, null out any pointers. This is
+        // Ensure our parameters are in a clean state. Complex values are always
-        // just to protect C++ callers which may read garbage if they forget to
+        // handled by CleanupValue, and others have a valid null representation.
-        // check the error value.
+        if (!param.Type().IsComplex()) {
-        if (param.Type().HasPointerRepr()) {
+            param.Type().ZeroValue(nativeParams[i].val.p);
-            *(void**)nativeParams[i].val.p = nullptr;
         }
     }
 }

xpcom/reflect/xptinfo/xptinfo.h

@@ -154,6 +154,13 @@ static_assert(sizeof(nsXPTInterfaceInfo) == 28, "wrong size?");
 enum nsXPTTypeTag : uint8_t
 {
   // Arithmetic (POD) Types
+  //  - Do not require cleanup,
+  //  - All bit patterns are valid,
+  //  - Outparams may be uninitialized by caller,
+  //  - Directly supported in xptcall.
+  //
+  // NOTE: The name 'Arithmetic' comes from Harbison/Steele. Despite being a tad
+  // unclear, it is used frequently in xptcall, so is unlikely to be changed.
   TD_INT8              = 0,
   TD_INT16             = 1,
   TD_INT32             = 2,
@@ -167,27 +174,42 @@ enum nsXPTTypeTag : uint8_t
   TD_BOOL              = 10,
   TD_CHAR              = 11,
   TD_WCHAR             = 12,
+  _TD_LAST_ARITHMETIC  = TD_WCHAR,
 
-  // Non-Arithmetic Types
+  // Pointer Types
+  //  - Require cleanup unless NULL,
+  //  - All-zeros (NULL) bit pattern is valid,
+  //  - Outparams may be uninitialized by caller,
+  //  - Supported in xptcall as raw pointer.
   TD_VOID              = 13,
   TD_PNSIID            = 14,
-  TD_DOMSTRING         = 15,
+  TD_PSTRING           = 15,
-  TD_PSTRING           = 16,
+  TD_PWSTRING          = 16,
-  TD_PWSTRING          = 17,
+  TD_INTERFACE_TYPE    = 17,
-  TD_INTERFACE_TYPE    = 18,
+  TD_INTERFACE_IS_TYPE = 18,
-  TD_INTERFACE_IS_TYPE = 19,
+  TD_ARRAY             = 19,
-  TD_ARRAY             = 20,
+  TD_PSTRING_SIZE_IS   = 20,
-  TD_PSTRING_SIZE_IS   = 21,
+  TD_PWSTRING_SIZE_IS  = 21,
-  TD_PWSTRING_SIZE_IS  = 22,
+  TD_DOMOBJECT         = 22,
-  TD_UTF8STRING        = 23,
+  TD_PROMISE           = 23,
-  TD_CSTRING           = 24,
+  _TD_LAST_POINTER     = TD_PROMISE,
-  TD_ASTRING           = 25,
+
-  TD_JSVAL             = 26,
+  // Complex Types
-  TD_DOMOBJECT         = 27,
+  //  - Require cleanup,
-  TD_PROMISE           = 28,
+  //  - Always passed indirectly,
-  TD_SEQUENCE          = 29
+  //  - Outparams must be initialized by caller,
+  //  - Supported in xptcall due to indirection.
+  TD_DOMSTRING         = 24,
+  TD_UTF8STRING        = 25,
+  TD_CSTRING           = 26,
+  TD_ASTRING           = 27,
+  TD_JSVAL             = 28,
+  TD_SEQUENCE          = 29,
+  _TD_LAST_COMPLEX     = TD_SEQUENCE
 };
 
+static_assert(_TD_LAST_COMPLEX < 32, "nsXPTTypeTag must fit in 5 bits");
+
 
 /*
  * A nsXPTType is a union used to identify the type of a method argument or
@@ -239,33 +261,22 @@ public:
     return xpt::detail::GetDOMObjectInfo(Data16());
   }
 
-  // 'Arithmetic' here roughly means that the value is self-contained and
+  // See the comments in nsXPTTypeTag for an explanation as to what each of
-  // doesn't depend on anything else in memory (ie: not a pointer, not an
+  // these categories mean.
-  // XPCOM object, not a jsval, etc).
+  bool IsArithmetic() const { return Tag() <= _TD_LAST_ARITHMETIC; }
-  //
+  bool IsPointer() const { return !IsArithmetic() && Tag() <= _TD_LAST_POINTER; }
-  // Supposedly this terminology comes from Harbison/Steele, but it's still
+  bool IsComplex() const { return Tag() > _TD_LAST_POINTER; }
-  // a rather crappy name. We'd change it if it wasn't used all over the
-  // place in xptcall. :-(
-  bool IsArithmetic() const { return Tag() <= TD_WCHAR; }
 
   bool IsInterfacePointer() const {
     return Tag() == TD_INTERFACE_TYPE || Tag() == TD_INTERFACE_IS_TYPE;
   }
 
-  bool IsArray() const { return Tag() == TD_ARRAY; }
-
   bool IsDependent() const {
     return (Tag() == TD_SEQUENCE && InnermostType().IsDependent()) ||
            Tag() == TD_INTERFACE_IS_TYPE || Tag() == TD_ARRAY ||
            Tag() == TD_PSTRING_SIZE_IS || Tag() == TD_PWSTRING_SIZE_IS;
   }
 
-  bool IsAlwaysIndirect() const {
-    return Tag() == TD_ASTRING || Tag() == TD_DOMSTRING ||
-           Tag() == TD_CSTRING || Tag() == TD_UTF8STRING ||
-           Tag() == TD_JSVAL || Tag() == TD_SEQUENCE;
-  }
-
   // Unwrap a nested type to its innermost value (e.g. through arrays).
   const nsXPTType& InnermostType() const {
     if (Tag() == TD_ARRAY || Tag() == TD_SEQUENCE) {
@@ -274,15 +285,20 @@ public:
     return *this;
   }
 
-  // Helper methods for working with the type's native representation.
+  // In-memory size of native type in bytes.
   inline size_t Stride() const;
-  inline bool HasPointerRepr() const;
 
   // Offset the given base pointer to reference the element at the given index.
   void* ElementPtr(const void* aBase, uint32_t aIndex) const {
     return (char*)aBase + (aIndex * Stride());
   }
 
+  // Zero out a native value of the given type. The type must not be 'complex'.
+  void ZeroValue(void* aValue) const {
+    MOZ_RELEASE_ASSERT(!IsComplex(), "Cannot zero a complex value");
+    memset(aValue, 0, Stride());
+  }
+
   // Indexes into the extra types array of a small set of known types.
   enum class Idx : uint8_t
   {
@@ -394,10 +410,10 @@ struct nsXPTParamInfo
   const nsXPTType& GetType() const { return Type(); } // XXX remove (backcompat)
 
   // Whether this parameter is passed indirectly on the stack. All out/inout
-  // params are passed indirectly, although some types are passed indirectly
+  // params are passed indirectly, and complex types are always passed
-  // unconditionally.
+  // indirectly.
   bool IsIndirect() const {
-    return IsOut() || Type().IsAlwaysIndirect();
+    return IsOut() || Type().IsComplex();
   }
 
   ////////////////////////////////////////////////////////////////
@@ -634,29 +650,6 @@ GetString(uint32_t aIndex)
 } // namespace detail
 } // namespace xpt
 
-inline bool
-nsXPTType::HasPointerRepr() const
-{
-  // This method should return `true` if the given type would be represented as
-  // a pointer when not passed indirectly.
-  switch (Tag()) {
-    case TD_VOID:
-    case TD_PNSIID:
-    case TD_PSTRING:
-    case TD_PWSTRING:
-    case TD_INTERFACE_TYPE:
-    case TD_INTERFACE_IS_TYPE:
-    case TD_ARRAY:
-    case TD_PSTRING_SIZE_IS:
-    case TD_PWSTRING_SIZE_IS:
-    case TD_DOMOBJECT:
-    case TD_PROMISE:
-        return true;
-    default:
-        return false;
-  }
-}
-
 inline size_t
 nsXPTType::Stride() const
 {
@@ -675,6 +668,7 @@ nsXPTType::Stride() const
     case TD_BOOL:              return sizeof(bool);
     case TD_CHAR:              return sizeof(char);
     case TD_WCHAR:             return sizeof(char16_t);
+
     case TD_VOID:              return sizeof(void*);
     case TD_PNSIID:            return sizeof(nsIID*);
     case TD_DOMSTRING:         return sizeof(nsString);
@@ -685,12 +679,13 @@ nsXPTType::Stride() const
     case TD_ARRAY:             return sizeof(void*);
     case TD_PSTRING_SIZE_IS:   return sizeof(char*);
     case TD_PWSTRING_SIZE_IS:  return sizeof(char16_t*);
+    case TD_DOMOBJECT:         return sizeof(void*);
+    case TD_PROMISE:           return sizeof(void*);
+
     case TD_UTF8STRING:        return sizeof(nsCString);
     case TD_CSTRING:           return sizeof(nsCString);
     case TD_ASTRING:           return sizeof(nsString);
     case TD_JSVAL:             return sizeof(JS::Value);
-    case TD_DOMOBJECT:         return sizeof(void*);
-    case TD_PROMISE:           return sizeof(void*);
     case TD_SEQUENCE:          return sizeof(xpt::detail::UntypedSequence);
   }