Bug 1644816 - Track number of deserialized SavedFrame children to fix error handling r=jonco

Differential Revision: https://phabricator.services.mozilla.com/D80562

js/src/vm/StructuredClone.cpp

@@ -1451,6 +1451,31 @@ static bool TryAppendNativeProperties(JSContext* cx, HandleObject obj,
   return true;
 }
 
+// Objects are written as a "preorder" traversal of the object graph: object
+// "headers" (the class tag and any data needed for initial construction) are
+// visited first, then the children are recursed through (where children are
+// properties, Set or Map entries, etc.). So for example
+//
+//     obj1 = { key1: { key1.1: val1.1, key1.2: val1.2 }, key2: {} }
+//
+// would be stored as:
+//
+//     <Object tag for obj1>
+//       <key1 data>
+//       <Object tag for key1's value>
+//         <key1.1 data>
+//         <val1.1 data>
+//         <key1.2 data>
+//         <val1.2 data>
+//       <end-of-children marker for key1's value>
+//       <key2 data>
+//       <Object tag for key2's value>
+//       <end-of-children marker for key2's value>
+//     <end-of-children marker for obj1>
+//
+// This nests nicely (ie, an entire recursive value starts with its tag and
+// ends with its end-of-children marker) and so it can be presented indented.
+// But see traverseMap below for how this looks different for Maps.
 bool JSStructuredCloneWriter::traverseObject(HandleObject obj, ESClass cls) {
   size_t count;
   bool optimized = false;
@@ -1508,6 +1533,26 @@ bool JSStructuredCloneWriter::traverseObject(HandleObject obj, ESClass cls) {
   return out.writePair(SCTAG_OBJECT_OBJECT, 0);
 }
 
+// Use the same basic setup as for traverseObject, but now keys can themselves
+// be complex objects. Keys and values are visited first via startWrite(), then
+// the key's children (if any) are handled, then the value's children.
+//
+//     m = new Map();
+//     m.set(key1 = ..., value1 = ...)
+//
+// where key1 and value2 are both objects would be stored as
+//
+//     <Map tag>
+//     <key1 class tag>
+//     <value1 class tag>
+//     ...key1 data...
+//     <end-of-children marker for key1>
+//     ...value1 data...
+//     <end-of-children marker for value1>
+//     <end-of-children marker for Map>
+//
+// Notice how the end-of-children marker for key1 is sandwiched between the
+// value1 beginning and end.
 bool JSStructuredCloneWriter::traverseMap(HandleObject obj) {
   Rooted<GCVector<Value>> newEntries(context(), GCVector<Value>(context()));
   {
@@ -1540,6 +1585,9 @@ bool JSStructuredCloneWriter::traverseMap(HandleObject obj) {
   return out.writePair(SCTAG_MAP_OBJECT, 0);
 }
 
+// Similar to traverseMap, only there is a single value instead of a key and
+// value, and thus no interleaving is possible: a value will be fully emitted
+// before the next value is begun.
 bool JSStructuredCloneWriter::traverseSet(HandleObject obj) {
   Rooted<GCVector<Value>> keys(context(), GCVector<Value>(context()));
   {
@@ -1572,25 +1620,6 @@ bool JSStructuredCloneWriter::traverseSet(HandleObject obj) {
   return out.writePair(SCTAG_SET_OBJECT, 0);
 }
 
-// Objects are written as a "preorder" traversal of the object graph: object
-// "headers" (the class tag and any data needed for initial construction) are
-// visited first, then the children are recursed through (where children are
-// properties, Set or Map entries, etc.). So for example
-//
-//     m = new Map();
-//     m.set(key1 = {}, value1 = {})
-//
-// would be stored as
-//
-//     <Map tag>
-//     <key1 class tag>
-//     <value1 class tag>
-//     <end-of-children marker for key1>
-//     <end-of-children marker for value1>
-//     <end-of-children marker for Map>
-//
-// Notice how the end-of-children marker for key1 is sandwiched between the
-// value1 beginning and end.
 bool JSStructuredCloneWriter::traverseSavedFrame(HandleObject obj) {
   RootedSavedFrame savedFrame(context(), obj->maybeUnwrapAs<SavedFrame>());
   MOZ_ASSERT(savedFrame);
@@ -3027,6 +3056,79 @@ JSObject* JSStructuredCloneReader::readSavedFrame(uint32_t principalsTag) {
   return savedFrame;
 }
 
+// Class for counting "children" (actually parent frames) of the SavedFrames on
+// the `objs` stack. When a SavedFrame is complete, it should have exactly 1
+// parent frame.
+//
+// This class must be notified after every startRead() call.
+//
+// If we add other types with restrictions on the number of children, this
+// should be expanded to handle those types as well.
+//
+class ChildCounter {
+  JSContext* cx;
+  Vector<size_t> counts;
+  size_t objsLength;
+  size_t objCountsIndex;
+
+ public:
+  explicit ChildCounter(JSContext* cx)
+   : cx(cx), counts(cx), objsLength(0) {}
+
+  void noteObjIsOnTopOfStack() { objCountsIndex = counts.length() - 1; }
+
+  // startRead() will have pushed any newly seen object onto the `objs` stack.
+  // If it did not read an object, or if the object it read was a backreference
+  // to an earlier object, the stack will be unchanged.
+  bool postStartRead(HandleValueVector objs) {
+    if (objs.length() == objsLength) {
+      // No new object pushed.
+      return true;
+    }
+
+    // Push a new child counter (initialized to zero) for the new object.
+    MOZ_ASSERT(objs.length() == objsLength + 1);
+    objsLength = objs.length();
+    if (objs.back().toObject().is<SavedFrame>()) {
+      return counts.append(0);
+    }
+
+    // Not a SavedFrame; do nothing.
+    return true;
+  }
+
+  // Reading has reached the end of the children for an object. Check whether
+  // we saw the right number of children.
+  bool handleEndOfChildren(HandleValueVector objs) {
+    MOZ_ASSERT(objsLength > 0);
+    objsLength--;
+
+    if (objs.back().toObject().is<SavedFrame>()) {
+      if (counts.back() != 1) {
+        JS_ReportErrorNumberASCII(cx, GetErrorMessage, nullptr,
+                                  JSMSG_SC_BAD_SERIALIZED_DATA,
+                                  "must have single SavedFrame parent");
+        return false;
+      }
+
+      counts.popBack();
+    }
+    return true;
+  }
+
+  // While we are reading children, we need to know whether this is the first
+  // child seen or not, in order to avoid double-initializing in the error
+  // case.
+  bool checkSingleParentFrame() {
+    // We are checking at a point where we have read 0 or more parent frames,
+    // in which case `obj` may not be on top of the `objs` stack anymore and
+    // the count on top of the `counts` stack will correspond to the most
+    // recently read frame, not `obj`. Use the remembered `counts` index from
+    // when `obj` *was* on top of the stack.
+    return ++counts[objCountsIndex] == 1;
+  }
+};
+
 // Perform the whole recursive reading procedure.
 bool JSStructuredCloneReader::read(MutableHandleValue vp) {
   if (!readHeader()) {
@@ -3037,10 +3139,12 @@ bool JSStructuredCloneReader::read(MutableHandleValue vp) {
     return false;
   }
 
+  ChildCounter childCounter(context());
+
   // Start out by reading in the main object and pushing it onto the 'objs'
   // stack. The data related to this object and its descendants extends from
   // here to the SCTAG_END_OF_KEYS at the end of the stream.
-  if (!startRead(vp)) {
+  if (!startRead(vp) || !childCounter.postStartRead(objs)) {
     return false;
   }
 
@@ -3048,6 +3152,7 @@ bool JSStructuredCloneReader::read(MutableHandleValue vp) {
   while (objs.length() != 0) {
     // What happens depends on the top obj on the objs stack.
     RootedObject obj(context(), &objs.back().toObject());
+    childCounter.noteObjIsOnTopOfStack();
 
     uint32_t tag, data;
     if (!in.getPair(&tag, &data)) {
@@ -3055,6 +3160,10 @@ bool JSStructuredCloneReader::read(MutableHandleValue vp) {
     }
 
     if (tag == SCTAG_END_OF_KEYS) {
+      if (!childCounter.handleEndOfChildren(objs)) {
+        return false;
+      }
+
       // Pop the current obj off the stack, since we are done with it and
       // its children.
       MOZ_ALWAYS_TRUE(in.readPair(&tag, &data));
@@ -3073,11 +3182,10 @@ bool JSStructuredCloneReader::read(MutableHandleValue vp) {
     // loop, processing the first child obj, and continuing until all
     // children have been fully created.
     //
-    // Note that this means the ordering in the stream is a little funky
-    // for things like Map. See the comment above startWrite() for an
-    // example.
+    // Note that this means the ordering in the stream is a little funky for
+    // things like Map. See the comment above traverseMap() for an example.
     RootedValue key(context());
-    if (!startRead(&key)) {
+    if (!startRead(&key) || !childCounter.postStartRead(objs)) {
       return false;
     }
 
@@ -3085,6 +3193,9 @@ bool JSStructuredCloneReader::read(MutableHandleValue vp) {
                           obj->is<SavedFrame>())) {
       // Backwards compatibility: Null formerly indicated the end of
       // object properties.
+      if (!childCounter.handleEndOfChildren(objs)) {
+        return false;
+      }
       objs.popBack();
       continue;
     }
@@ -3098,8 +3209,8 @@ bool JSStructuredCloneReader::read(MutableHandleValue vp) {
       continue;
     }
 
-    // SavedFrame object: there is one following value, the parent
-    // SavedFrame, which is either null or another SavedFrame object.
+    // SavedFrame object: there is one following value, the parent SavedFrame,
+    // which is either null or another SavedFrame object.
     if (obj->is<SavedFrame>()) {
       SavedFrame* parentFrame;
       if (key.isNull()) {
@@ -3107,17 +3218,27 @@ bool JSStructuredCloneReader::read(MutableHandleValue vp) {
       } else if (key.isObject() && key.toObject().is<SavedFrame>()) {
         parentFrame = &key.toObject().as<SavedFrame>();
       } else {
+        JS_ReportErrorNumberASCII(context(), GetErrorMessage, nullptr,
+                                  JSMSG_SC_BAD_SERIALIZED_DATA,
+                                  "invalid SavedFrame parent");
         return false;
       }
 
-      obj->as<SavedFrame>().initParent(parentFrame);
+      if (!childCounter.checkSingleParentFrame()) {
+        // This is an error (more than one parent given), but it will be
+        // reported when the SavedFrame is complete so it can be handled along
+        // with the "no parent given" case.
+      } else {
+        obj->as<SavedFrame>().initParent(parentFrame);
+      }
+
       continue;
     }
 
     // Everything else uses a series of key,value,key,value,... Value
     // objects.
     RootedValue val(context());
-    if (!startRead(&val)) {
+    if (!startRead(&val) || !childCounter.postStartRead(objs)) {
       return false;
     }