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

@@ -9,3 +9,6 @@ tracing = { workspace = true }
 thiserror = { workspace = true }
 libc = { workspace = true }
 lazy_static = "1.5"
+
+[dev-dependencies]
+tempfile = { workspace = true }

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<u8>,
+    data: Arc<Vec<u8>>,
     cursor: usize,
 }
 
 impl AssetFile {
     pub fn new(data: Vec<u8>) -> Self {
+        Self::new_shared(Arc::new(data))
+    }
+
+    pub fn new_shared(data: Arc<Vec<u8>>) -> Self {
         Self { data, cursor: 0 }
     }
 
@@ -21,8 +26,12 @@ impl AssetFile {
     }
 
     pub fn get_buffer(&self) -> *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 {
         let available = self.data.len().saturating_sub(self.cursor);
@@ -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<i64> = 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 {
+        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
     }
 }
-}

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() {
+    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 slice = std::slice::from_raw_parts_mut(buf as *mut u8, count);
+            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]

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<String, Vec<u8>>,
-    mod_overrides: HashMap<String, Vec<u8>>,
+    base_assets: HashMap<String, Arc<Vec<u8>>>,
+    mod_overrides: HashMap<String, Arc<Vec<u8>>>,
+    mods_dir: Option<PathBuf>,
 }
 
 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<PathBuf>) -> 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<u8>) {
-        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<u8>) {
-        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<AssetFile> {
-        // 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());
                 }
             }
         }

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");
+}