From be41fa839fe99f152d26048675b290599492f16b Mon Sep 17 00:00:00 2001 From: Valentin Popov Date: Mon, 22 Jun 2026 16:02:16 +0400 Subject: fix: harden resource and world state correctness --- crates/fparkan-assets/src/lib.rs | 179 +++++++++++++++++++++++++++++++------ crates/fparkan-material/src/lib.rs | 49 ++++++++-- crates/fparkan-resource/src/lib.rs | 51 ++++++++++- crates/fparkan-world/src/lib.rs | 97 ++++++++++++++------ 4 files changed, 313 insertions(+), 63 deletions(-) diff --git a/crates/fparkan-assets/src/lib.rs b/crates/fparkan-assets/src/lib.rs index 20d08cb..9015c2c 100644 --- a/crates/fparkan-assets/src/lib.rs +++ b/crates/fparkan-assets/src/lib.rs @@ -6,7 +6,7 @@ use fparkan_msh::{decode_msh, validate_msh}; use fparkan_nres::{decode as decode_nres, ReadProfile}; use fparkan_path::{normalize_relative, NormalizedPath, PathPolicy, ResourceName}; use fparkan_prototype::{EffectivePrototype, PrototypeGeometry, PrototypeGraph}; -use fparkan_resource::{ResourceKey, ResourceRepository}; +use fparkan_resource::{ResourceError, ResourceKey, ResourceRepository}; use fparkan_texm::decode_texm; use std::collections::BTreeSet; use std::fmt; @@ -279,17 +279,13 @@ pub fn prepare_visual_with_repository( material_count += 1; for texture in material.document.texture_requests() { - resolve_texm(repository, &texture, &[TEXTURES_ARCHIVE, LIGHTMAP_ARCHIVE])?; + resolve_texture(repository, &texture)?; texture_count += 1; } } for lightmap in &wear.lightmaps { - resolve_texm( - repository, - &lightmap.lightmap, - &[LIGHTMAP_ARCHIVE, TEXTURES_ARCHIVE], - )?; + resolve_lightmap(repository, &lightmap.lightmap)?; lightmap_count += 1; } @@ -325,28 +321,59 @@ fn read_key( Ok(Arc::from(bytes.into_owned())) } +fn resolve_texture( + repository: &R, + name: &ResourceName, +) -> Result<(), AssetError> { + resolve_texm(repository, name, TEXTURES_ARCHIVE, "texture") +} + +fn resolve_lightmap( + repository: &R, + name: &ResourceName, +) -> Result<(), AssetError> { + resolve_texm(repository, name, LIGHTMAP_ARCHIVE, "lightmap") +} + fn resolve_texm( repository: &R, name: &ResourceName, - archives: &[&str], + archive: &str, + label: &'static str, ) -> Result<(), AssetError> { - for archive in archives { - let key = ResourceKey { - archive: parse_path(archive)?, - name: name.clone(), - type_id: None, - }; - match read_key(repository, &key, Some("texm")) { - Ok(bytes) => { - decode_texm(bytes).map_err(|err| AssetError::Texture(err.to_string()))?; - return Ok(()); - } - Err(AssetError::MissingDependency(_) | AssetError::Resource(_)) => {} - Err(err) => return Err(err), - } - } + let key = ResourceKey { + archive: parse_path(archive)?, + name: name.clone(), + type_id: None, + }; + let Some(bytes) = read_optional_key(repository, &key, Some(label))? else { + return Err(AssetError::MissingDependency(format!("{label} {name:?}"))); + }; + decode_texm(bytes) + .map(|_| ()) + .map_err(|err| AssetError::Texture(err.to_string())) +} - Err(AssetError::MissingDependency(format!("{name:?}"))) +fn read_optional_key( + repository: &R, + key: &ResourceKey, + label: Option<&str>, +) -> Result>, AssetError> { + let archive = match repository.open_archive(&key.archive) { + Ok(archive) => archive, + Err(ResourceError::MissingArchive | ResourceError::MissingEntry) => return Ok(None), + Err(err) => return Err(AssetError::Resource(format!("{label:?} {key:?}: {err}"))), + }; + let Some(handle) = repository + .find(archive, &key.name) + .map_err(|err| AssetError::Resource(format!("{label:?} {key:?}: {err}")))? + else { + return Ok(None); + }; + let bytes = repository + .read(handle) + .map_err(|err| AssetError::Resource(format!("{label:?} {key:?}: {err}")))?; + Ok(Some(Arc::from(bytes.into_owned()))) } fn sibling_name(key: &ResourceKey, extension: &str) -> Result { @@ -412,7 +439,7 @@ mod tests { use super::*; use fparkan_prototype::build_prototype_graph; use fparkan_resource::{resource_name, CachedResourceRepository}; - use fparkan_vfs::{DirectoryVfs, Vfs}; + use fparkan_vfs::{DirectoryVfs, MemoryVfs, Vfs}; use std::path::PathBuf; #[test] @@ -425,6 +452,47 @@ mod tests { assert_eq!(plan.model_count, 0); } + #[test] + fn texture_resolver_does_not_fallback_to_lightmap_archive() { + let texm = texm_payload(); + let repo = repository_with_archives(&[( + LIGHTMAP_ARCHIVE, + &[(b"TEX_ONLY".as_slice(), texm.as_slice())], + )]); + + let err = resolve_texture(&repo, &resource_name(b"TEX_ONLY")).expect_err("missing texture"); + + assert!(matches!(err, AssetError::MissingDependency(_))); + } + + #[test] + fn lightmap_resolver_does_not_fallback_to_texture_archive() { + let texm = texm_payload(); + let repo = repository_with_archives(&[( + TEXTURES_ARCHIVE, + &[(b"LM_ONLY".as_slice(), texm.as_slice())], + )]); + + let err = + resolve_lightmap(&repo, &resource_name(b"LM_ONLY")).expect_err("missing lightmap"); + + assert!(matches!(err, AssetError::MissingDependency(_))); + } + + #[test] + fn texture_resolver_does_not_continue_after_malformed_texture() { + let malformed = b"not texm".as_slice(); + let texm = texm_payload(); + let repo = repository_with_archives(&[ + (TEXTURES_ARCHIVE, &[(b"BAD".as_slice(), malformed)]), + (LIGHTMAP_ARCHIVE, &[(b"BAD".as_slice(), texm.as_slice())]), + ]); + + let err = resolve_texture(&repo, &resource_name(b"BAD")).expect_err("malformed texture"); + + assert!(matches!(err, AssetError::Texture(_))); + } + #[test] #[ignore = "requires licensed corpus"] fn prepares_real_unit_asset_plan() { @@ -480,4 +548,65 @@ mod tests { .join("testdata") .join(part) } + + fn repository_with_archives( + archives: &[(&str, &[(&[u8], &[u8])])], + ) -> CachedResourceRepository { + let mut vfs = MemoryVfs::default(); + for (archive, entries) in archives { + let path = parse_path(archive).expect("archive path"); + vfs.insert(path, Arc::from(build_nres(entries).into_boxed_slice())); + } + CachedResourceRepository::new(Arc::new(vfs)) + } + + fn texm_payload() -> Vec { + let mut out = Vec::new(); + out.extend_from_slice(&0x6d78_6554_u32.to_le_bytes()); + out.extend_from_slice(&1_u32.to_le_bytes()); + out.extend_from_slice(&1_u32.to_le_bytes()); + out.extend_from_slice(&1_u32.to_le_bytes()); + out.extend_from_slice(&0_u32.to_le_bytes()); + out.extend_from_slice(&0_u32.to_le_bytes()); + out.extend_from_slice(&0_u32.to_le_bytes()); + out.extend_from_slice(&565_u32.to_le_bytes()); + out.extend_from_slice(&0xffff_u16.to_le_bytes()); + out + } + + fn build_nres(entries: &[(&[u8], &[u8])]) -> Vec { + let mut out = vec![0; 16]; + let mut offsets = Vec::with_capacity(entries.len()); + for (_, payload) in entries { + offsets.push(u32::try_from(out.len()).expect("offset")); + out.extend_from_slice(payload); + let padding = (8 - (out.len() % 8)) % 8; + out.resize(out.len() + padding, 0); + } + let mut order: Vec = (0..entries.len()).collect(); + order.sort_by(|left, right| entries[*left].0.cmp(entries[*right].0)); + for (idx, (name, payload)) in entries.iter().enumerate() { + push_u32(&mut out, 0); + push_u32(&mut out, 0); + push_u32(&mut out, 0); + push_u32(&mut out, u32::try_from(payload.len()).expect("payload")); + push_u32(&mut out, 0); + let mut name_raw = [0; 36]; + let len = name_raw.len().saturating_sub(1).min(name.len()); + name_raw[..len].copy_from_slice(&name[..len]); + out.extend_from_slice(&name_raw); + push_u32(&mut out, offsets[idx]); + push_u32(&mut out, u32::try_from(order[idx]).expect("sort index")); + } + out[0..4].copy_from_slice(b"NRes"); + out[4..8].copy_from_slice(&0x100_u32.to_le_bytes()); + out[8..12].copy_from_slice(&u32::try_from(entries.len()).expect("count").to_le_bytes()); + let total_size = u32::try_from(out.len()).expect("total size"); + out[12..16].copy_from_slice(&total_size.to_le_bytes()); + out + } + + fn push_u32(out: &mut Vec, value: u32) { + out.extend_from_slice(&value.to_le_bytes()); + } } diff --git a/crates/fparkan-material/src/lib.rs b/crates/fparkan-material/src/lib.rs index a7ec5d7..2a05f87 100644 --- a/crates/fparkan-material/src/lib.rs +++ b/crates/fparkan-material/src/lib.rs @@ -417,15 +417,8 @@ pub fn resolve_material( { return Ok(resolved); } - if let Some(first) = table.entries.first() { - if let Some(resolved) = load_material_entry( - repository, - archive, - &first.material, - MaterialFallback::FirstEntry, - )? { - return Ok(resolved); - } + if let Some(resolved) = load_first_material_entry(repository, archive)? { + return Ok(resolved); } Err(MaterialError::MissingMaterial( String::from_utf8_lossy(&entry.material.0).into_owned(), @@ -610,6 +603,26 @@ fn load_material_entry( })) } +fn load_first_material_entry( + repository: &dyn ResourceRepository, + archive: fparkan_resource::ArchiveId, +) -> Result, MaterialError> { + let Some(handle) = repository.first_entry(archive)? else { + return Ok(None); + }; + let info = repository.entry_info(handle)?; + if info.key.type_id != Some(MAT0_KIND) { + return Ok(None); + } + let bytes = repository.read(handle)?.into_owned(); + let document = decode_mat0(&bytes, info.attr2)?; + Ok(Some(ResolvedMaterial { + name: info.key.name, + fallback: MaterialFallback::FirstEntry, + document, + })) +} + fn parse_lightmaps(lines: &[&str]) -> Result, MaterialError> { if lines.is_empty() || lines.iter().all(|line| line.trim().is_empty()) { return Ok(Vec::new()); @@ -926,6 +939,24 @@ mod tests { assert_eq!(resolved.fallback, MaterialFallback::FirstEntry); } + #[test] + fn resolve_material_first_entry_uses_material_archive_not_wear_row_zero() { + let repo = material_repo(&[ + material_entry(b"MAT_ARCHIVE_FIRST", &mat0_with_texture(b"TEX_ARCHIVE")), + material_entry(b"MAT_WEAR_FIRST", &mat0_with_texture(b"TEX_WEAR")), + ]); + let table = decode_wear(b"2\n0 MAT_WEAR_FIRST\n1 MISSING\n").expect("wear"); + + let resolved = resolve_material(&repo, &table, 1).expect("resolved"); + + assert_eq!(resolved.name.0, b"MAT_ARCHIVE_FIRST"); + assert_eq!(resolved.fallback, MaterialFallback::FirstEntry); + assert_eq!( + resolved.document.primary_texture().expect("texture").0, + b"TEX_ARCHIVE" + ); + } + #[test] fn resolve_material_empty_texture_means_untextured() { let repo = material_repo(&[material_entry(b"MAT_EMPTY", &mat0_with_texture(b""))]); diff --git a/crates/fparkan-resource/src/lib.rs b/crates/fparkan-resource/src/lib.rs index 7dd90b5..05b022c 100644 --- a/crates/fparkan-resource/src/lib.rs +++ b/crates/fparkan-resource/src/lib.rs @@ -40,6 +40,8 @@ pub struct ArchiveId(pub u64); pub struct EntryHandle { /// Archive. pub archive: ArchiveId, + /// Archive generation at the time the entry was resolved. + pub generation: u64, /// Local entry index. pub local: u32, } @@ -108,6 +110,8 @@ pub enum ResourceError { MissingEntry, /// Stale or invalid handle. InvalidHandle, + /// Handle belongs to an older archive generation. + StaleHandle, /// Format error. Format(String), /// Entry-specific read error. @@ -148,6 +152,12 @@ pub trait ResourceRepository { archive: ArchiveId, name: &ResourceName, ) -> Result, ResourceError>; + /// Returns the first entry in archive directory order. + /// + /// # Errors + /// + /// Returns [`ResourceError`] when `archive` is not a valid opened archive. + fn first_entry(&self, archive: ArchiveId) -> Result, ResourceError>; /// Reads bytes. /// /// # Errors @@ -179,6 +189,7 @@ struct RepositoryState { struct ArchiveSlot { path: NormalizedPath, fingerprint: u64, + generation: u64, kind: ArchiveKind, document: ArchiveDocument, } @@ -250,12 +261,13 @@ impl ResourceRepository for CachedResourceRepository { } let bytes = self.vfs.read(path).map_err(resource_error_from_vfs)?; - let slot = decode_archive(path.clone(), bytes, fingerprint)?; + let mut slot = decode_archive(path.clone(), bytes, fingerprint)?; let mut state = self.state.lock().map_err(|_| ResourceError::Poisoned)?; if let Some(id) = state.paths.get(path.as_str()).copied() { if state.archive(id)?.fingerprint == fingerprint { return Ok(id); } + slot.generation = state.archive(id)?.generation.saturating_add(1); *state.archive_mut(id)? = slot; state.payload_cache.remove_archive(id); return Ok(id); @@ -279,7 +291,25 @@ impl ResourceRepository for CachedResourceRepository { ArchiveDocument::Nres(document) => document.find_bytes(&name.0).map(|id| id.0), ArchiveDocument::Rsli(document) => document.find_bytes(&name.0).map(|id| id.0), }; - Ok(local.map(|local| EntryHandle { archive, local })) + Ok(local.map(|local| EntryHandle { + archive, + generation: slot.generation, + local, + })) + } + + fn first_entry(&self, archive: ArchiveId) -> Result, ResourceError> { + let state = self.state.lock().map_err(|_| ResourceError::Poisoned)?; + let slot = state.archive(archive)?; + let local = match &slot.document { + ArchiveDocument::Nres(document) => document.entries().first().map(|entry| entry.id().0), + ArchiveDocument::Rsli(document) => document.entry(fparkan_rsli::EntryId(0)).map(|_| 0), + }; + Ok(local.map(|local| EntryHandle { + archive, + generation: slot.generation, + local, + })) } fn read(&self, entry: EntryHandle) -> Result { @@ -289,7 +319,7 @@ impl ResourceRepository for CachedResourceRepository { } let payload = { - let slot = state.archive(entry.archive)?; + let slot = state.entry_archive(entry)?; let key = slot.entry_key(entry.local)?; slot.read_payload(entry.local) .map_err(|source| ResourceError::EntryRead { @@ -304,7 +334,7 @@ impl ResourceRepository for CachedResourceRepository { fn entry_info(&self, entry: EntryHandle) -> Result { let state = self.state.lock().map_err(|_| ResourceError::Poisoned)?; - let slot = state.archive(entry.archive)?; + let slot = state.entry_archive(entry)?; match &slot.document { ArchiveDocument::Nres(document) => { let local = @@ -420,6 +450,14 @@ impl RepositoryState { .get_mut(index) .ok_or(ResourceError::InvalidHandle) } + + fn entry_archive(&self, entry: EntryHandle) -> Result<&ArchiveSlot, ResourceError> { + let slot = self.archive(entry.archive)?; + if slot.generation != entry.generation { + return Err(ResourceError::StaleHandle); + } + Ok(slot) + } } impl ArchiveSlot { @@ -474,6 +512,7 @@ fn decode_archive( return Ok(ArchiveSlot { path, fingerprint, + generation: 0, kind: ArchiveKind::Nres, document: ArchiveDocument::Nres(document), }); @@ -484,6 +523,7 @@ fn decode_archive( return Ok(ArchiveSlot { path, fingerprint, + generation: 0, kind: ArchiveKind::Rsli, document: ArchiveDocument::Rsli(document), }); @@ -554,6 +594,7 @@ mod tests { assert!(matches!( repo.read(EntryHandle { archive: ArchiveId(99), + generation: 0, local: 0 }), Err(ResourceError::InvalidHandle) @@ -661,6 +702,8 @@ mod tests { .expect("updated handle"); assert_eq!(reopened, archive); + assert_ne!(first, second); + assert!(matches!(repo.read(first), Err(ResourceError::StaleHandle))); assert_eq!( repo.read(second).expect("read updated").as_slice(), b"after" diff --git a/crates/fparkan-world/src/lib.rs b/crates/fparkan-world/src/lib.rs index 58412d9..a253586 100644 --- a/crates/fparkan-world/src/lib.rs +++ b/crates/fparkan-world/src/lib.rs @@ -357,36 +357,45 @@ pub fn step_with_handler( where F: FnMut(&mut World, &WorldCommand) -> Result<(), WorldError>, { + let before = world.clone(); world.phase = WorldPhase::Calculating; let mut events = Vec::new(); - while let Some(command) = world.queue.pop_front() { - if let Some(handle) = command.target { - if world.deferred_delete.contains(&handle) { - continue; + let result = (|| { + while let Some(command) = world.queue.pop_front() { + if let Some(handle) = command.target { + if world.deferred_delete.contains(&handle) { + continue; + } + checked_slot(world, handle)?; } - checked_slot(world, handle)?; + handler(world, &command)?; + events.push(WorldEvent { + sequence: command.sequence, + target: command.target, + }); } - handler(world, &command)?; - events.push(WorldEvent { - sequence: command.sequence, - target: command.target, - }); - } - world.phase = WorldPhase::ApplyingDeferred; - let deletes = std::mem::take(&mut world.deferred_delete); - for handle in deletes { - let _ = delete_now(world, handle); + world.phase = WorldPhase::ApplyingDeferred; + let deletes = std::mem::take(&mut world.deferred_delete); + for handle in deletes { + delete_now(world, handle)?; + } + world.tick.0 = world.tick.0.saturating_add(1); + world.phase = WorldPhase::PublishingSnapshot; + let snapshot = WorldSnapshot { + tick: world.tick, + objects: live_registered(world), + events, + hash: canonical_state_hash(world), + }; + world.phase = WorldPhase::Idle; + Ok(snapshot) + })(); + if let Err(err) = result { + *world = before; + world.phase = WorldPhase::Idle; + return Err(err); } - world.tick.0 = world.tick.0.saturating_add(1); - world.phase = WorldPhase::PublishingSnapshot; - let snapshot = WorldSnapshot { - tick: world.tick, - objects: live_registered(world), - events, - hash: canonical_state_hash(world), - }; - world.phase = WorldPhase::Idle; - Ok(snapshot) + result } /// Computes canonical state hash. @@ -710,6 +719,44 @@ mod tests { ); } + #[test] + fn callback_error_rolls_back_phase_queue_and_deferred_deletes() { + let mut world = new(WorldConfig); + let first = construct_object(&mut world, ObjectDraft { original_id: None }).expect("first"); + register_object(&mut world, first).expect("register"); + enqueue( + &mut world, + WorldCommand { + sequence: 7, + target: Some(first), + }, + ) + .expect("enqueue"); + + let err = step_with_handler(&mut world, &InputSnapshot, |world, _| { + request_delete(world, first)?; + Err(WorldError::InvalidFixedStep) + }) + .expect_err("handler error"); + + assert_eq!(err, WorldError::InvalidFixedStep); + assert_eq!(world.phase, WorldPhase::Idle); + assert_eq!(world.tick, Tick(0)); + assert!(world.deferred_delete.is_empty()); + assert_eq!(world.queue.len(), 1); + + let snapshot = step(&mut world, &InputSnapshot).expect("retry step"); + assert_eq!(snapshot.tick, Tick(1)); + assert_eq!( + snapshot.events, + vec![WorldEvent { + sequence: 0, + target: Some(first) + }] + ); + assert_eq!(snapshot.objects, vec![first]); + } + #[test] fn snapshot_hash_determinism_and_immutability() { let mut left = new(WorldConfig); -- cgit v1.2.3