Bug 1503406 - Fix cross-compartment bug in ReadableStreamTee_Pull. r=tcampbell
authorJason Orendorff <jorendorff@mozilla.com>
Fri, 16 Nov 2018 13:30:47 +0000
changeset 503214 79c527fa7f0f2c2ed645f59a30f2bc105e2868d4
parent 503189 8c5eaa1d4356ff8a4cd295f43d89f82d375bcc1b
child 503215 e3e05897de25a3b5d4d8d41ac5c20463c62153e0
push id10290
push userffxbld-merge
push dateMon, 03 Dec 2018 16:23:23 +0000
treeherdermozilla-beta@700bed2445e6 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerstcampbell
bugs1503406
milestone65.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 1503406 - Fix cross-compartment bug in ReadableStreamTee_Pull. r=tcampbell Differential Revision: https://phabricator.services.mozilla.com/D11697
js/src/builtin/Stream.cpp
js/src/jit-test/tests/stream/bug-1503406.js
js/src/vm/Compartment-inl.h
--- a/js/src/builtin/Stream.cpp
+++ b/js/src/builtin/Stream.cpp
@@ -150,21 +150,25 @@ NewHandler(JSContext* cx, Native handler
                                                     GenericObject));
     if (!handlerFun) {
         return nullptr;
     }
     handlerFun->setExtendedSlot(0, ObjectValue(*target));
     return handlerFun;
 }
 
-template<class T>
+/**
+ * Helper for handler functions that "close over" a value that is always a
+ * direct reference to an object of class T, never a wrapper.
+ */
+template <class T>
 inline static MOZ_MUST_USE T*
-TargetFromHandler(JSObject& handler)
+TargetFromHandler(CallArgs& args)
 {
-    return &handler.as<JSFunction>().getExtendedSlot(0).toObject().as<T>();
+    return &args.callee().as<JSFunction>().getExtendedSlot(0).toObject().as<T>();
 }
 
 inline static MOZ_MUST_USE bool
 ResetQueue(JSContext* cx, Handle<ReadableStreamController*> unwrappedContainer);
 
 inline static MOZ_MUST_USE bool
 InvokeOrNoop(JSContext* cx, HandleValue O, HandlePropertyName P, HandleValue arg,
              MutableHandleValue rval);
