Bug 982974 - Be paranoid about neutering ArrayBuffer objects. r=sfink, r=jorendorff, a=dveditz
authorJeff Walden <jwalden@mit.edu>
Fri, 14 Mar 2014 17:20:22 -0700
changeset 176478 30a46e893204b5c2c2de29cd62950b7192f077cd
parent 176477 160108f82b6269a9e637599cebdb0e3449254571
child 176479 12cbf785d76923bf4917642e9c5220b85f99d050
child 184423 0612e4747b5bb95c4ddd5d89dd8824404fceaf8d
push id449
push userryanvm@gmail.com
push dateSat, 15 Mar 2014 03:34:11 +0000
treeherdermozilla-release@12cbf785d769 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerssfink, jorendorff, dveditz
bugs982974
milestone28.0
Bug 982974 - Be paranoid about neutering ArrayBuffer objects. r=sfink, r=jorendorff, a=dveditz
js/src/vm/TypedArrayObject.cpp
js/src/vm/TypedArrayObject.h
--- a/js/src/vm/TypedArrayObject.cpp
+++ b/js/src/vm/TypedArrayObject.cpp
@@ -400,31 +400,38 @@ ArrayBufferObject::changeContents(JSCont
 
     elements = newHeader->elements();
 
     initElementsHeader(newHeader, byteLengthCopy);
     InitViewList(this, viewListHead);
 }
 
 void
