fix: stack overflow and hang due to cyclic ingredients

sdk/src/reader.rs

@@ -17,7 +17,7 @@
 #[cfg(feature = "file_io")]
 use std::fs::{read, File};
 use std::{
-    collections::HashMap,
+    collections::{HashMap, HashSet},
     io::{Read, Seek, Write},
 };
 
@@ -876,8 +876,11 @@ impl Reader {
     ) -> Result<HashMap<String, Value>> {
         let mut assertion_values = HashMap::new();
         let mut stack: Vec<(String, Option<String>)> = vec![(manifest_label.to_string(), None)];
+        let mut seen = HashSet::new();
 
         while let Some((current_label, parent_uri)) = stack.pop() {
+            seen.insert(current_label.clone());
+
             // If we're processing an ingredient, push its URI to the validation log
             if let Some(uri) = &parent_uri {
                 validation_log.push_ingredient_uri(uri.clone());
@@ -931,11 +934,14 @@ impl Reader {
             // Add ingredients to stack for processing
             for ingredient in manifest.ingredients().iter() {
                 if let Some(label) = ingredient.active_manifest() {
-                    let ingredient_uri = crate::jumbf::labels::to_assertion_uri(
-                        &current_label,
-                        ingredient.label().unwrap_or("unknown"),
-                    );
-                    stack.push((label.to_string(), Some(ingredient_uri)));
+                    // REVIEW-NOTE: should we error if there's a cyclic ingredient?
+                    if !seen.contains(label) {
+                        let ingredient_uri = crate::jumbf::labels::to_assertion_uri(
+                            &current_label,
+                            ingredient.label().unwrap_or("unknown"),
+                        );
+                        stack.push((label.to_string(), Some(ingredient_uri)));
+                    }
                 }
             }
 

sdk/src/store.rs

@@ -4019,6 +4019,24 @@ impl Store {
         svi: &mut StoreValidationInfo<'a>,
         recurse: bool,
         validation_log: &mut StatusTracker,
+    ) -> Result<()> {
+        Store::get_claim_referenced_manifests_impl(
+            claim,
+            store,
+            svi,
+            recurse,
+            validation_log,
+            &mut HashSet::new(),
+        )
+    }
+
+    fn get_claim_referenced_manifests_impl<'a>(
+        claim: &'a Claim,
+        store: &'a Store,
+        svi: &mut StoreValidationInfo<'a>,
+        recurse: bool,
+        validation_log: &mut StatusTracker,
+        seen: &mut HashSet<&'a str>,
     ) -> Result<()> {
         // add in current redactions
         if let Some(c_redactions) = claim.redactions() {
@@ -4051,13 +4069,17 @@ impl Store {
                     .insert(claim_label.clone());
 
                 // recurse nested ingredients
-                if recurse {
-                    Store::get_claim_referenced_manifests(
+                // REVIEW-NOTE: add an error to the validation log if seen already?
+                if recurse && !seen.contains(ingredient.label()) {
+                    seen.insert(ingredient.label());
+
+                    Store::get_claim_referenced_manifests_impl(
                         ingredient,
                         store,
                         svi,
                         recurse,
                         validation_log,
+                        seen,
                     )?;
                 }
             } else {

sdk/tests/test_builder.rs

@@ -11,11 +11,11 @@
 // specific language governing permissions and limitations under
 // each license.
 
-use std::io::{self, Cursor};
+use std::io::{self, Cursor, Seek};
 
 use c2pa::{
-    settings::Settings, validation_status, Builder, BuilderIntent, ManifestAssertionKind, Reader,
-    Result, ValidationState,
+    identity::validator::CawgValidator, settings::Settings, validation_status, Builder,
+    BuilderIntent, Error, ManifestAssertionKind, Reader, Result, ValidationState,
 };
 
 mod common;
@@ -79,6 +79,94 @@ fn test_builder_riff() -> Result<()> {
     Ok(())
 }
 
+// Constructs a C2PA asset that has an ingredient that references the main asset's active
+// manifest as the ingredients active manifest.
+//
+// Source: https://github.com/contentauth/c2pa-rs/issues/1554
+#[test]
+fn test_builder_cyclic_ingredient() -> Result<()> {
+    Settings::from_toml(include_str!("../tests/fixtures/test_settings.toml"))?;
+
+    let mut source = Cursor::new(include_bytes!("fixtures/no_manifest.jpg"));
+    let format = "image/jpeg";
+
+    let mut ingredient = Cursor::new(Vec::new());
+
+    // Start by making a basic ingredient.
+    let mut builder = Builder::new();
+    builder.set_intent(BuilderIntent::Edit);
+    builder.sign(&Settings::signer()?, format, &mut source, &mut ingredient)?;
+
+    source.rewind()?;
+    ingredient.rewind()?;
+
+    let mut dest = Cursor::new(Vec::new());
+
+    // Then create an asset with the basic ingredient.
+    let mut builder = Builder::new();
+    builder.set_intent(BuilderIntent::Edit);
+    builder.add_ingredient_from_stream(
+        serde_json::json!({}).to_string(),
+        format,
+        &mut ingredient,
+    )?;
+    builder.sign(&Settings::signer()?, format, &mut source, &mut dest)?;
+
+    dest.rewind()?;
+    ingredient.rewind()?;
+
+    let active_manifest_uri = Reader::from_stream(format, &mut dest)?
+        .active_label()
+        .unwrap()
+        .to_owned();
+    let ingredient_uri = Reader::from_stream(format, ingredient)?
+        .active_label()
+        .unwrap()
+        .to_owned();
+
+    // If they aren't the same number of bytes then we can't reliably substitute the URI.
+    assert_eq!(active_manifest_uri.len(), ingredient_uri.len());
+
+    // Replace the ingredient active manifest with the main active manifest.
+    let mut bytes = dest.into_inner();
+    let old = ingredient_uri.as_bytes();
+    let new = active_manifest_uri.as_bytes();
+
+    let mut i = 0;
+    while i + old.len() <= bytes.len() {
+        if &bytes[i..i + old.len()] == old {
+            bytes[i..i + old.len()].copy_from_slice(new);
+            i += old.len();
+        } else {
+            i += 1;
+        }
+    }
+
+    // Attempt to read the manifest with a cyclical ingredient.
+    let mut cyclic_ingredient = Cursor::new(bytes);
+    assert!(matches!(
+        Reader::from_stream(format, &mut cyclic_ingredient),
+        Err(Error::HashMismatch(..))
+    ));
+
+    cyclic_ingredient.rewind()?;
+
+    // Read the manifest without validating so we can test with post-validating the CAWG.
+    Settings::from_toml(
+        &toml::toml! {
+            [verify]
+            verify_after_reading = false
+        }
+        .to_string(),
+    )?;
+    let mut reader = Reader::from_stream(format, cyclic_ingredient)?;
+    // Ideally we'd use a sync path for this. There are limitations for tokio on WASM.
+    #[cfg(not(target_arch = "wasm32"))]
+    tokio::runtime::Runtime::new()?.block_on(reader.post_validate_async(&CawgValidator {}))?;
+
+    Ok(())
+}
+
 #[test]
 fn test_builder_sidecar_only() -> Result<()> {
     Settings::from_toml(include_str!("../tests/fixtures/test_settings.toml"))?;