Bug 982974 - Be paranoid about neutering ArrayBuffer objects. r=sfink, r=jorendorff
authorJeff Walden <jwalden@mit.edu>
Fri, 14 Mar 2014 16:55:48 -0700
changeset 191942 355266055005c093a8fdb53955408a69ad1221c1
parent 191941 fa8369f3ad5799506ee984b86d731544d7145965
child 191943 5e86e89e463f3ba23c72a16ea99b57ab273fa6b9
push id474
push userasasaki@mozilla.com
push dateMon, 02 Jun 2014 21:01:02 +0000
treeherdermozilla-release@967f4cf1b31c [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerssfink, jorendorff
bugs982974
milestone30.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 982974 - Be paranoid about neutering ArrayBuffer objects. r=sfink, r=jorendorff
js/src/vm/ArrayBufferObject.cpp
js/src/vm/ArrayBufferObject.h
--- a/js/src/vm/ArrayBufferObject.cpp
+++ b/js/src/vm/ArrayBufferObject.cpp
@@ -468,33 +468,38 @@ ArrayBufferObject::changeContents(JSCont
         rt->gcNursery.notifyNewElements(this, newHeader);
 #endif
 
     initElementsHeader(newHeader, byteLengthCopy);
     InitViewList(this, viewListHead);
 }
 
 void
-ArrayBufferObject::neuter(JSContext *cx)
+ArrayBufferObject::neuter(ObjectElements *newHeader, JSContext *cx)
 {
-    JS_ASSERT(!isSharedArrayBuffer());
+    MOZ_ASSERT(!isSharedArrayBuffer());
 
-    JS_ASSERT(cx);
-    if (hasDynamicElements() && !isAsmJSArrayBuffer()) {
+    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);
+    updateElementsHeader(newHeader, byteLen);
 
-    getElementsHeader()->setIsNeuteredBuffer();
+    newHeader->setIsNeuteredBuffer();
 }
 
 /* static */ bool
 ArrayBufferObject::ensureNonInline(JSContext *cx, Handle<ArrayBufferObject*> buffer)
 {
     JS_ASSERT(!buffer->isSharedArrayBuffer());
     if (buffer->hasDynamicElements())
         return true;
@@ -751,54 +756,58 @@ 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);
+        if (!newHeader)
+            return false;
     } else {
-        stolen = false;
-
-        uint32_t byteLen = buffer->byteLength();
         transferableHeader = AllocateArrayBufferContents(cx, byteLen);
         if (!transferableHeader)
             return false;
 
         initElementsHeader(transferableHeader, byteLen);
         void *headerDataPointer = reinterpret_cast<void*>(transferableHeader->elements());
         memcpy(headerDataPointer, buffer->dataPointer(), 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,
@@ -1265,19 +1274,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());
+        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/ArrayBufferObject.h
+++ b/js/src/vm/ArrayBufferObject.h
@@ -127,16 +127,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'
@@ -171,20 +189,26 @@ class ArrayBufferObject : public JSObjec
     /*
      * Neuter all views of an ArrayBuffer.
      */
     static bool neuterViews(JSContext *cx, Handle<ArrayBufferObject*> buffer);
 
     uint8_t * dataPointer() const;
 
     /*
-     * 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 *cx);
+    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_;
     }