Fix handling of self-referential amf3 objects

This fixes some instances of the "failed to get object" panic in ruffle by no longer trying to handle value cycles
Now all object values are tagged with an ObjectId and the new Value::Amf3ObjectReference's refer back to this so consumers can choose how to handle cycles (e.g. in Ruffles case, via the GC)

Also moves raw-amf/byteobject tests into their own dir, adds a test for amf3 cycles and improves the lso-to-json tool to support bulk-test json recreation and generating raw-amf tests

flash-lso/src/amf0/read.rs

@@ -4,7 +4,7 @@ use crate::amf0::type_marker::TypeMarker;
 #[cfg(feature = "amf3")]
 use crate::amf3;
 use crate::nom_utils::{take_str, AMFResult};
-use crate::types::{ClassDefinition, Element, Reference, Value};
+use crate::types::{ClassDefinition, Element, ObjectId, Reference, Value};
 use crate::PADDING;
 use nom::bytes::complete::{tag, take};
 use nom::combinator::{map, map_res};
@@ -97,6 +97,7 @@ impl AMF0Decoder {
             |i| self.parse_array_element(i),
             move |elms: Vec<Element>| {
                 Rc::new(Value::Object(
+                    ObjectId::INVALID,
                     elms,
                     Some(ClassDefinition::default_with_name(name.to_string())),
                 ))
@@ -106,7 +107,7 @@ impl AMF0Decoder {
 
     fn parse_element_object<'a>(&mut self, i: &'a [u8]) -> AMFResult<'a, Rc<Value>> {
         let (i, v) = self.parse_array_element(i)?;
-        Ok((i, Rc::new(Value::Object(v, None))))
+        Ok((i, Rc::new(Value::Object(ObjectId::INVALID, v, None))))
     }
 
     #[cfg(fuzzing)]

flash-lso/src/amf0/write.rs

@@ -169,7 +169,7 @@ pub(crate) fn write_value<'a, 'b: 'a, W: Write + 'a>(
                 write_string_element(writer, s)
             }
         }
