Bug 1178793 - Throw on OOB atomics access, interpreter+Ion. r=luke
authorLars T Hansen <lhansen@mozilla.com>
Wed, 02 Sep 2015 09:25:12 +0200
changeset 260548 51f25ea57b7445fdc633fde8e4ef8ac391377327
parent 260547 0dcb70eb5798c80fae1ddec7577eb1308acb7a72
child 260549 3e0f851d44435338e2e62211344af9a615c7899e
push id29314
push userryanvm@gmail.com
push dateWed, 02 Sep 2015 18:53:49 +0000
treeherdermozilla-central@7ca8e465cbd2 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersluke
bugs1178793
milestone43.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 1178793 - Throw on OOB atomics access, interpreter+Ion. r=luke
js/src/builtin/AtomicsObject.cpp
js/src/builtin/AtomicsObject.h
js/src/jit-test/tests/atomics/basic-tests.js
js/src/js.msg
js/src/vm/Xdr.h
--- a/js/src/builtin/AtomicsObject.cpp
+++ b/js/src/builtin/AtomicsObject.cpp
@@ -74,65 +74,55 @@ const Class AtomicsObject::class_ = {
 static bool
 ReportBadArrayType(JSContext* cx)
 {
     JS_ReportErrorNumber(cx, GetErrorMessage, nullptr, JSMSG_ATOMICS_BAD_ARRAY);
     return false;
 }
 
 static bool
+ReportOutOfRange(JSContext* cx)
+{
+    JS_ReportErrorNumber(cx, GetErrorMessage, nullptr, JSMSG_ATOMICS_BAD_INDEX);
+    return false;
+}
+
+static bool
 GetSharedTypedArray(JSContext* cx, HandleValue v,
                     MutableHandle<SharedTypedArrayObject*> viewp)
 {
     if (!v.isObject())
         return ReportBadArrayType(cx);
     if (!v.toObject().is<SharedTypedArrayObject>())
         return ReportBadArrayType(cx);
     viewp.set(&v.toObject().as<SharedTypedArrayObject>());
     return true;
 }
 
-// Returns true so long as the conversion succeeds, and then *inRange
-// is set to false if the index is not in range.
 static bool
 GetSharedTypedArrayIndex(JSContext* cx, HandleValue v, Handle<SharedTypedArrayObject*> view,
-                         uint32_t* offset, bool* inRange)
+                         uint32_t* offset)
 {
     RootedId id(cx);
     if (!ValueToId<CanGC>(cx, v, &id))
         return false;
     uint64_t index;
-    if (!IsTypedArrayIndex(id, &index) || index >= view->length()) {
-        *inRange = false;
-    } else {
-        *offset = (uint32_t)index;
-        *inRange = true;
-    }
-    return true;
-}
-
-void
-js::atomics_fullMemoryBarrier()
-{
-    jit::AtomicOperations::fenceSeqCst();
-}
-
-static bool
-AtomicsFence(JSContext* cx, MutableHandleValue r)
-{
-    atomics_fullMemoryBarrier();
-    r.setUndefined();
+    if (!IsTypedArrayIndex(id, &index) || index >= view->length())
+        return ReportOutOfRange(cx);
+    *offset = (uint32_t)index;
     return true;
 }
 
 bool
 js::atomics_fence(JSContext* cx, unsigned argc, Value* vp)
 {
     CallArgs args = CallArgsFromVp(argc, vp);
-    return AtomicsFence(cx, args.rval());
+    jit::AtomicOperations::fenceSeqCst();
+    args.rval().setUndefined();
+    return true;
 }
 
 static int32_t
 CompareExchange(Scalar::Type viewType, int32_t oldCandidate, int32_t newCandidate, void* viewData,
                 uint32_t offset, bool* badArrayType=nullptr)
 {
     switch (viewType) {
       case Scalar::Int8: {
@@ -193,29 +183,25 @@ js::atomics_compareExchange(JSContext* c
     HandleValue oldv = args.get(2);
     HandleValue newv = args.get(3);
     MutableHandleValue r = args.rval();
 
     Rooted<SharedTypedArrayObject*> view(cx, nullptr);
     if (!GetSharedTypedArray(cx, objv, &view))
         return false;
     uint32_t offset;
-    bool inRange;
-    if (!GetSharedTypedArrayIndex(cx, idxv, view, &offset, &inRange))
+    if (!GetSharedTypedArrayIndex(cx, idxv, view, &offset))
         return false;
     int32_t oldCandidate;
     if (!ToInt32(cx, oldv, &oldCandidate))
         return false;
     int32_t newCandidate;
     if (!ToInt32(cx, newv, &newCandidate))
         return false;
 
-    if (!inRange)
-        return AtomicsFence(cx, r);
-
     bool badType = false;
     int32_t result = CompareExchange(view->type(), oldCandidate, newCandidate, view->viewData(), offset, &badType);
 
     if (badType)
         return ReportBadArrayType(cx);
 
     if (view->type() == Scalar::Uint32)
         r.setNumber((double)(uint32_t)result);
@@ -231,23 +217,19 @@ js::atomics_load(JSContext* cx, unsigned
     HandleValue objv = args.get(0);
     HandleValue idxv = args.get(1);
     MutableHandleValue r = args.rval();
 
     Rooted<SharedTypedArrayObject*> view(cx, nullptr);
     if (!GetSharedTypedArray(cx, objv, &view))
         return false;
     uint32_t offset;
-    bool inRange;
-    if (!GetSharedTypedArrayIndex(cx, idxv, view, &offset, &inRange))
+    if (!GetSharedTypedArrayIndex(cx, idxv, view, &offset))
         return false;
 
-    if (!inRange)
-        return AtomicsFence(cx, r);
-
     switch (view->type()) {
       case Scalar::Uint8:
       case Scalar::Uint8Clamped: {
         uint8_t v = jit::AtomicOperations::loadSeqCst((uint8_t*)view->viewData() + offset);
         r.setInt32(v);
         return true;
       }
       case Scalar::Int8: {
@@ -351,29 +333,22 @@ ExchangeOrStore(JSContext* cx, unsigned 
     HandleValue idxv = args.get(1);
     HandleValue valv = args.get(2);
     MutableHandleValue r = args.rval();
 
     Rooted<SharedTypedArrayObject*> view(cx, nullptr);
     if (!GetSharedTypedArray(cx, objv, &view))
         return false;
     uint32_t offset;
-    bool inRange;
-    if (!GetSharedTypedArrayIndex(cx, idxv, view, &offset, &inRange))
+    if (!GetSharedTypedArrayIndex(cx, idxv, view, &offset))
         return false;
     int32_t numberValue;
     if (!ToInt32(cx, valv, &numberValue))
         return false;
 
-    if (!inRange) {
-        atomics_fullMemoryBarrier();
-        r.set(valv);
-        return true;
-    }
-
     bool badType = false;
     int32_t result = ExchangeOrStore<op>(view->type(), numberValue, view->viewData(), offset, &badType);
 
     if (badType)
         return ReportBadArrayType(cx);
 
     if (view->type() == Scalar::Uint32)
         r.setNumber((double)(uint32_t)result);
@@ -398,26 +373,22 @@ template<typename T>
 static bool
 AtomicsBinop(JSContext* cx, HandleValue objv, HandleValue idxv, HandleValue valv,
              MutableHandleValue r)
 {
     Rooted<SharedTypedArrayObject*> view(cx, nullptr);
     if (!GetSharedTypedArray(cx, objv, &view))
         return false;
     uint32_t offset;
-    bool inRange;
-    if (!GetSharedTypedArrayIndex(cx, idxv, view, &offset, &inRange))
+    if (!GetSharedTypedArrayIndex(cx, idxv, view, &offset))
         return false;
     int32_t numberValue;
     if (!ToInt32(cx, valv, &numberValue))
         return false;
 
-    if (!inRange)
-        return AtomicsFence(cx, r);
-
     switch (view->type()) {
       case Scalar::Int8: {
         int8_t v = (int8_t)numberValue;
         r.setInt32(T::operate((int8_t*)view->viewData() + offset, v));
         return true;
       }
       case Scalar::Uint8: {
         uint8_t v = (uint8_t)numberValue;
@@ -799,40 +770,33 @@ js::atomics_futexWait(JSContext* cx, uns
     JSRuntime* rt = cx->runtime();
 
     Rooted<SharedTypedArrayObject*> view(cx, nullptr);
     if (!GetSharedTypedArray(cx, objv, &view))
         return false;
     if (view->type() != Scalar::Int32)
         return ReportBadArrayType(cx);
     uint32_t offset;
-    bool inRange;
-    if (!GetSharedTypedArrayIndex(cx, idxv, view, &offset, &inRange))
+    if (!GetSharedTypedArrayIndex(cx, idxv, view, &offset))
         return false;
     int32_t value;
     if (!ToInt32(cx, valv, &value))
         return false;
     double timeout_ms;
     if (timeoutv.isUndefined()) {
         timeout_ms = mozilla::PositiveInfinity<double>();
     } else {
         if (!ToNumber(cx, timeoutv, &timeout_ms))
             return false;
         if (mozilla::IsNaN(timeout_ms))
             timeout_ms = mozilla::PositiveInfinity<double>();
         else if (timeout_ms < 0)
             timeout_ms = 0;
     }
 
-    if (!inRange) {
-        atomics_fullMemoryBarrier();
-        r.setUndefined();
-        return true;
-    }
-
     // This lock also protects the "waiters" field on SharedArrayRawBuffer,
     // and it provides the necessary memory fence.
     AutoLockFutexAPI lock;
 
     int32_t* addr = (int32_t*)view->viewData() + offset;
     if (*addr != value) {
         r.setInt32(AtomicsObject::FutexNotequal);
         return true;
@@ -878,24 +842,18 @@ js::atomics_futexWake(JSContext* cx, uns
     MutableHandleValue r = args.rval();
 
     Rooted<SharedTypedArrayObject*> view(cx, nullptr);
     if (!GetSharedTypedArray(cx, objv, &view))
         return false;
     if (view->type() != Scalar::Int32)
         return ReportBadArrayType(cx);
     uint32_t offset;
-    bool inRange;
-    if (!GetSharedTypedArrayIndex(cx, idxv, view, &offset, &inRange))
+    if (!GetSharedTypedArrayIndex(cx, idxv, view, &offset))
         return false;
-    if (!inRange) {
-        atomics_fullMemoryBarrier();
-        r.setUndefined();
-        return true;
-    }
     double count;
     if (!ToInteger(cx, countv, &count))
         return false;
     if (count < 0)
         count = 0;
 
     AutoLockFutexAPI lock;
 
@@ -933,36 +891,29 @@ js::atomics_futexWakeOrRequeue(JSContext
     MutableHandleValue r = args.rval();
 
     Rooted<SharedTypedArrayObject*> view(cx, nullptr);
     if (!GetSharedTypedArray(cx, objv, &view))
         return false;
     if (view->type() != Scalar::Int32)
         return ReportBadArrayType(cx);
     uint32_t offset1;
-    bool inRange1;
-    if (!GetSharedTypedArrayIndex(cx, idx1v, view, &offset1, &inRange1))
+    if (!GetSharedTypedArrayIndex(cx, idx1v, view, &offset1))
         return false;
     double count;
     if (!ToInteger(cx, countv, &count))
         return false;
     if (count < 0)
         count = 0;
     int32_t value;
     if (!ToInt32(cx, valv, &value))
         return false;
     uint32_t offset2;
-    bool inRange2;
-    if (!GetSharedTypedArrayIndex(cx, idx2v, view, &offset2, &inRange2))
+    if (!GetSharedTypedArrayIndex(cx, idx2v, view, &offset2))
         return false;
-    if (!(inRange1 && inRange2)) {
-        atomics_fullMemoryBarrier();
-        r.setUndefined();
-        return true;
-    }
 
     AutoLockFutexAPI lock;
 
     int32_t* addr = (int32_t*)view->viewData() + offset1;
     if (*addr != value) {
         r.setInt32(AtomicsObject::FutexNotequal);
         return true;
     }
--- a/js/src/builtin/AtomicsObject.h
+++ b/js/src/builtin/AtomicsObject.h
@@ -24,18 +24,16 @@ class AtomicsObject : public JSObject
     // return a value that is either the number of tasks woken or an error code.
     enum FutexWaitResult : int32_t {
         FutexOK = 0,
         FutexNotequal = -1,
         FutexTimedout = -2
     };
 };
 
-void atomics_fullMemoryBarrier();
-
 bool atomics_compareExchange(JSContext* cx, unsigned argc, Value* vp);
 bool atomics_exchange(JSContext* cx, unsigned argc, Value* vp);
 bool atomics_load(JSContext* cx, unsigned argc, Value* vp);
 bool atomics_store(JSContext* cx, unsigned argc, Value* vp);
 bool atomics_fence(JSContext* cx, unsigned argc, Value* vp);
 bool atomics_add(JSContext* cx, unsigned argc, Value* vp);
 bool atomics_sub(JSContext* cx, unsigned argc, Value* vp);
 bool atomics_and(JSContext* cx, unsigned argc, Value* vp);
--- a/js/src/jit-test/tests/atomics/basic-tests.js
+++ b/js/src/jit-test/tests/atomics/basic-tests.js
@@ -1,15 +1,17 @@
 // Basic functional tests for the Atomics primitives.
 //
 // These do not test atomicity, just that calling and coercions and
 // indexing and exception behavior all work right.
 //
 // These do not test the futex operations.
 
+load(libdir + "asserts.js");
+
 var DEBUG = false;		// Set to true for useful printouts
 
 function dprint(...xs) {
     if (!DEBUG)
 	return;
     var s = "";
     for ( var x in xs )
 	s += String(xs[x]);
@@ -199,28 +201,33 @@ function testTypeBinop(a, op) {
     assertEq(thrown, true);
 
     // These are all OK
     op(a, 0, 0.7);
     op(a, 0, "0");
     op(a, 0);
 }
 
+var globlength = 0;		// Will be set later
+
 function testRangeCAS(a) {
     dprint("Range: " + a.constructor.name);
 
-    assertEq(Atomics.compareExchange(a, -1, 0, 1), undefined); // out of range => undefined, no effect
+    var msg = /out-of-range index for atomic access/;
+
+    assertErrorMessage(() => Atomics.compareExchange(a, -1, 0, 1), RangeError, msg);
     assertEq(a[0], 0);
-    a[0] = 0;
 
-    assertEq(Atomics.compareExchange(a, "hi", 0, 1), undefined); // invalid => undefined, no effect
+    assertErrorMessage(() => Atomics.compareExchange(a, "hi", 0, 1), RangeError, msg);
     assertEq(a[0], 0);
-    a[0] = 0;
 
-    assertEq(Atomics.compareExchange(a, a.length + 5, 0, 1), undefined); // out of range => undefined, no effect
+    assertErrorMessage(() => Atomics.compareExchange(a, a.length + 5, 0, 1), RangeError, msg);
+    assertEq(a[0], 0);
+
+    assertErrorMessage(() => Atomics.compareExchange(a, globlength, 0, 1), RangeError, msg);
     assertEq(a[0], 0);
 }
 
 // Ad-hoc tests for extreme and out-of-range values.
 // None of these should throw
 
 function testInt8Extremes(a) {
     dprint("Int8 extremes");
@@ -474,17 +481,19 @@ function runTests() {
 
     CLONE(testTypeBinop)(v32, Atomics.add);
     CLONE(testTypeBinop)(v32, Atomics.sub);
     CLONE(testTypeBinop)(v32, Atomics.and);
     CLONE(testTypeBinop)(v32, Atomics.or);
     CLONE(testTypeBinop)(v32, Atomics.xor);
 
     // Test out-of-range references
+    globlength = v8.length + 5;
     CLONE(testRangeCAS)(v8);
+    globlength = v32.length + 5;
     CLONE(testRangeCAS)(v32);
 
     // Test extreme values
     testInt8Extremes(new SharedInt8Array(sab));
     testUint8Extremes(new SharedUint8Array(sab));
     testInt16Extremes(new SharedInt16Array(sab));
     testUint32(new SharedUint32Array(sab));
 
--- a/js/src/js.msg
+++ b/js/src/js.msg
@@ -485,16 +485,17 @@ MSG_DEF(JSMSG_BAD_PARSE_NODE,          0
 MSG_DEF(JSMSG_BAD_SYMBOL,              1, JSEXN_TYPEERR, "{0} is not a well-known @@-symbol")
 MSG_DEF(JSMSG_SYMBOL_TO_STRING,        0, JSEXN_TYPEERR, "can't convert symbol to string")
 MSG_DEF(JSMSG_SYMBOL_TO_NUMBER,        0, JSEXN_TYPEERR, "can't convert symbol to number")
 
 // Atomics and futexes
 MSG_DEF(JSMSG_ATOMICS_BAD_ARRAY,         0, JSEXN_TYPEERR, "invalid array type for the operation")
 MSG_DEF(JSMSG_ATOMICS_TOO_LONG,          0, JSEXN_RANGEERR, "timeout value too large")
 MSG_DEF(JSMSG_ATOMICS_WAIT_NOT_ALLOWED,  0, JSEXN_ERR, "waiting is not allowed on this thread")
+MSG_DEF(JSMSG_ATOMICS_BAD_INDEX,         0, JSEXN_RANGEERR, "out-of-range index for atomic access")
 
 // XPConnect wrappers and DOM bindings
 MSG_DEF(JSMSG_CANT_SET_INTERPOSED,       1, JSEXN_TYPEERR, "unable to set interposed data property '{0}'")
 MSG_DEF(JSMSG_CANT_DEFINE_WINDOW_ELEMENT, 0, JSEXN_TYPEERR, "can't define elements on a Window object")
 MSG_DEF(JSMSG_CANT_DELETE_WINDOW_ELEMENT, 0, JSEXN_TYPEERR, "can't delete elements from a Window object")
 MSG_DEF(JSMSG_CANT_DELETE_WINDOW_NAMED_PROPERTY, 1, JSEXN_TYPEERR, "can't delete property {0} from window's named properties object")
 MSG_DEF(JSMSG_CANT_PREVENT_EXTENSIONS,   0, JSEXN_TYPEERR, "can't prevent extensions on this proxy object")
 MSG_DEF(JSMSG_NO_NAMED_SETTER,           2, JSEXN_TYPEERR, "{0} doesn't have a named property setter for '{1}'")
--- a/js/src/vm/Xdr.h
+++ b/js/src/vm/Xdr.h
@@ -24,21 +24,21 @@ namespace js {
  * versions.  If deserialization fails, the data should be invalidated if
  * possible.
  *
  * When you change this, run make_opcode_doc.py and copy the new output into
  * this wiki page:
  *
  *  https://developer.mozilla.org/en-US/docs/SpiderMonkey/Internals/Bytecode
  */
-static const uint32_t XDR_BYTECODE_VERSION_SUBTRAHEND = 303;
+static const uint32_t XDR_BYTECODE_VERSION_SUBTRAHEND = 304;
 static const uint32_t XDR_BYTECODE_VERSION =
     uint32_t(0xb973c0de - XDR_BYTECODE_VERSION_SUBTRAHEND);
 
-static_assert(JSErr_Limit == 405,
+static_assert(JSErr_Limit == 406,
               "GREETINGS, POTENTIAL SUBTRAHEND INCREMENTER! If you added or "
               "removed MSG_DEFs from js.msg, you should increment "
               "XDR_BYTECODE_VERSION_SUBTRAHEND and update this assertion's "
               "expected JSErr_Limit value.");
 
 class XDRBuffer {
   public:
     explicit XDRBuffer(JSContext* cx)