-ArrayBufferObject::neuter(JSContext *cx)
+ArrayBufferObject::neuter(ObjectElements *newHeader, JSContext *cx)
 {
-    JS_ASSERT(cx);
-    if (hasDynamicElements() && !isAsmJSArrayBuffer()) {
+    MOZ_ASSERT(cx);
+
+    if (hasStealableContents()) {
+        MOZ_ASSERT(newHeader);
+
         ObjectElements *oldHeader = getElementsHeader();
-        changeContents(cx, ObjectElements::fromElements(fixedElements()));
+        MOZ_ASSERT(newHeader != oldHeader);
+
+        changeContents(cx, newHeader);
 
         FreeOp fop(cx->runtime(), false);
         fop.free_(oldHeader);
+    } else {
+        elements = newHeader->elements();
     }
 
     uint32_t byteLen = 0;
-    updateElementsHeader(getElementsHeader(), byteLen);
-
-    getElementsHeader()->setIsNeuteredBuffer();
+    updateElementsHeader(newHeader, byteLen);
+
+    newHeader->setIsNeuteredBuffer();
 }
 
 bool
 ArrayBufferObject::copyData(JSContext *maybecx)
 {
     ObjectElements *newHeader = AllocateArrayBufferContents(maybecx, byteLength(), dataPointer());
     if (!newHeader)
         return false;
@@ -667,52 +674,56 @@ ArrayBufferObject::createDataViewForThis
     CallArgs args = CallArgsFromVp(argc, vp);
     return CallNonGenericMethod<IsArrayBuffer, createDataViewForThisImpl>(cx, args);
 }
 
 /* static */ bool
 ArrayBufferObject::stealContents(JSContext *cx, Handle<ArrayBufferObject*> buffer, void **contents,
                                  uint8_t **data)
 {
-    // If the ArrayBuffer's elements are dynamically allocated and nothing else
-    // prevents us from stealing them, transfer ownership directly.  Otherwise,
-    // the elements are small and allocated inside the ArrayBuffer object's GC
-    // header so we must make a copy.
+    uint32_t byteLen = buffer->byteLength();
+
+    // If the ArrayBuffer's elements are transferrable, transfer ownership
+    // directly.  Otherwise we have to copy the data into new elements.
     ObjectElements *transferableHeader;
-    bool stolen;
-    if (buffer->hasDynamicElements() && !buffer->isAsmJSArrayBuffer()) {
-        stolen = true;
+    ObjectElements *newHeader;
+    bool stolen = buffer->hasStealableContents();
+    if (stolen) {
         transferableHeader = buffer->getElementsHeader();
+
+        newHeader = AllocateArrayBufferContents(cx, byteLen, nullptr);
+        if (!newHeader)
+            return false;
     } else {
-        stolen = false;
-
-        uint32_t byteLen = buffer->byteLength();
         transferableHeader = AllocateArrayBufferContents(cx, byteLen, buffer->dataPointer());
         if (!transferableHeader)
             return false;
 
         initElementsHeader(transferableHeader, byteLen);
+
+        // Keep using the current elements.
+        newHeader = buffer->getElementsHeader();
     }
 
     JS_ASSERT(!IsInsideNursery(cx->runtime(), transferableHeader));
     *contents = transferableHeader;
     *data = reinterpret_cast<uint8_t *>(transferableHeader + 1);
 
     // Neuter the views, which may also mprotect(PROT_NONE) the buffer. So do
     // it after copying out the data.
     if (!ArrayBufferObject::neuterViews(cx, buffer))
         return false;
 
-    // If the elements were taken from the neutered buffer, revert it back to
-    // using inline storage so it doesn't attempt to free the stolen elements
-    // when finalized.
+    // If the elements were transferrable, revert the buffer back to using
+    // inline storage so it doesn't attempt to free the stolen elements when
+    // finalized.
     if (stolen)
         buffer->changeContents(cx, ObjectElements::fromElements(buffer->fixedElements()));
 
-    buffer->neuter(cx);
+    buffer->neuter(newHeader, cx);
     return true;
 }
 
 void
 ArrayBufferObject::obj_trace(JSTracer *trc, JSObject *obj)
 {
     /*
      * If this object changes, it will get marked via the private data barrier,
@@ -4056,19 +4067,43 @@ JS_FRIEND_API(bool)
 JS_NeuterArrayBuffer(JSContext *cx, HandleObject obj)
 {
     if (!obj->is<ArrayBufferObject>()) {
         JS_ReportError(cx, "ArrayBuffer object required");
         return false;
     }
 
     Rooted<ArrayBufferObject*> buffer(cx, &obj->as<ArrayBufferObject>());
-    if (!ArrayBufferObject::neuterViews(cx, buffer))
+
+    ObjectElements *newHeader;
+    if (buffer->hasStealableContents()) {
+        // If we're "disposing" with the buffer contents, allocate zeroed
+        // memory of equal size and swap that in as contents.  This ensures
+        // that stale indexes that assume the original length, won't index out
+        // of bounds.  This is a temporary hack: when we're confident we've
+        // eradicated all stale accesses, we'll stop doing this.
+        newHeader = AllocateArrayBufferContents(cx, buffer->byteLength(), nullptr);
+        if (!newHeader)
+            return false;
+    } else {
+        // This case neuters out the existing elements in-place, so use the
+        // old header as new.
+        newHeader = buffer->getElementsHeader();
+    }
+
+    // Mark all views of the ArrayBuffer as neutered.
+    if (!ArrayBufferObject::neuterViews(cx, buffer)) {
+        if (buffer->hasStealableContents()) {
+            FreeOp fop(cx->runtime(), false);
+            fop.free_(newHeader);
+        }
         return false;
-    buffer->neuter(cx);
+    }
+
+    buffer->neuter(newHeader, cx);
     return true;
 }
 
 JS_FRIEND_API(JSObject *)
 JS_NewArrayBuffer(JSContext *cx, uint32_t nbytes)
 {
     JS_ASSERT(nbytes <= INT32_MAX);
     return ArrayBufferObject::create(cx, nbytes);
--- a/js/src/vm/TypedArrayObject.h
+++ b/js/src/vm/TypedArrayObject.h
@@ -138,16 +138,34 @@ class ArrayBufferObject : public JSObjec
                               MutableHandleValue statep, MutableHandleId idp);
 
     static void sweep(JSCompartment *rt);
 
     static void resetArrayBufferList(JSCompartment *rt);
     static bool saveArrayBufferList(JSCompartment *c, ArrayBufferVector &vector);
     static void restoreArrayBufferLists(ArrayBufferVector &vector);
 
+    bool hasStealableContents() const {
+        // Inline elements strictly adhere to the corresponding buffer.
+        if (!hasDynamicElements())
+            return false;
+
+        // asm.js buffer contents are transferred by copying, just like inline
+        // elements.
+        if (isAsmJSArrayBuffer())
+            return false;
+
+        // Neutered contents aren't transferrable because we want a neutered
+        // array's contents to be backed by zeroed memory equal in length to
+        // the original buffer contents.  Transferring these contents would
+        // allocate new ones based on the current byteLength, which is 0 for a
+        // neutered array -- not the original byteLength.
+        return !isNeutered();
+    }
+
     static bool stealContents(JSContext *cx, Handle<ArrayBufferObject*> buffer, void **contents,
                               uint8_t **data);
 
     static void updateElementsHeader(js::ObjectElements *header, uint32_t bytes) {
         header->initializedLength = bytes;
 
         // NB: one or both of these fields is clobbered by GetViewList to store
         // the 'views' link. Set them to 0 to effectively initialize 'views'
@@ -191,21 +209,27 @@ class ArrayBufferObject : public JSObjec
      * Neuter all views of an ArrayBuffer.
      */
     static bool neuterViews(JSContext *cx, Handle<ArrayBufferObject*> buffer);
 
     inline uint8_t * dataPointer() const {
         return (uint8_t *) elements;
     }
 
-    /*
-     * Discard the ArrayBuffer contents. For asm.js buffers, at least, should
+     /*
+     * Discard the ArrayBuffer contents, and use |newHeader| for the buffer's
+     * new contents.  (These new contents are zeroed, of identical size in
+     * memory as the current contents, but appear to be neutered and of zero
+     * length.  This is purely precautionary against stale indexes that were
+     * in-bounds with respect to the initial length but would not be after
+     * neutering.  This precaution will be removed once we're sure such stale
+     * indexing no longer happens.)  For asm.js buffers, at least, should
      * be called after neuterViews().
      */
-    void neuter(JSContext *maybecx);
+    void neuter(ObjectElements *newHeader, JSContext *cx);
 
     /*
      * Check if the arrayBuffer contains any data. This will return false for
      * ArrayBuffer.prototype and neutered ArrayBuffers.
      */
     bool hasData() const {
         return getClass() == &class_;
     }