@@ -920,17 +924,17 @@ static MOZ_MUST_USE bool
 ReadableStreamDefaultControllerEnqueue(JSContext* cx,
                                        Handle<ReadableStreamDefaultController*> unwrappedController,
                                        HandleValue chunk);
 
 static bool
 TeeReaderReadHandler(JSContext* cx, unsigned argc, Value* vp)
 {
     CallArgs args = CallArgsFromVp(argc, vp);
-    Rooted<TeeState*> teeState(cx, TargetFromHandler<TeeState>(args.callee()));
+    Rooted<TeeState*> unwrappedTeeState(cx, UnwrapCalleeSlot<TeeState>(cx, args, 0));
     HandleValue resultVal = args.get(0);
 
     // Step a: Assert: Type(result) is Object.
     RootedObject result(cx, &resultVal.toObject());
 
     // Step b: Let value be ? Get(result, "value").
     RootedValue value(cx);
     if (!GetPropertyPure(cx, result, NameToId(cx->names().value), value.address())) {
@@ -942,70 +946,70 @@ TeeReaderReadHandler(JSContext* cx, unsi
     if (!GetPropertyPure(cx, result, NameToId(cx->names().done), doneVal.address())) {
         return false;
     }
 
     // Step d: Assert: Type(done) is Boolean.
     bool done = doneVal.toBoolean();
 
     // Step e: If done is true and teeState.[[closedOrErrored]] is false,
-    if (done && !teeState->closedOrErrored()) {
+    if (done && !unwrappedTeeState->closedOrErrored()) {
         // Step i: If teeState.[[canceled1]] is false,
-        if (!teeState->canceled1()) {
+        if (!unwrappedTeeState->canceled1()) {
             // Step 1: Perform ! ReadableStreamDefaultControllerClose(branch1).
-            Rooted<ReadableStreamDefaultController*> branch1(cx, teeState->branch1());
+            Rooted<ReadableStreamDefaultController*> branch1(cx, unwrappedTeeState->branch1());
             if (!ReadableStreamDefaultControllerClose(cx, branch1)) {
                 return false;
             }
         }
 
         // Step ii: If teeState.[[canceled2]] is false,
-        if (!teeState->canceled2()) {
+        if (!unwrappedTeeState->canceled2()) {
             // Step 1: Perform ! ReadableStreamDefaultControllerClose(branch1).
-            Rooted<ReadableStreamDefaultController*> branch2(cx, teeState->branch2());
+            Rooted<ReadableStreamDefaultController*> branch2(cx, unwrappedTeeState->branch2());
             if (!ReadableStreamDefaultControllerClose(cx, branch2)) {
                 return false;
             }
         }
 
         // Step iii: Set teeState.[[closedOrErrored]] to true.
-        teeState->setClosedOrErrored();
+        unwrappedTeeState->setClosedOrErrored();
     }
 
     // Step f: If teeState.[[closedOrErrored]] is true, return.
-    if (teeState->closedOrErrored()) {
+    if (unwrappedTeeState->closedOrErrored()) {
         return true;
     }
 
     // Step g: Let value1 and value2 be value.
     RootedValue value1(cx, value);
     RootedValue value2(cx, value);
 
     // Step h: If teeState.[[canceled2]] is false and cloneForBranch2 is
     //         true, set value2 to
     //         ? StructuredDeserialize(StructuredSerialize(value2),
     //                                 the current Realm Record).
     // TODO: add StructuredClone() intrinsic.
-    MOZ_ASSERT(!teeState->cloneForBranch2(), "tee(cloneForBranch2=true) should not be exposed");
+    MOZ_ASSERT(!unwrappedTeeState->cloneForBranch2(), "tee(cloneForBranch2=true) should not be exposed");
 
     // Step i: If teeState.[[canceled1]] is false, perform
     //         ? ReadableStreamDefaultControllerEnqueue(branch1, value1).
-    Rooted<ReadableStreamDefaultController*> controller(cx);
-    if (!teeState->canceled1()) {
-        controller = teeState->branch1();
-        if (!ReadableStreamDefaultControllerEnqueue(cx, controller, value1)) {
+    Rooted<ReadableStreamDefaultController*> unwrappedController(cx);
+    if (!unwrappedTeeState->canceled1()) {
+        unwrappedController = unwrappedTeeState->branch1();
+        if (!ReadableStreamDefaultControllerEnqueue(cx, unwrappedController, value1)) {
             return false;
         }
     }
 
     // Step j: If teeState.[[canceled2]] is false,
     //         perform ? ReadableStreamDefaultControllerEnqueue(branch2, value2).
-    if (!teeState->canceled2()) {
-        controller = teeState->branch2();
-        if (!ReadableStreamDefaultControllerEnqueue(cx, controller, value2)) {
+    if (!unwrappedTeeState->canceled2()) {
+        unwrappedController = unwrappedTeeState->branch2();
+        if (!ReadableStreamDefaultControllerEnqueue(cx, unwrappedController, value2)) {
             return false;
         }
     }
 
     args.rval().setUndefined();
     return true;
 }
 
@@ -1036,17 +1040,21 @@ ReadableStreamTee_Pull(JSContext* cx, Ha
     Rooted<ReadableStreamDefaultReader*> unwrappedReader(cx,
         &unwrappedReaderObj->as<ReadableStreamDefaultReader>());
 
     RootedObject readPromise(cx, ::ReadableStreamDefaultReaderRead(cx, unwrappedReader));
     if (!readPromise) {
         return nullptr;
     }
 
-    RootedObject onFulfilled(cx, NewHandler(cx, TeeReaderReadHandler, unwrappedTeeState));
+    RootedObject teeState(cx, unwrappedTeeState);
+    if (!cx->compartment()->wrap(cx, &teeState)) {
+        return nullptr;
+    }
+    RootedObject onFulfilled(cx, NewHandler(cx, TeeReaderReadHandler, teeState));
     if (!onFulfilled) {
         return nullptr;
     }
 
     return JS::CallOriginalPromiseThen(cx, readPromise, onFulfilled, nullptr);
 }
 
 /**
@@ -1147,17 +1155,17 @@ ReadableStreamDefaultControllerErrorIfNe
 /**
  * Streams spec, 3.3.9. step 18:
  * Upon rejection of reader.[[closedPromise]] with reason r,
  */
 static bool
 TeeReaderClosedHandler(JSContext* cx, unsigned argc, Value* vp)
 {
     CallArgs args = CallArgsFromVp(argc, vp);
-    Rooted<TeeState*> teeState(cx, TargetFromHandler<TeeState>(args.callee()));
+    Rooted<TeeState*> teeState(cx, TargetFromHandler<TeeState>(args));
     HandleValue reason = args.get(0);
 
     // Step a: If teeState.[[closedOrErrored]] is false, then:
     if (!teeState->closedOrErrored()) {
         // Step a.iii: Set teeState.[[closedOrErrored]] to true.
         // Reordered to ensure that internal errors in the other steps don't
         // leave the teeState in an undefined state.
         teeState->setClosedOrErrored();
@@ -2133,18 +2141,18 @@ ReadableStreamControllerCallPullIfNeeded
  * Streams spec, 3.8.3, step 11.a.
  * and
  * Streams spec, 3.10.3, step 16.a.
  */
 static bool
 ControllerStartHandler(JSContext* cx, unsigned argc, Value* vp)
 {
     CallArgs args = CallArgsFromVp(argc, vp);
-    Rooted<ReadableStreamController*> controller(cx);
-    controller = TargetFromHandler<ReadableStreamController>(args.callee());
+    Rooted<ReadableStreamController*> controller(cx,
+        TargetFromHandler<ReadableStreamController>(args));
 
     // Step i: Set controller.[[started]] to true.
     controller->setStarted();
 
     // Step ii: Assert: controller.[[pulling]] is false.
     MOZ_ASSERT(!controller->pulling());
 
     // Step iii: Assert: controller.[[pullAgain]] is false.
@@ -2170,17 +2178,17 @@ ReadableStreamControllerError(JSContext*
  * and
  * Streams spec, 3.10.3, step 16.b.
  */
 static bool
 ControllerStartFailedHandler(JSContext* cx, unsigned argc, Value* vp)
 {
     CallArgs args = CallArgsFromVp(argc, vp);
     Rooted<ReadableStreamController*> controller(cx,
-        TargetFromHandler<ReadableStreamController>(args.callee()));
+        TargetFromHandler<ReadableStreamController>(args));
 
     // 3.8.3, Step 11.b.i:
     // Perform ! ReadableStreamDefaultControllerErrorIfNeeded(controller, r).
     if (controller->is<ReadableStreamDefaultController>()) {
         Rooted<ReadableStreamDefaultController*> defaultController(cx,
             &controller->as<ReadableStreamDefaultController>());
         return ReadableStreamDefaultControllerErrorIfNeeded(cx, defaultController, args.get(0));
     }
@@ -2682,19 +2690,18 @@ ReadableStreamDefaultControllerPullSteps
  * Streams spec, 3.9.2 and 3.12.3. step 7:
  * Upon fulfillment of pullPromise,
  */
 static bool
 ControllerPullHandler(JSContext* cx, unsigned argc, Value* vp)
 {
     CallArgs args = CallArgsFromVp(argc, vp);
 
-    RootedValue controllerVal(cx, args.callee().as<JSFunction>().getExtendedSlot(0));
-    Rooted<ReadableStreamController*> controller(cx);
-    controller = ToUnwrapped<ReadableStreamController>(cx, controllerVal);
+    Rooted<ReadableStreamController*> controller(cx,
+        UnwrapCalleeSlot<ReadableStreamController>(cx, args, 0));
     if (!controller) {
         return false;
     }
 
     bool pullAgain = controller->pullAgain();
 
     // Step a: Set controller.[[pulling]] to false.
     // Step b.i: Set controller.[[pullAgain]] to false.
@@ -2717,19 +2724,18 @@ ControllerPullHandler(JSContext* cx, uns
  * Upon rejection of pullPromise with reason e,
  */
 static bool
 ControllerPullFailedHandler(JSContext* cx, unsigned argc, Value* vp)
 {
     CallArgs args = CallArgsFromVp(argc, vp);
     HandleValue e = args.get(0);
 
-    RootedValue controllerVal(cx, args.callee().as<JSFunction>().getExtendedSlot(0));
-    Rooted<ReadableStreamController*> controller(cx);
-    controller = ToUnwrapped<ReadableStreamController>(cx, controllerVal);
+    Rooted<ReadableStreamController*> controller(cx,
+        UnwrapCalleeSlot<ReadableStreamController>(cx, args, 0));
     if (!controller) {
         return false;
     }
 
     // Step a: If controller.[[controlledReadableStream]].[[state]] is "readable",
     //         perform ! ReadableByteStreamControllerError(controller, e).
     if (controller->stream()->readable()) {
         if (!ReadableStreamControllerError(cx, controller, e)) {
@@ -2785,19 +2791,20 @@ ReadableStreamControllerCallPullIfNeeded
     if (!cx->compartment()->wrap(cx, &wrappedController)) {
         return false;
     }
     RootedValue controllerVal(cx, ObjectValue(*wrappedController));
     RootedValue unwrappedUnderlyingSource(cx, unwrappedController->underlyingSource());
     RootedObject pullPromise(cx);
 
     if (IsMaybeWrapped<TeeState>(unwrappedUnderlyingSource)) {
-        Rooted<TeeState*> unwrappedTeeState(cx);
-        unwrappedTeeState = &UncheckedUnwrap(&unwrappedUnderlyingSource.toObject())->as<TeeState>();
-        Rooted<ReadableStream*> stream(cx, unwrappedController->stream());
+        MOZ_ASSERT(unwrappedUnderlyingSource.toObject().is<TeeState>(),
+                   "tee streams and controllers are always same-compartment with the TeeState object");
+        Rooted<TeeState*> unwrappedTeeState(cx,
+            &unwrappedUnderlyingSource.toObject().as<TeeState>());
         pullPromise = ReadableStreamTee_Pull(cx, unwrappedTeeState);
     } else if (unwrappedController->hasExternalSource()) {
         {
             AutoRealm ar(cx, unwrappedController);
             Rooted<ReadableStream*> stream(cx, unwrappedController->stream());
             void* source = unwrappedUnderlyingSource.toPrivate();
             double desiredSize = ReadableStreamControllerGetDesiredSizeUnchecked(unwrappedController);
             cx->runtime()->readableStreamDataRequestCallback(cx,
new file mode 100644
--- /dev/null
+++ b/js/src/jit-test/tests/stream/bug-1503406.js
@@ -0,0 +1,13 @@
+let g = newGlobal();
+let reader = g.eval(`
+    let stream = new ReadableStream({
+        start(controller) {
+            controller.enqueue([]);
+        },
+    });
+    let [b1, b2] = stream.tee();
+    b1.getReader();
+`);
+let read = new ReadableStream({}).getReader().read;
+drainJobQueue();  // let the stream be started before reading
+read.call(reader);
--- a/js/src/vm/Compartment-inl.h
+++ b/js/src/vm/Compartment-inl.h
@@ -11,16 +11,17 @@
 
 #include <type_traits>
 
 #include "jsapi.h"
 #include "jsfriendapi.h"
 #include "jsnum.h"
 #include "gc/Barrier.h"
 #include "gc/Marking.h"
+#include "js/CallArgs.h"
 #include "js/Wrapper.h"
 #include "vm/Iteration.h"
 #include "vm/JSObject.h"
 
 #include "vm/JSContext-inl.h"
 
 inline bool
 JS::Compartment::wrap(JSContext* cx, JS::MutableHandleValue vp)
@@ -261,16 +262,52 @@ UnwrapAndTypeCheckArgument(JSContext* cx
                            int argIndex,
                            Rooted<T*>* unwrappedResult)
 {
     return UnwrapAndTypeCheckArgument(cx, args, methodName, argIndex,
                                       MutableHandle<T*>(unwrappedResult));
 }
 
 /**
+ * Unwrap a value of a known type.
+ *
+ * If `value` is an object of class T, this returns a pointer to that object.
+ * If `value` is a wrapper for such an object, this tries to unwrap the object
+ * and return a pointer to it. If access is denied, or `value` was a wrapper
+ * but has been nuked, this reports an error and returns null.
+ *
+ * In all other cases, the behavior is undefined, so call this only if `value`
+ * is known to have been initialized with an object of class T.
+ */
+template <class T>
+MOZ_MUST_USE T*
+UnwrapAndDowncastValue(JSContext* cx, const Value& value)
+{
+    static_assert(!std::is_convertible<T*, Wrapper*>::value,
+                  "T can't be a Wrapper type; this function discards wrappers");
+    JSObject* result = &value.toObject();
+    if (IsProxy(result)) {
+        if (JS_IsDeadWrapper(result)) {
+            JS_ReportErrorNumberASCII(cx, GetErrorMessage, nullptr, JSMSG_DEAD_OBJECT);
+            return nullptr;
+        }
+
+        // It would probably be OK to do an unchecked unwrap here, but we allow
+        // arbitrary security policies, so check anyway.
+        result = CheckedUnwrap(result);
+        if (!result) {
+            ReportAccessDenied(cx);
+            return nullptr;
+        }
+    }
+
+    return &result->as<T>();
+}
+
+/**
  * Read a private slot that is known to point to a particular type of object.
  *
  * Some internal slots specified in various standards effectively have static
  * types. For example, the [[ownerReadableStream]] slot of a stream reader is
  * guaranteed to be a ReadableStream. However, because of compartments, we
  * sometimes store a cross-compartment wrapper in that slot. And since wrappers
  * can be nuked, that wrapper may become a dead object proxy.
  *
@@ -289,44 +326,47 @@ MOZ_MUST_USE bool
 UnwrapInternalSlot(JSContext* cx,
                    Handle<NativeObject*> unwrappedObj,
                    uint32_t slot,
                    MutableHandle<T*> unwrappedResult)
 {
     static_assert(!std::is_convertible<T*, Wrapper*>::value,
                   "T can't be a Wrapper type; this function discards wrappers");
 
-    JSObject* result = &unwrappedObj->getFixedSlot(slot).toObject();
-    if (IsProxy(result)) {
-        if (JS_IsDeadWrapper(result)) {
-            JS_ReportErrorNumberASCII(cx, GetErrorMessage, nullptr, JSMSG_DEAD_OBJECT);
-            return false;
-        }
-
-        // It would probably be OK to do an unchecked unwrap here, but we allow
-        // arbitrary security policies, so check anyway.
-        result = CheckedUnwrap(result);
-        if (!result) {
-            ReportAccessDenied(cx);
-            return false;
-        }
+    T* result = UnwrapAndDowncastValue<T>(cx, unwrappedObj->getFixedSlot(slot));
+    if (!result) {
+        return false;
     }
-
-    unwrappedResult.set(&result->as<T>());
+    unwrappedResult.set(result);
     return true;
 }
 
 /**
  * Extra signature so callers don't have to specify T explicitly.
  */
 template <class T>
 inline MOZ_MUST_USE bool
 UnwrapInternalSlot(JSContext* cx,
                    Handle<NativeObject*> obj,
                    uint32_t slot,
                    Rooted<T*>* unwrappedResult)
 {
     return UnwrapInternalSlot(cx, obj, slot, MutableHandle<T*>(unwrappedResult));
 }
 
+/**
+ * Read a function slot that is known to point to a particular type of object.
+ *
+ * This is like UnwrapInternalSlot, but for extended function slots. Call this
+ * only if the specified slot is known to have been initialized with an object
+ * of class T or a wrapper for such an object.
+ */
+template <class T>
+MOZ_MUST_USE T*
+UnwrapCalleeSlot(JSContext* cx, CallArgs& args, size_t extendedSlot)
+{
+    JSFunction& func = args.callee().as<JSFunction>();
+    return UnwrapAndDowncastValue<T>(cx, func.getExtendedSlot(extendedSlot));
+}
+
 } // namespace js
 
 #endif /* vm_Compartment_inl_h */