-        Value::Object(elements, class_def) => {
+        Value::Object(_, elements, class_def) => {
             if let Some(class_def) = class_def {
                 write_typed_object_element(writer, &class_def.name, elements)
             } else {

flash-lso/src/amf0/writer/object_writer.rs

@@ -1,6 +1,6 @@
 use std::rc::Rc;
 
-use crate::types::{Element, Reference, Value};
+use crate::types::{Element, ObjectId, Reference, Value};
 
 use super::{ArrayWriter, CacheKey, ObjWriter};
 
@@ -94,7 +94,10 @@ impl<'a, 'b> ObjectWriter<'a, 'b> {
     /// If this is not called, the object will not be added
     pub fn commit<T: AsRef<str>>(self, name: T) {
         //TODO: this doent work for multi level nesting
-        self.parent
-            .add_element(name.as_ref(), Value::Object(self.elements, None), false);
+        self.parent.add_element(
+            name.as_ref(),
+            Value::Object(ObjectId::INVALID, self.elements, None),
+            false,
+        );
     }
 }

flash-lso/src/amf3/read.rs

@@ -16,7 +16,7 @@ use nom::number::complete::{be_f64, be_i32, be_u32, be_u8};
 use nom::Err;
 
 use std::convert::{TryFrom, TryInto};
-use std::ops::DerefMut;
+use std::ops::{Deref, DerefMut};
 use std::rc::Rc;
 
 const REFERENCE_FLAG: u32 = 0x01;
@@ -151,12 +151,17 @@ fn parse_element_int(i: &[u8]) -> AMFResult<'_, Rc<Value>> {
 pub struct AMF3Decoder {
     /// The table used to cache repeated byte strings
     pub string_reference_table: Vec<Vec<u8>>,
+
     /// The table used to cache repeated trait definitions
     pub trait_reference_table: Vec<ClassDefinition>,
+
     /// The table used to cache repeated objects
     pub object_reference_table: Vec<Rc<Value>>,
+
     /// Encoders used for handling externalized types
     pub external_decoders: HashMap<String, ExternalDecoderFn>,
+
+    object_id: i64,
 }
 
 fn parse_element_number(i: &[u8]) -> AMFResult<'_, Rc<Value>> {
@@ -345,17 +350,25 @@ impl AMF3Decoder {
                 .try_into()
                 .map_err(|_| Err::Error(make_error(i, ErrorKind::Digit)))?;
 
-            let obj = Rc::clone(
-                self.object_reference_table
-                    .get(len_usize)
-                    .ok_or_else(|| Err::Error(make_error(i, ErrorKind::Digit)))?,
-            );
+            let o = self
+                .object_reference_table
+                .get(len_usize)
+                .ok_or_else(|| Err::Error(make_error(i, ErrorKind::Digit)))?;
+            let id = if let Value::Object(id, _, _) = o.deref() {
+                *id
+            } else {
+                unreachable!("object_reference_table should only have objects")
+            };
+
+            let obj = Rc::new(Value::Amf3ObjectReference(id));
 
             return Ok((i, obj));
         }
         length >>= 1;
 
-        let obj = Rc::new(Value::Object(Vec::new(), None));
+        self.object_id += 1;
+        let obj = Rc::new(Value::Object(ObjectId(self.object_id), Vec::new(), None));
+
         let index = self.object_reference_table.len();
         self.object_reference_table.push(obj);
 
@@ -369,7 +382,7 @@ impl AMF3Decoder {
                     .expect("Index invalid"),
             )
             .expect("Unable to get Object");
-            if let Value::Object(_, ref mut def) = mut_obj {
+            if let Value::Object(_, _, ref mut def) = mut_obj {
                 *def = Some(class_def.clone());
             }
         }
@@ -433,7 +446,7 @@ impl AMF3Decoder {
                     .expect("Index invalid"),
             )
             .expect("Unable to get Object");
-            if let Value::Object(ref mut elements_inner, _) = mut_obj {
+            if let Value::Object(_, ref mut elements_inner, _) = mut_obj {
                 *elements_inner = elements;
             }
         }

flash-lso/src/amf3/write.rs

@@ -3,11 +3,11 @@ use crate::amf3::custom_encoder::CustomEncoder;
 use crate::amf3::element_cache::ElementCache;
 use crate::amf3::length::Length;
 use crate::amf3::type_marker::TypeMarker;
-use crate::types::{Attribute, ClassDefinition, Element, Value};
+use crate::types::{Attribute, ClassDefinition, Element, ObjectId, Value};
 use crate::write::WriteExt;
 use crate::PADDING;
 use std::cell::RefCell;
-use std::collections::HashMap;
+use std::collections::{BTreeMap, HashMap};
 use std::io::Result;
 use std::io::Write;
 use std::ops::Deref;
@@ -27,6 +27,8 @@ pub struct AMF3Encoder {
 
     /// Encoders used for handling externalized types
     pub external_encoders: HashMap<String, Box<dyn CustomEncoder>>,
+
+    object_id_to_reference: RefCell<BTreeMap<ObjectId, usize>>,
 }
 
 impl AMF3Encoder {
@@ -422,14 +424,18 @@ impl AMF3Encoder {
     fn write_object_element<'a, 'b: 'a, W: Write + 'a>(
         &'a self,
         writer: &mut W,
+        id: ObjectId,
         children: &'b [Element],
         custom_props: Option<&'b [Element]>,
         class_def: &'b Option<ClassDefinition>,
     ) -> Result<()> {
         let had_object = Length::Size(0);
 
-        self.object_reference_table
-            .store(Value::Object(children.to_vec(), class_def.clone()));
+        let obj = Value::Object(id, children.to_vec(), class_def.clone());
+        self.object_reference_table.store(obj.clone());
+        if let Length::Reference(r) = self.object_reference_table.to_length(obj, 0) {
+            self.object_id_to_reference.borrow_mut().insert(id, r);
+        }
 
         let def = class_def.clone().unwrap_or_default();
         let def2 = def.clone();
@@ -579,8 +585,8 @@ impl AMF3Encoder {
             Value::Number(x) => self.write_number_element(writer, *x),
             Value::Bool(b) => self.write_boolean_element(writer, *b),
             Value::String(s) => self.write_string_element(writer, s),
-            Value::Object(children, class_def) => {
-                self.write_object_element(writer, children, None, class_def)
+            Value::Object(id, children, class_def) => {
+                self.write_object_element(writer, *id, children, None, class_def)
             }
             Value::Null => self.write_null_element(writer),
             Value::Undefined => self.write_undefined_element(writer),
@@ -608,12 +614,25 @@ impl AMF3Encoder {
                 self.write_dictionary_element(writer, kv, *weak_keys)
             }
 
-            Value::Custom(elements, dynamic_elements, def) => {
-                self.write_object_element(writer, dynamic_elements, Some(elements), def)
-            }
+            Value::Custom(elements, dynamic_elements, def) => self.write_object_element(
+                writer,
+                ObjectId::INVALID,
+                dynamic_elements,
+                Some(elements),
+                def,
+            ),
             Value::AMF3(e) => self.write_value_element(writer, e),
             Value::Unsupported => self.write_undefined_element(writer),
             Value::Reference(_) => unimplemented!(),
+            Value::Amf3ObjectReference(id) => {
+                let r = *self
+                    .object_id_to_reference
+                    .borrow()
+                    .get(id)
+                    .expect("Invalid reference");
+                self.write_type_marker(writer, TypeMarker::Object)?;
+                self.write_object_reference(writer, r as u32)
+            }
         }
     }
 

flash-lso/src/types/mod.rs

@@ -7,11 +7,14 @@ mod lso_header;
 mod reference;
 mod value;
 
+mod object_id;
+
 pub use amf_version::AMFVersion;
 pub use attribute::Attribute;
 pub use class_definition::ClassDefinition;
 pub use element::Element;
 pub use lso::Lso;
 pub use lso_header::Header;
+pub use object_id::ObjectId;
 pub use reference::Reference;
 pub use value::Value;

flash-lso/src/types/object_id.rs

@@ -0,0 +1,15 @@
+/// A locally unique identifier for an Amf3 object
+/// See the comment on `Value::Amf3ObjectReference` for details
+#[cfg_attr(feature = "serde", derive(Serialize, Deserialize))]
+#[derive(Copy, Clone, Debug, Eq, PartialEq, Ord, PartialOrd, Hash)]
+pub struct ObjectId(pub i64);
+
+impl ObjectId {
+    /// An invalid object id
+    ///
+    /// Use this when the id of an object can't be known or will never need to be referenced (e.g. amf0)
+    /// Unlike valid object id's, multiple objects with an `INVALID` id are explicitly allowed
+    /// Attempting to write a reference to an invalid object id is illegal and may error
+    /// Attempting to write an object with an invalid object id is allowed, but it cannot be referenced later
+    pub const INVALID: Self = ObjectId(-1);
+}

flash-lso/src/types/value.rs

@@ -1,4 +1,4 @@
-use super::{ClassDefinition, Element, Reference};
+use super::{ClassDefinition, Element, ObjectId, Reference};
 use std::rc::Rc;
 
 //TODO: should amf3 assoc arrays be their own type with a dense and assoc section
@@ -16,7 +16,7 @@ pub enum Value {
     String(String),
 
     /// Represents the object type in both amf0 and amf3, class definition are only available with amf3
-    Object(Vec<Element>, Option<ClassDefinition>),
+    Object(ObjectId, Vec<Element>, Option<ClassDefinition>),
 
     /// Represent the null type
     Null,
@@ -77,6 +77,15 @@ pub enum Value {
 
     /// Represent an existing value, stored by reference, the value here should be considered opaque
     Reference(Reference),
+
+    /// A reference to a previously parsed element
+    ///
+    /// While traversing `Value`s you should maintain a mapping of `ObjectId` to your internal
+    /// representation of a value and consider this a reference to the exact same value.
+    ///
+    /// As `Value` graphs can contain cycles which are best handled by garbage collected structures
+    /// we leave the handling of this to the user, sorry
+    Amf3ObjectReference(ObjectId),
 }
 
 impl FromIterator<Value> for Vec<Rc<Value>> {

flash-lso/tests/amf/self-referential-object.json

@@ -0,0 +1 @@
+{"Object":[1,[{"name":"AAAA","value":{"Amf3ObjectReference":1}}],{"name":"","attributes":0,"static_properties":["AAAA"]}]}

flash-lso/tests/amf/self-referential-object.txt

@@ -0,0 +1,21 @@
+This file was generated with the following:
+
+let mut v = vec![];
+let  r = Rc::new(Value::Object(0, Vec::new().into(), Some(ClassDefinition {
+    name: "".to_string(),
+    attributes: Default::default(),
+    static_properties: vec![],
+})));
+
+let o = Value::Object(0, vec![
+    Element::new("AAAA", Rc::clone(&r))
+].into(), Some(ClassDefinition {
+    name: "".to_string(),
+    attributes: Default::default(),
+    static_properties: vec!["AAAA".to_string()],
+}));
+let oo = Rc::new(o);
+
+AMF3Encoder::default().write_value(&mut v, oo.as_ref()).unwrap();
+v[9] = 0;
+v.pop();

flash-lso/tests/integration_tests.rs

@@ -1,7 +1,11 @@
 use core::fmt;
 use flash_lso::errors::Error;
 use flash_lso::read::Reader;
+use flash_lso::types::Value;
 use nom::error::ErrorKind;
+use std::borrow::Borrow;
+use std::ops::Deref;
+
 // #[cfg(test)]
 // use pretty_assertions::assert_eq;
 
@@ -195,18 +199,19 @@ macro_rules! json_test_bytearray_object_amf3 {
         pub fn $name() -> Result<(), Box<dyn std::error::Error>> {
             use serde_json;
 
-            let data = include_bytes!(concat!("sol/", $path, ".sol"));
-            let parse_res = Reader::default().amf3_decoder.parse_single_element(data)?;
+            let data = include_bytes!(concat!("amf/", $path, ".amf"));
+            let (remaining, parse_res) = Reader::default().amf3_decoder.parse_single_element(data)?;
             let output_json = serde_json::to_string(&parse_res)?;
+            assert_eq!(remaining.len(), 0, "Unparsed data");
 
 
-            let json_expected = include_str!(concat!("sol/", $path, ".json"));
+            let json_expected = include_str!(concat!("amf/", $path, ".json"));
 
             assert_eq!(json_expected.trim(), output_json);
 
             let mut lso = flash_lso::types::Lso::new(vec![flash_lso::types::Element {
                 name: "".to_string(),
-                value: parse_res.1
+                value: parse_res,
             }], "", flash_lso::types::AMFVersion::AMF3);
 
             let bytes = flash_lso::write::write_to_bytes(&mut lso)
@@ -333,10 +338,6 @@ json_test! {
     [json_metadata_history, "MetadataHistory"]
 }
 
-json_test_bytearray_object_amf3! {
-    [json_learntofly3, "LearnToFly3.profileData.saveString"]
-}
-
 json_test_flex! {
     [json_opp_detail_prefs, "oppDetailPrefs"]
 }
@@ -424,6 +425,11 @@ packet_test! {
     [armorgames_auth_response, "armorgames_auth_response", false]
 }
 
+json_test_bytearray_object_amf3! {
+    [json_learntofly3, "LearnToFly3.profileData.saveString"],
+    [self_referential_object, "self-referential-object"]
+}
+
 // Samples that can be parsed but not written
 test_parse_only! {
     [infectonator_survivors_76561198009932603, "InfectonatorSurvivors76561198009932603"],
@@ -449,3 +455,16 @@ should_fail! {
     // OOB read
     [zero_four, "00000004", nom::Err::Error(Error::Nom(vec![0, 255, 0, 0, 0, 86, 0, 84, 47, 117, 112, 108, 111, 97, 100, 115, 46, 117, 110, 103, 114, 111, 117, 110, 100, 101, 100, 46, 110, 101, 116, 47, 53, 57, 50, 48, 48, 48, 47, 53, 57, 50, 52, 55, 51, 95, 77, 97, 100, 110, 101, 115, 115, 71, 97, 109, 101, 95, 85, 76, 84, 73, 77, 65, 84, 69, 46, 115, 119, 102, 47, 97, 114, 101, 110, 97, 77, 97, 100, 110, 101, 115, 115, 71, 97, 109, 101, 50, 46, 115, 111, 108].as_slice(), ErrorKind::Eof))]
 }
+
+#[test]
+pub fn test_recursive_object() {
+    let data = include_bytes!("./amf/self-referential-object.amf");
+    let (_, obj) = flash_lso::amf3::read::AMF3Decoder::default()
+        .parse_single_element(data)
+        .expect("Failed to parse object");
+    if let Value::Object(id, elem, _) = obj.borrow() {
+        assert_eq!(elem[0].value.deref(), &Value::Amf3ObjectReference(*id));
+    } else {
+        panic!("Expected object");
+    }
+}

flash-lso/tests/sol/AS2-Array-Demo.json

@@ -1 +1 @@
-{"header":{"length":87,"name":"AS2-Array-Demo","format_version":"AMF0"},"body":[{"name":"myIntArray","value":{"ECMAArray":[[],[{"name":"0","value":{"Number":1.0}},{"name":"1","value":{"Number":2.0}},{"name":"2","value":{"Number":3.0}}],3]}}]}
+{"header":{"length":87,"name":"AS2-Array-Demo","format_version":"AMF0"},"body":[{"name":"myIntArray","value":{"ECMAArray":[[],[{"name":"0","value":{"Number":1.0}},{"name":"1","value":{"Number":2.0}},{"name":"2","value":{"Number":3.0}}],3]}}]}

flash-lso/tests/sol/AS2-Boolean-Demo.json

@@ -1 +1 @@
-{"header":{"length":43,"name":"AS2-Boolean-Demo","format_version":"AMF0"},"body":[{"name":"myBool","value":{"Bool":true}}]}
+{"header":{"length":43,"name":"AS2-Boolean-Demo","format_version":"AMF0"},"body":[{"name":"myBool","value":{"Bool":true}}]}

flash-lso/tests/sol/AS2-Date-Demo.json

@@ -1 +1 @@
-{"header":{"length":49,"name":"AS2-Date-Demo","format_version":"AMF0"},"body":[{"name":"myDate","value":{"Date":[1409653383774.0,240]}}]}
+{"header":{"length":49,"name":"AS2-Date-Demo","format_version":"AMF0"},"body":[{"name":"myDate","value":{"Date":[1409653383774.0,240]}}]}

flash-lso/tests/sol/AS2-Integer-Demo.json

@@ -1 +1 @@
-{"header":{"length":49,"name":"AS2-Integer-Demo","format_version":"AMF0"},"body":[{"name":"myInt","value":{"Number":7.0}}]}
+{"header":{"length":49,"name":"AS2-Integer-Demo","format_version":"AMF0"},"body":[{"name":"myInt","value":{"Number":7.0}}]}

flash-lso/tests/sol/AS2-Null-Demo.json

@@ -1 +1 @@
-{"header":{"length":39,"name":"AS2-Null-Demo","format_version":"AMF0"},"body":[{"name":"myNull","value":"Null"}]}
+{"header":{"length":39,"name":"AS2-Null-Demo","format_version":"AMF0"},"body":[{"name":"myNull","value":"Null"}]}