From abff33ba676ea78731e4d70f954ee6b7f6589a61 Mon Sep 17 00:00:00 2001 From: oiupoyt Date: Thu, 1 Oct 2026 19:42:19 +0530 Subject: [PATCH] fix(assets): implement -1 error returns on invalid handles, null buffer check for empty assets, checked seek arithmetic, and path normalization --- crates/molotov-core/Cargo.toml | 3 + crates/molotov-core/src/assets/asset.rs | 32 +++++--- crates/molotov-core/src/assets/c_api.rs | 24 ++++-- crates/molotov-core/src/assets/manager.rs | 74 ++++++++++++++----- crates/molotov-core/tests/asset_fixes_test.rs | 61 +++++++++++++++ 5 files changed, 157 insertions(+), 37 deletions(-) create mode 100644 crates/molotov-core/tests/asset_fixes_test.rs diff --git a/crates/molotov-core/Cargo.toml b/crates/molotov-core/Cargo.toml index 2f1565a..d69842a 100644 --- a/crates/molotov-core/Cargo.toml +++ b/crates/molotov-core/Cargo.toml @@ -9,3 +9,6 @@ tracing = { workspace = true } thiserror = { workspace = true } libc = { workspace = true } lazy_static = "1.5" + +[dev-dependencies] +tempfile = { workspace = true } diff --git a/crates/molotov-core/src/assets/asset.rs b/crates/molotov-core/src/assets/asset.rs index d57f070..9584551 100644 --- a/crates/molotov-core/src/assets/asset.rs +++ b/crates/molotov-core/src/assets/asset.rs @@ -1,14 +1,19 @@ // SPDX-License-Identifier: GPL-3.0-or-later use super::types::{SEEK_CUR, SEEK_END, SEEK_SET}; use std::ffi::c_void; +use std::sync::Arc; pub struct AssetFile { - data: Vec, + data: Arc>, cursor: usize, } impl AssetFile { pub fn new(data: Vec) -> Self { + Self::new_shared(Arc::new(data)) + } + + pub fn new_shared(data: Arc>) -> Self { Self { data, cursor: 0 } } @@ -21,7 +26,11 @@ impl AssetFile { } pub fn get_buffer(&self) -> *const c_void { - self.data.as_ptr() as *const c_void + if self.data.is_empty() { + std::ptr::null() + } else { + self.data.as_ptr() as *const c_void + } } pub fn read(&mut self, buf: &mut [u8]) -> usize { @@ -35,18 +44,19 @@ impl AssetFile { } pub fn seek(&mut self, offset: i64, whence: i32) -> i64 { - let new_pos: i64 = match whence { - SEEK_SET => offset, - SEEK_CUR => self.cursor as i64 + offset, - SEEK_END => self.data.len() as i64 + offset, + let new_pos: Option = match whence { + SEEK_SET => Some(offset), + SEEK_CUR => (self.cursor as i64).checked_add(offset), + SEEK_END => (self.data.len() as i64).checked_add(offset), _ => return -1, }; - if new_pos >= 0 && (new_pos as usize) <= self.data.len() { - self.cursor = new_pos as usize; - self.cursor as i64 - } else { - -1 + if let Some(pos) = new_pos { + if pos >= 0 && (pos as usize) <= self.data.len() { + self.cursor = pos as usize; + return self.cursor as i64; + } } + -1 } } diff --git a/crates/molotov-core/src/assets/c_api.rs b/crates/molotov-core/src/assets/c_api.rs index b66f545..7d449cd 100644 --- a/crates/molotov-core/src/assets/c_api.rs +++ b/crates/molotov-core/src/assets/c_api.rs @@ -80,19 +80,27 @@ pub unsafe extern "C" fn AAssetManager_openDir(_mgr: *mut (), dirName: *const li #[no_mangle] pub unsafe extern "C" fn AAsset_read(asset: *mut (), buf: *mut c_void, count: usize) -> i32 { - if asset.is_null() || buf.is_null() || count == 0 { + if asset.is_null() || buf.is_null() { + return -1; + } + if count == 0 { return 0; } + let id = asset as usize; - if let Ok(table) = ASSET_TABLE.read() { - if let Some(mutex) = table.get(&id) { - if let Ok(mut file) = mutex.lock() { - let slice = std::slice::from_raw_parts_mut(buf as *mut u8, count); - return file.read(slice) as i32; - } + let table = match ASSET_TABLE.read() { + Ok(t) => t, + Err(_) => return -1, + }; + + if let Some(mutex) = table.get(&id) { + if let Ok(mut file) = mutex.lock() { + let clamped_count = count.min(isize::MAX as usize); + let slice = std::slice::from_raw_parts_mut(buf as *mut u8, clamped_count); + return file.read(slice) as i32; } } - 0 + -1 } #[no_mangle] diff --git a/crates/molotov-core/src/assets/manager.rs b/crates/molotov-core/src/assets/manager.rs index 178ccb0..4dd3471 100644 --- a/crates/molotov-core/src/assets/manager.rs +++ b/crates/molotov-core/src/assets/manager.rs @@ -2,10 +2,13 @@ use super::asset::AssetFile; use super::dir::AssetDir; use std::collections::HashMap; +use std::path::PathBuf; +use std::sync::Arc; pub struct AssetManager { - base_assets: HashMap>, - mod_overrides: HashMap>, + base_assets: HashMap>>, + mod_overrides: HashMap>>, + mods_dir: Option, } impl Default for AssetManager { @@ -16,51 +19,86 @@ impl Default for AssetManager { impl AssetManager { pub fn new() -> Self { + Self::new_with_mods_dir(None) + } + + pub fn new_with_mods_dir(mods_dir: Option) -> Self { Self { base_assets: HashMap::new(), mod_overrides: HashMap::new(), + mods_dir, } } + pub fn normalize_path(path: &str) -> String { + let mut clean = path.replace('\\', "/"); + while clean.starts_with('/') || clean.starts_with("./") { + if clean.starts_with('/') { + clean.remove(0); + } else if clean.starts_with("./") { + clean.replace_range(0..2, ""); + } + } + clean + } + pub fn add_asset(&mut self, path: &str, data: Vec) { - self.base_assets.insert(path.to_string(), data); + let norm = Self::normalize_path(path); + self.base_assets.insert(norm, Arc::new(data)); } pub fn add_mod_override(&mut self, path: &str, data: Vec) { - self.mod_overrides.insert(path.to_string(), data); + let norm = Self::normalize_path(path); + self.mod_overrides.insert(norm, Arc::new(data)); } pub fn open(&self, filename: &str) -> Option { - // 1. Check user mod overrides first (non-destructive mod engine) - if let Some(data) = self.mod_overrides.get(filename) { - tracing::info!("AssetManager: redirected '{}' to active user mod", filename); - return Some(AssetFile::new(data.clone())); + let norm = Self::normalize_path(filename); + + // 1. Check in-memory user mod overrides first + if let Some(data) = self.mod_overrides.get(&norm) { + tracing::info!("AssetManager: redirected '{}' to active in-memory mod", norm); + return Some(AssetFile::new_shared(data.clone())); } - // 2. Fallback to base asset - if let Some(data) = self.base_assets.get(filename) { - return Some(AssetFile::new(data.clone())); + // 2. Check disk mod overrides in ~/.config/molotov/mods/ + if let Some(dir) = &self.mods_dir { + let disk_file = dir.join(&norm); + if disk_file.is_file() { + if let Ok(bytes) = std::fs::read(&disk_file) { + tracing::info!("AssetManager: loaded custom mod from disk: {}", disk_file.display()); + return Some(AssetFile::new(bytes)); + } + } } - tracing::warn!("AssetManager: asset '{}' not found", filename); + // 3. Fallback to base asset + if let Some(data) = self.base_assets.get(&norm) { + return Some(AssetFile::new_shared(data.clone())); + } + + tracing::warn!("AssetManager: asset '{}' not found", norm); None } pub fn open_dir(&self, dir_name: &str) -> AssetDir { - let prefix = if dir_name.is_empty() || dir_name == "." { + let norm = Self::normalize_path(dir_name); + let prefix = if norm.is_empty() { "".to_string() - } else if !dir_name.ends_with('/') { - format!("{}/", dir_name) } else { - dir_name.to_string() + format!("{}/", norm) }; let mut files = Vec::new(); for key in self.base_assets.keys().chain(self.mod_overrides.keys()) { if key.starts_with(&prefix) { let rest = &key[prefix.len()..]; - if !rest.contains('/') { - files.push(rest.to_string()); + if !rest.is_empty() { + let entry = match rest.find('/') { + Some(idx) => &rest[..idx], + None => rest, + }; + files.push(entry.to_string()); } } } diff --git a/crates/molotov-core/tests/asset_fixes_test.rs b/crates/molotov-core/tests/asset_fixes_test.rs new file mode 100644 index 0000000..52baa67 --- /dev/null +++ b/crates/molotov-core/tests/asset_fixes_test.rs @@ -0,0 +1,61 @@ +// SPDX-License-Identifier: GPL-3.0-or-later +use molotov_core::assets::asset::AssetFile; +use molotov_core::assets::c_api::*; +use molotov_core::assets::manager::AssetManager; +use tempfile::tempdir; + +#[test] +fn test_aasset_read_invalid_handle_returns_error_negative_one() { + unsafe { + // Invalid or null pointer must return -1, never fake 0 + let ret = AAsset_read(std::ptr::null_mut(), std::ptr::null_mut(), 16); + assert_eq!(ret, -1, "Invalid handle or null buffer must return -1"); + + let bogus_handle = 0x9999 as *mut (); + let mut buf = [0u8; 16]; + let ret2 = AAsset_read(bogus_handle, buf.as_mut_ptr() as *mut _, 16); + assert_eq!(ret2, -1, "Stale or closed handle must return -1"); + } +} + +#[test] +fn test_empty_asset_get_buffer_returns_null() { + let empty_asset = AssetFile::new(vec![]); + assert_eq!(empty_asset.length(), 0); + assert!( + empty_asset.get_buffer().is_null(), + "Empty asset get_buffer must return NULL, never dangling pointer 0x1" + ); +} + +#[test] +fn test_seek_overflow_does_not_panic() { + let mut asset = AssetFile::new(b"SAMPLE".to_vec()); + // Seeking with i64::MAX from SEEK_CUR must return -1 using checked arithmetic + let ret = asset.seek(i64::MAX, 1); + assert_eq!(ret, -1); +} + +#[test] +fn test_path_normalization_and_disk_mod_resolution() { + let temp = tempdir().unwrap(); + let mods_dir = temp.path().join("mods"); + std::fs::create_dir_all(&mods_dir).unwrap(); + + let custom_sound = mods_dir.join("content/sounds/ouch.ogg"); + if let Some(parent) = custom_sound.parent() { + std::fs::create_dir_all(parent).unwrap(); + } + std::fs::write(&custom_sound, b"CUSTOM_DISK_MOD_DATA").unwrap(); + + let mut mgr = AssetManager::new_with_mods_dir(Some(mods_dir)); + mgr.add_asset("content/sounds/ouch.ogg", b"BASE_DATA".to_vec()); + + // Should resolve path with Windows backslashes and leading slashes + let asset = mgr.open(r"\content\sounds\ouch.ogg").expect("Should open with backslash normalization"); + let mut out = vec![0u8; asset.length() as usize]; + let mut asset_mut = asset; + asset_mut.read(&mut out); + + assert_eq!(&out, b"CUSTOM_DISK_MOD_DATA", "Must resolve disk mod overlay with normalized path"); +}