Bug 1461821 - Call the relevant scope-data constructor when allocating it, and poison/mark as undefined the memory for the trailing array of BindingNames, ratther than impermissibly PodZero-ing non-trivial classes willy-nilly. r=jandem
authorJeff Walden <jwalden@mit.edu>
Wed, 16 May 2018 10:43:51 -0700
changeset 796222 2ddbcf79bc48390a605ffe053b21a834521e6cb6
parent 796221 73c36389c5f77846d89754386ef6f2d319aea86c
child 796223 19d2aace5b3ca316f548906b4283ebad44d3c8dd
push id110183
push userbmo:mh+mozilla@glandium.org
push dateThu, 17 May 2018 09:00:08 +0000
reviewersjandem
bugs1461821
milestone62.0a1
Bug 1461821 - Call the relevant scope-data constructor when allocating it, and poison/mark as undefined the memory for the trailing array of BindingNames, ratther than impermissibly PodZero-ing non-trivial classes willy-nilly. r=jandem
js/src/ds/LifoAlloc.h
js/src/frontend/Parser.cpp
js/src/vm/Scope.cpp
js/src/vm/Scope.h
--- a/js/src/ds/LifoAlloc.h
+++ b/js/src/ds/LifoAlloc.h
@@ -11,16 +11,18 @@
 #include "mozilla/MathAlgorithms.h"
 #include "mozilla/MemoryChecking.h"
 #include "mozilla/MemoryReporting.h"
 #include "mozilla/Move.h"
 #include "mozilla/PodOperations.h"
 #include "mozilla/TemplateLib.h"
 #include "mozilla/TypeTraits.h"
 
+#include <new>
+
 // This data structure supports stacky LIFO allocation (mark/release and
 // LifoAllocScope). It does not maintain one contiguous segment; instead, it
 // maintains a bunch of linked memory segments. In order to prevent malloc/free
 // thrashing, unused segments are deallocated when garbage collection occurs.
 
 #include "jsutil.h"
 
 #include "js/UniquePtr.h"
@@ -588,16 +590,30 @@ class LifoAlloc
         // Only simulate OOMs when we are not using the LifoAlloc as an
         // infallible allocator.
         if (fallibleScope_)
             JS_OOM_POSSIBLY_FAIL();
 #endif
         return allocImpl(n);
     }
 
+    template<typename T, typename... Args>
+    MOZ_ALWAYS_INLINE T*
+    allocInSize(size_t n, Args&&... args)
+    {
+        MOZ_ASSERT(n >= sizeof(T), "must request enough space to store a T");
+        static_assert(alignof(T) <= detail::LIFO_ALLOC_ALIGN,
+                      "LifoAlloc must provide enough alignment to store T");
+        void* ptr = alloc(n);
+        if (!ptr)
+            return nullptr;
+
+        return new (ptr) T(mozilla::Forward<Args>(args)...);
+    }
+
     MOZ_ALWAYS_INLINE
     void* allocInfallible(size_t n) {
         AutoEnterOOMUnsafeRegion oomUnsafe;
         if (void* result = allocImpl(n))
             return result;
         oomUnsafe.crash("LifoAlloc::allocInfallible");
         return nullptr;
     }
--- a/js/src/frontend/Parser.cpp
+++ b/js/src/frontend/Parser.cpp
@@ -1757,23 +1757,21 @@ Parser<FullParseHandler, CharT>::checkSt
     }
     return true;
 }
 
 template <typename Scope>
 typename Scope::Data*
 NewEmptyBindingData(JSContext* cx, LifoAlloc& alloc, uint32_t numBindings)
 {
+    using Data = typename Scope::Data;
     size_t allocSize = Scope::sizeOfData(numBindings);
-    auto* bindings = static_cast<typename Scope::Data*>(alloc.alloc(allocSize));
-    if (!bindings) {
+    auto* bindings = alloc.allocInSize<Data>(allocSize, numBindings);
+    if (!bindings)
         ReportOutOfMemory(cx);
-        return nullptr;
-    }
-    PodZero(bindings);
     return bindings;
 }
 
 /**
  * Copy-construct |BindingName|s from |bindings| into |cursor|, then return
  * the location one past the newly-constructed |BindingName|s.
  */
 static MOZ_MUST_USE BindingName*
