Bug 995278 - JS_NewArrayBufferContents frees user data on error. r=sfink
authorAnuj Agarwal <anujagarwal464@gmail.com>
Sun, 27 Apr 2014 01:22:00 -0400
changeset 180890 ebdd57580809e75dc1def2721253883c4ed63ed9
parent 180889 5aac645646304195a0edb048461303b7b82ac107
child 180891 eaa0d64e544e0dd8ba29ed2ec66e8ecdb5ab248e
push id272
push userpvanderbeken@mozilla.com
push dateMon, 05 May 2014 16:31:18 +0000
reviewerssfink
bugs995278
milestone31.0a1
Bug 995278 - JS_NewArrayBufferContents frees user data on error. r=sfink
content/base/src/nsXMLHttpRequest.cpp
js/src/jsapi-tests/testMappedArrayBuffer.cpp
js/src/jsapi.h
js/src/vm/ArrayBufferObject.cpp
js/src/vm/StructuredClone.cpp
--- a/content/base/src/nsXMLHttpRequest.cpp
+++ b/content/base/src/nsXMLHttpRequest.cpp
@@ -3925,23 +3925,22 @@ ArrayBufferBuilder::getArrayBuffer(JSCon
   // added
   if (mCapacity > mLength || mLength == 0) {
     if (!setCapacity(mLength)) {
       return nullptr;
     }
   }
 
   JSObject* obj = JS_NewArrayBufferWithContents(aCx, mLength, mDataPtr);
+  mDataPtr = nullptr;
+  mLength = mCapacity = 0;
   if (!obj) {
+    js_free(mDataPtr);
     return nullptr;
   }
-
-  mDataPtr = nullptr;
-  mLength = mCapacity = 0;
-
   return obj;
 }
 
 /* static */ bool
 ArrayBufferBuilder::areOverlappingRegions(const uint8_t* aStart1,
                                           uint32_t aLength1,
                                           const uint8_t* aStart2,
                                           uint32_t aLength2)
--- a/js/src/jsapi-tests/testMappedArrayBuffer.cpp
+++ b/js/src/jsapi-tests/testMappedArrayBuffer.cpp
@@ -60,17 +60,20 @@ BEGIN_TEST(testMappedArrayBuffer_bug9451
 JSObject *CreateNewObject(const int offset, const int length)
 {
     int fd = open(test_filename, O_RDONLY);
     void *ptr = JS_CreateMappedArrayBufferContents(fd, offset, length);
     close(fd);
     if (!ptr)
         return nullptr;
     JSObject *obj = JS_NewMappedArrayBufferWithContents(cx, length, ptr);
-
+    if (!obj) {
+        JS_ReleaseMappedArrayBufferContents(ptr, length);
+        return nullptr;
+    }
     return obj;
 }
 
 bool VerifyObject(JS::HandleObject obj, const int offset, const int length, const bool mapped)
 {
     CHECK(obj);
     CHECK(JS_IsArrayBufferObject(obj));
     CHECK_EQUAL(JS_GetArrayBufferByteLength(obj), length);
--- a/js/src/jsapi.h
+++ b/js/src/jsapi.h
@@ -3166,19 +3166,18 @@ JS_ClearNonGlobalObject(JSContext *cx, J
 /*
  * Assign 'undefined' to all of the object's non-reserved slots. Note: this is
  * done for all slots, regardless of the associated property descriptor.
  */
 JS_PUBLIC_API(void)
 JS_SetAllNonReservedSlotsToUndefined(JSContext *cx, JSObject *objArg);
 
 /*
- * Create a new array buffer with the given contents. The new array buffer
- * takes ownership: after calling this function, do not free |contents| or use
- * |contents| from another thread.
+ * Create a new array buffer with the given contents. On success, the ownership
+ * is transferred to the new array buffer.
  */
 extern JS_PUBLIC_API(JSObject *)
 JS_NewArrayBufferWithContents(JSContext *cx, size_t nbytes, void *contents);
 
 /*
  * Steal the contents of the given array buffer. The array buffer has its
  * length set to 0 and its contents array cleared. The caller takes ownership
  * of the return value and must free it or transfer ownership via
@@ -3200,17 +3199,18 @@ JS_AllocateArrayBufferContents(JSContext
  * Reallocate memory allocated by JS_AllocateArrayBufferContents, growing or
  * shrinking it as appropriate. If oldContents is null then this behaves like
  * JS_AllocateArrayBufferContents.
  */
 extern JS_PUBLIC_API(void *)
 JS_ReallocateArrayBufferContents(JSContext *cx, uint32_t nbytes, void *oldContents, uint32_t oldNbytes);
 
 /*
- * Create a new mapped array buffer with the given memory mapped contents.
+ * Create a new mapped array buffer with the given memory mapped contents. On success,
+ * the ownership is transferred to the new mapped array buffer.
  */
 extern JS_PUBLIC_API(JSObject *)
 JS_NewMappedArrayBufferWithContents(JSContext *cx, size_t nbytes, void *contents);
 
 /*
  * Create memory mapped array buffer contents.
  * Caller must take care of closing fd after calling this function.
  */
--- a/js/src/vm/ArrayBufferObject.cpp
+++ b/js/src/vm/ArrayBufferObject.cpp
@@ -646,21 +646,19 @@ ArrayBufferObject::create(JSContext *cx,
                 return nullptr;
         }
     }
 
     JS_ASSERT(!(class_.flags & JSCLASS_HAS_PRIVATE));
     gc::AllocKind allocKind = GetGCObjectKind(nslots);
 
     Rooted<ArrayBufferObject*> obj(cx, NewBuiltinClassInstance<ArrayBufferObject>(cx, allocKind, newKind));
-    if (!obj) {
-        if (data)
-            js_free(data);
+    if (!obj)
         return nullptr;
-    }
+
     JS_ASSERT(obj->getClass() == &class_);
 
     JS_ASSERT(!gc::IsInsideNursery(cx->runtime(), obj));
 
     if (data) {
         obj->initialize(nbytes, data, OwnsData);
         if (mapped)
             obj->setIsMappedArrayBuffer();
--- a/js/src/vm/StructuredClone.cpp
+++ b/js/src/vm/StructuredClone.cpp
@@ -1567,17 +1567,19 @@ JSStructuredCloneReader::readTransferMap
                 ReportErrorTransferable(cx, callbacks);
                 return false;
             }
             if (!callbacks->readTransfer(cx, this, tag, content, extraData, closure, &obj))
                 return false;
             MOZ_ASSERT(obj);
             MOZ_ASSERT(!cx->isExceptionPending());
         }
-
+        
+        // On failure, the buffer will still own the data (since its ownership will not get set to SCTAG_TMO_UNOWNED),
+        // so the data will be freed by ClearStructuredClone
         if (!obj)
             return false;
 
         // Mark the SCTAG_TRANSFER_MAP_* entry as no longer owned by the input
         // buffer.
         *pos = PairToUInt64(tag, JS::SCTAG_TMO_UNOWNED);
         MOZ_ASSERT(headerPos < pos && pos < in.end());