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 418609 2ddbcf79bc48
parent 418608 73c36389c5f7
child 418610 19d2aace5b3c
push id103351
push userjwalden@mit.edu
push dateThu, 17 May 2018 07:17:38 +0000
treeherdermozilla-inbound@e016aa76775e [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjandem
bugs1461821
milestone62.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
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);
     }