--- a/js/src/vm/Scope.cpp
+++ b/js/src/vm/Scope.cpp
@@ -199,22 +199,22 @@ PrepareScopeData(JSContext* cx, BindingI
 
     return true;
 }
 
 template <typename ConcreteScope>
 static UniquePtr<typename ConcreteScope::Data>
 NewEmptyScopeData(JSContext* cx, uint32_t length = 0)
 {
-    uint8_t* bytes = cx->zone()->pod_calloc<uint8_t>(ConcreteScope::sizeOfData(length));
+    uint8_t* bytes = cx->zone()->pod_malloc<uint8_t>(ConcreteScope::sizeOfData(length));
     if (!bytes)
         ReportOutOfMemory(cx);
     auto data = reinterpret_cast<typename ConcreteScope::Data*>(bytes);
     if (data)
-        new (data) typename ConcreteScope::Data();
+        new (data) typename ConcreteScope::Data(length);
     return UniquePtr<typename ConcreteScope::Data>(data);
 }
 
 static XDRResult
 XDRBindingName(XDRState<XDR_ENCODE>* xdr, BindingName* bindingName)
 {
     JSContext* cx = xdr->cx();
 
--- a/js/src/vm/Scope.h
+++ b/js/src/vm/Scope.h
@@ -5,16 +5,18 @@
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 #ifndef vm_Scope_h
 #define vm_Scope_h
 
 #include "mozilla/Maybe.h"
 #include "mozilla/Variant.h"
 
+#include "jsutil.h"
+
 #include "gc/DeletePolicy.h"
 #include "gc/Heap.h"
 #include "gc/Policy.h"
 #include "js/UbiNode.h"
 #include "js/UniquePtr.h"
 #include "vm/BytecodeUtil.h"
 #include "vm/JSObject.h"
 #include "vm/Xdr.h"
@@ -152,16 +154,25 @@ class TrailingNamesArray
     // -Werror compile error) to reinterpret_cast<> |data_| to |T*|, even
     // through |void*|.  Placing the latter cast in these separate functions
     // breaks the chain such that affected GCC versions no longer warn/error.
     void* ptr() {
         return data_;
     }
 
   public:
+    // Explicitly ensure no one accidentally allocates scope data without
+    // poisoning its trailing names.
+    TrailingNamesArray() = delete;
+
+    explicit TrailingNamesArray(size_t nameCount) {
+        if (nameCount)
+            JS_POISON(&data_, 0xCC, sizeof(BindingName) * nameCount, MemCheckKind::MakeUndefined);
+    }
+
     BindingName* start() { return reinterpret_cast<BindingName*>(ptr()); }
 
     BindingName& operator[](size_t i) { return start()[i]; }
 };
 
 class BindingLocation
 {
   public:
@@ -407,27 +418,30 @@ class LexicalScope : public Scope
     // Data is public because it is created by the frontend. See
     // Parser<FullParseHandler>::newLexicalScopeData.
     struct Data
     {
         // Bindings are sorted by kind in both frames and environments.
         //
         //   lets - [0, constStart)
         // consts - [constStart, length)
-        uint32_t constStart;
-        uint32_t length;
+        uint32_t constStart = 0;
+        uint32_t length = 0;
 
         // Frame slots [0, nextFrameSlot) are live when this is the innermost
         // scope.
-        uint32_t nextFrameSlot;
+        uint32_t nextFrameSlot = 0;
 
         // The array of tagged JSAtom* names, allocated beyond the end of the
         // struct.
         TrailingNamesArray trailingNames;
 
+        explicit Data(size_t nameCount) : trailingNames(nameCount) {}
+        Data() = delete;
+
         void trace(JSTracer* trc);
     };
 
     static size_t sizeOfData(uint32_t length) {
         return sizeof(Data) + (length ? length - 1 : 0) * sizeof(BindingName);
     }
 
     static void getDataNamesAndLength(Data* data, BindingName** names, uint32_t* length) {
@@ -509,47 +523,50 @@ class FunctionScope : public Scope
   public:
     // Data is public because it is created by the
     // frontend. See Parser<FullParseHandler>::newFunctionScopeData.
     struct Data
     {
         // The canonical function of the scope, as during a scope walk we
         // often query properties of the JSFunction (e.g., is the function an
         // arrow).
-        GCPtrFunction canonicalFunction;
+        GCPtrFunction canonicalFunction = {};
 
         // If parameter expressions are present, parameters act like lexical
         // bindings.
-        bool hasParameterExprs;
+        bool hasParameterExprs = false;
 
         // Bindings are sorted by kind in both frames and environments.
         //
         // Positional formal parameter names are those that are not
         // destructured. They may be referred to by argument slots if
         // !script()->hasParameterExprs().
         //
         // An argument slot that needs to be skipped due to being destructured
         // or having defaults will have a nullptr name in the name array to
         // advance the argument slot.
         //
         // positional formals - [0, nonPositionalFormalStart)
         //      other formals - [nonPositionalParamStart, varStart)
         //               vars - [varStart, length)
-        uint16_t nonPositionalFormalStart;
-        uint16_t varStart;
-        uint32_t length;
+        uint16_t nonPositionalFormalStart = 0;
+        uint16_t varStart = 0;
+        uint32_t length = 0;
 
         // Frame slots [0, nextFrameSlot) are live when this is the innermost
         // scope.
-        uint32_t nextFrameSlot;
+        uint32_t nextFrameSlot = 0;
 
         // The array of tagged JSAtom* names, allocated beyond the end of the
         // struct.
         TrailingNamesArray trailingNames;
 
+        explicit Data(size_t nameCount) : trailingNames(nameCount) {}
+        Data() = delete;
+
         void trace(JSTracer* trc);
         Zone* zone() const;
     };
 
     static size_t sizeOfData(uint32_t length) {
         return sizeof(Data) + (length ? length - 1 : 0) * sizeof(BindingName);
     }
 
@@ -632,26 +649,29 @@ class VarScope : public Scope
     friend class Scope;
 
   public:
     // Data is public because it is created by the
     // frontend. See Parser<FullParseHandler>::newVarScopeData.
     struct Data
     {
         // All bindings are vars.
-        uint32_t length;
+        uint32_t length = 0;
 
         // Frame slots [firstFrameSlot(), nextFrameSlot) are live when this is
         // the innermost scope.
-        uint32_t nextFrameSlot;
+        uint32_t nextFrameSlot = 0;
 
         // The array of tagged JSAtom* names, allocated beyond the end of the
         // struct.
         TrailingNamesArray trailingNames;
 
+        explicit Data(size_t nameCount) : trailingNames(nameCount) {}
+        Data() = delete;
+
         void trace(JSTracer* trc);
     };
 
     static size_t sizeOfData(uint32_t length) {
         return sizeof(Data) + (length ? length - 1 : 0) * sizeof(BindingName);
     }
 
     static void getDataNamesAndLength(Data* data, BindingName** names, uint32_t* length) {
@@ -727,25 +747,28 @@ class GlobalScope : public Scope
     struct Data
     {
         // Bindings are sorted by kind.
         //
         // top-level funcs - [0, varStart)
         //            vars - [varStart, letStart)
         //            lets - [letStart, constStart)
         //          consts - [constStart, length)
-        uint32_t varStart;
-        uint32_t letStart;
-        uint32_t constStart;
-        uint32_t length;
+        uint32_t varStart = 0;
+        uint32_t letStart = 0;
+        uint32_t constStart = 0;
+        uint32_t length = 0;
 
         // The array of tagged JSAtom* names, allocated beyond the end of the
         // struct.
         TrailingNamesArray trailingNames;
 
+        explicit Data(size_t nameCount) : trailingNames(nameCount) {}
+        Data() = delete;
+
         void trace(JSTracer* trc);
     };
 
     static size_t sizeOfData(uint32_t length) {
         return sizeof(Data) + (length ? length - 1 : 0) * sizeof(BindingName);
     }
 
     static void getDataNamesAndLength(Data* data, BindingName** names, uint32_t* length) {
@@ -830,27 +853,30 @@ class EvalScope : public Scope
     {
         // All bindings in an eval script are 'var' bindings. The implicit
         // lexical scope around the eval is present regardless of strictness
         // and is its own LexicalScope. However, we need to track top-level
         // functions specially for redeclaration checks.
         //
         // top-level funcs - [0, varStart)
         //            vars - [varStart, length)
-        uint32_t varStart;
-        uint32_t length;
+        uint32_t varStart = 0;
+        uint32_t length = 0;
 
         // Frame slots [0, nextFrameSlot) are live when this is the innermost
         // scope.
-        uint32_t nextFrameSlot;
+        uint32_t nextFrameSlot = 0;
 
         // The array of tagged JSAtom* names, allocated beyond the end of the
         // struct.
         TrailingNamesArray trailingNames;
 
+        explicit Data(size_t nameCount) : trailingNames(nameCount) {}
+        Data() = delete;
+
         void trace(JSTracer* trc);
     };
 
     static size_t sizeOfData(uint32_t length) {
         return sizeof(Data) + (length ? length - 1 : 0) * sizeof(BindingName);
     }
 
     static void getDataNamesAndLength(Data* data, BindingName** names, uint32_t* length) {
@@ -926,37 +952,40 @@ class ModuleScope : public Scope
     static const ScopeKind classScopeKind_ = ScopeKind::Module;
 
   public:
     // Data is public because it is created by the frontend. See
     // Parser<FullParseHandler>::newModuleScopeData.
     struct Data
     {
         // The module of the scope.
-        GCPtr<ModuleObject*> module;
+        GCPtr<ModuleObject*> module = {};
 
         // Bindings are sorted by kind.
         //
         // imports - [0, varStart)
         //    vars - [varStart, letStart)
         //    lets - [letStart, constStart)
         //  consts - [constStart, length)
-        uint32_t varStart;
-        uint32_t letStart;
-        uint32_t constStart;
-        uint32_t length;
+        uint32_t varStart = 0;
+        uint32_t letStart = 0;
+        uint32_t constStart = 0;
+        uint32_t length = 0;
 
         // Frame slots [0, nextFrameSlot) are live when this is the innermost
         // scope.
-        uint32_t nextFrameSlot;
+        uint32_t nextFrameSlot = 0;
 
         // The array of tagged JSAtom* names, allocated beyond the end of the
         // struct.
         TrailingNamesArray trailingNames;
 
+        explicit Data(size_t nameCount) : trailingNames(nameCount) {}
+        Data() = delete;
+
         void trace(JSTracer* trc);
         Zone* zone() const;
     };
 
     static size_t sizeOfData(uint32_t length) {
         return sizeof(Data) + (length ? length - 1 : 0) * sizeof(BindingName);
     }
 
@@ -998,26 +1027,29 @@ class WasmInstanceScope : public Scope
 {
     friend class BindingIter;
     friend class Scope;
     static const ScopeKind classScopeKind_ = ScopeKind::WasmInstance;
 
   public:
     struct Data
     {
-        uint32_t memoriesStart;
-        uint32_t globalsStart;
-        uint32_t length;
-        uint32_t nextFrameSlot;
+        uint32_t memoriesStart = 0;
+        uint32_t globalsStart = 0;
+        uint32_t length = 0;
+        uint32_t nextFrameSlot = 0;
 
         // The wasm instance of the scope.
-        GCPtr<WasmInstanceObject*> instance;
+        GCPtr<WasmInstanceObject*> instance = {};
 
         TrailingNamesArray trailingNames;
 
+        explicit Data(size_t nameCount) : trailingNames(nameCount) {}
+        Data() = delete;
+
         void trace(JSTracer* trc);
     };
 
     static WasmInstanceScope* create(JSContext* cx, WasmInstanceObject* instance);
 
     static size_t sizeOfData(uint32_t length) {
         return sizeof(Data) + (length ? length - 1 : 0) * sizeof(BindingName);
     }
@@ -1058,22 +1090,25 @@ class WasmFunctionScope : public Scope
 {
     friend class BindingIter;
     friend class Scope;
     static const ScopeKind classScopeKind_ = ScopeKind::WasmFunction;
 
   public:
     struct Data
     {
-        uint32_t length;
-        uint32_t nextFrameSlot;
-        uint32_t funcIndex;
+        uint32_t length = 0;
+        uint32_t nextFrameSlot = 0;
+        uint32_t funcIndex = 0;
 
         TrailingNamesArray trailingNames;
 
+        explicit Data(size_t nameCount) : trailingNames(nameCount) {}
+        Data() = delete;
+
         void trace(JSTracer* trc);
     };
 
     static WasmFunctionScope* create(JSContext* cx, HandleScope enclosing, uint32_t funcIndex);
 
     static size_t sizeOfData(uint32_t length) {
         return sizeof(Data) + (length ? length - 1 : 0) * sizeof(BindingName);
     }