fix: stack overflow and hang due to cyclic ingredients

sdk/src/error.rs

@@ -262,6 +262,9 @@ pub enum Error {
     #[error("hash verification( {0} )")]
     HashMismatch(String),
 
+    #[error("cyclic ingredient found in path: {claim_label_path:?}")]
+    CyclicIngredients { claim_label_path: Vec<String> },
+
     #[error("claim verification failure: {0}")]
     ClaimVerification(String),
 

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: if we already saw this manifest, do we need to compute it again here?
+                    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

@@ -4020,16 +4020,41 @@ impl Store {
         recurse: bool,
         validation_log: &mut StatusTracker,
     ) -> Result<()> {
+        Self::get_claim_referenced_manifests_impl(
+            claim,
+            store,
+            svi,
+            recurse,
+            validation_log,
+            &mut Vec::new(),
+        )
+    }
+
+    fn get_claim_referenced_manifests_impl<'a>(
+        claim: &'a Claim,
+        store: &'a Store,
+        svi: &mut StoreValidationInfo<'a>,
+        recurse: bool,
+        validation_log: &mut StatusTracker,
+        claim_label_path: &mut Vec<&'a str>,
+    ) -> Result<()> {
+        let claim_label = claim.label();
+
+        // REVIEW-NOTE: added this so we don't compute the same ingredients more than once.
+        if svi.manifest_map.contains_key(claim_label) {
+            return Ok(());
+        }
+
+        claim_label_path.push(claim_label);
+
         // add in current redactions
         if let Some(c_redactions) = claim.redactions() {
             svi.redactions
                 .append(&mut c_redactions.clone().into_iter().collect::<Vec<_>>());
         }
 
-        let claim_label = claim.label().to_owned();
-
         // save the addressible claims for quicker lookup
-        svi.manifest_map.insert(claim_label.clone(), claim);
+        svi.manifest_map.insert(claim_label.to_owned(), claim);
 
         for i in claim.ingredient_assertions() {
             let ingredient_assertion = Ingredient::from_assertion(i.assertion())?;
@@ -4044,20 +4069,39 @@ impl Store {
             let ingredient_label = Store::manifest_label_from_path(&c2pa_manifest.url());
 
             if let Some(ingredient) = store.get_claim(&ingredient_label) {
+                if claim_label_path.contains(&ingredient.label()) {
+                    return Err(log_item!(
+                        jumbf::labels::to_assertion_uri(claim_label, &i.label()),
+                        "ingredient cannot be cyclic",
+                        "ingredient_checks"
+                    )
+                    .validation_status(validation_status::ASSERTION_INGREDIENT_MALFORMED)
+                    .failure_as_err(
+                        validation_log,
+                        Error::CyclicIngredients {
+                            claim_label_path: claim_label_path
+                                .iter()
+                                .map(|&label| label.to_owned())
+                                .collect(),
+                        },
+                    ));
+                }
+
                 // build mapping of ingredients and those claims that reference it
                 svi.ingredient_references
-                    .entry(ingredient_label)
-                    .or_insert(HashSet::from_iter(vec![claim_label.clone()].into_iter()))
-                    .insert(claim_label.clone());
+                    .entry(ingredient_label.clone())
+                    .or_insert(HashSet::from_iter(vec![claim_label.to_owned()].into_iter()))
+                    .insert(claim_label.to_owned());
 
                 // recurse nested ingredients
                 if recurse {
-                    Store::get_claim_referenced_manifests(
+                    Store::get_claim_referenced_manifests_impl(
                         ingredient,
                         store,
                         svi,
                         recurse,
                         validation_log,
+                        claim_label_path,
                     )?;
                 }
             } else {
@@ -4076,6 +4120,8 @@ impl Store {
             }
         }
 
+        claim_label_path.pop();
+
         Ok(())
     }
 

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::CyclicIngredients { .. })
+    ));
+
+    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"))?;