feat: Update C API to consume Read+Seek callbacks (#111)

* Update C API to consume Read+Seek callbacks

* remove open and close functions

* update to reflect new interface

* Refactor posix file i/o to c_api (#113)

* Refactor posix file i/o to c_api

* fix comment

* rename ExternalFile to ReadSeek

* cleanup and comments

* Add SHOREBIRD_PATCH_BASE_FILENAME const

* fix lint

* add fake callbacks for c_api tests

* fix tests

* remove os_last_error

* remove todos, add comments

* cleanup

* remove params to open

* add c_api module, tests

* reorganize

* Return Err if CFileProvider open returns null

* add comments and docs

library/Cargo.toml

@@ -22,6 +22,7 @@ bipatch = "1.0.0"
 comde = { version = "0.2.3", default-features = false, features = [
     "zstandard",
 ] }
+dyn-clone = "1.0.16"
 # For decoding the hex-encoded hashes in Patch network responses.
 hex = "0.4.3"
 # Used to construct mock responses.

library/include/updater.h

@@ -47,6 +47,28 @@ typedef struct AppParameters {
   const char *code_cache_dir;
 } AppParameters;
 
+typedef struct FileCallbacks {
+  /**
+   * Opens the "file" (actually an in-memory buffer) and returns a handle.
+   */
+  void *(*open)(void);
+  /**
+   * Reads count bytes from the file into buffer.  Returns the number of
+   * bytes read.
+   */
+  uintptr_t (*read)(void *file_handle, uint8_t *buffer, uintptr_t count);
+  /**
+   * Moves the file pointer to the given offset relative from whence (one of
+   * libc::SEEK_SET, libc::SEEK_CUR, or libc::SEEK_END). Returns the new
+   * offset relative to the start of the file.
+   */
+  int64_t (*seek)(void *file_handle, int64_t offset, int32_t whence);
+  /**
+   * Closes and frees the file handle.
+   */
+  void (*close)(void *file_handle);
+} FileCallbacks;
+
 #ifdef __cplusplus
 extern "C" {
 #endif // __cplusplus
@@ -59,6 +81,7 @@ extern "C" {
  */
 SHOREBIRD_EXPORT
 bool shorebird_init(const struct AppParameters *c_params,
+                    struct FileCallbacks c_file_callbacks,
                     const char *c_yaml);
 
 /**

library/src/c_api/c_file.rs

@@ -0,0 +1,250 @@
+use std::io::{Read, Seek};
+
+use crate::{ExternalFileProvider, ReadSeek};
+
+use super::FileCallbacks;
+
+struct CFile {
+    file_callbacks: FileCallbacks,
+    handle: *mut libc::c_void,
+}
+
+#[derive(Clone, Debug)]
+pub struct CFileProvder {
+    pub file_callbacks: FileCallbacks,
+}
+
+impl ExternalFileProvider for CFileProvder {
+    fn open(&self) -> anyhow::Result<Box<dyn ReadSeek>> {
+        let handle = (self.file_callbacks.open)();
+        if handle.is_null() {
+            return Err(anyhow::anyhow!("CFile open failed"));
+        }
+        let file = CFile {
+            file_callbacks: self.file_callbacks,
+            handle,
+        };
+        Ok(Box::new(file))
+    }
+}
+
+impl ReadSeek for CFile {}
+
+impl Drop for CFile {
+    fn drop(&mut self) {
+        (self.file_callbacks.close)(self.handle);
+    }
+}
+
+impl Read for CFile {
+    fn read(&mut self, buf: &mut [u8]) -> std::io::Result<usize> {
+        Ok((self.file_callbacks.read)(
+            self.handle,
+            buf.as_mut_ptr(),
+            buf.len(),
+        ))
+    }
+}
+
+impl Seek for CFile {
+    fn seek(&mut self, pos: std::io::SeekFrom) -> std::io::Result<u64> {
+        let (offset, whence) = match pos {
+            std::io::SeekFrom::Start(offset) => (offset as i64, libc::SEEK_SET),
+            std::io::SeekFrom::End(offset) => (offset, libc::SEEK_END),
+            std::io::SeekFrom::Current(offset) => (offset, libc::SEEK_CUR),
+        };
+        let result = (self.file_callbacks.seek)(self.handle, offset, whence);
+        if result < 0 {
+            Err(std::io::Error::new(
+                std::io::ErrorKind::Other,
+                format!("CFile seek failed with error code: {}", result),
+            ))
+        } else {
+            Ok(result as u64)
+        }
+    }
+}
+
+#[cfg(test)]
+mod test {
+    use serial_test::serial;
+
+    use super::*;
+
+    static OPEN_RET_VAL: u32 = 42;
+
+    static mut OPEN_CALL_COUNT: usize = 0;
+    static mut CLOSE_CALL_COUNT: usize = 0;
+    static mut OPEN_RET: *mut libc::c_void = OPEN_RET_VAL as *mut libc::c_void;
+    static mut READ_ARGS: Vec<(*mut libc::c_void, *mut u8, usize)> = Vec::new();
+    static mut SEEK_ARGS: Vec<(*mut libc::c_void, i64, i32)> = Vec::new();
+    static mut SEEK_RET: i64 = 0;
+
+    fn reset_tests() {
+        unsafe {
+            OPEN_RET = OPEN_RET_VAL as *mut libc::c_void;
+            OPEN_CALL_COUNT = 0;
+            CLOSE_CALL_COUNT = 0;
+            READ_ARGS.clear();
+            SEEK_ARGS.clear();
+        }
+    }
+
+    extern "C" fn fake_open() -> *mut libc::c_void {
+        unsafe {
+            OPEN_CALL_COUNT += 1;
+            OPEN_RET
+        }
+    }
+
+    extern "C" fn fake_read(_handle: *mut libc::c_void, _buffer: *mut u8, _length: usize) -> usize {
+        unsafe {
+            READ_ARGS.push((_handle, _buffer, _length));
+        }
+        0
+    }
+
+    extern "C" fn fake_seek(_handle: *mut libc::c_void, _offset: i64, _seek_from: i32) -> i64 {
+        unsafe {
+            SEEK_ARGS.push((_handle, _offset, _seek_from));
+            SEEK_RET
+        }
+    }
+
+    extern "C" fn fake_close(_handle: *mut libc::c_void) {
+        unsafe {
+            CLOSE_CALL_COUNT += 1;
+        }
+    }
+
+    impl FileCallbacks {
+        pub fn new() -> Self {
+            Self {
+                open: fake_open,
+                read: fake_read,
+                seek: fake_seek,
+                close: fake_close,
+            }
+        }
+    }
+
+    impl Default for FileCallbacks {
+        fn default() -> Self {
+            Self::new()
+        }
+    }
+
+    #[serial]
+    #[test]
+    fn test_open() {
+        reset_tests();
+
+        let file_provider = CFileProvder {
+            file_callbacks: FileCallbacks::new(),
+        };
+        let handle = file_provider.open().unwrap();
+        drop(handle);
+        unsafe {
+            assert_eq!(OPEN_CALL_COUNT, 1);
+            assert_eq!(CLOSE_CALL_COUNT, 1);
+        }
+    }
+
+    #[serial]
+    #[test]
+    fn test_open_failure() {
+        reset_tests();
+        unsafe {
+            OPEN_RET = std::ptr::null_mut();
+        }
+
+        let file_provider = CFileProvder {
+            file_callbacks: FileCallbacks::new(),
+        };
+        let result = file_provider.open();
+        assert!(result.is_err());
+    }
+
+    #[serial]
+    #[test]
+    fn test_read() {
+        reset_tests();
+
+        let file_provider = CFileProvder {
+            file_callbacks: FileCallbacks::new(),
+        };
+        let mut handle = file_provider.open().unwrap();
+        let mut buffer = [0u8; 10];
+        let _read = handle.read(&mut buffer).unwrap();
+        unsafe {
+            assert_eq!(READ_ARGS.len(), 1);
+            assert_eq!(READ_ARGS[0].2, 10);
+        }
+    }
+
+    #[serial]
+    #[test]
+    fn test_seek() {
+        reset_tests();
+
+        let file_provider = CFileProvder {
+            file_callbacks: FileCallbacks::new(),
+        };
+        let mut handle = file_provider.open().unwrap();
+        unsafe {
+            SEEK_RET = 1;
+        }
+        let result = handle.seek(std::io::SeekFrom::Start(10));
+        unsafe {
+            assert!(result.is_ok());
+            assert_eq!(result.unwrap(), 1);
+            assert_eq!(SEEK_ARGS.len(), 1);
+            assert_eq!(SEEK_ARGS.last().unwrap().1, 10);
+            assert_eq!(SEEK_ARGS.last().unwrap().2, libc::SEEK_SET);
+        }
+
+        unsafe {
+            SEEK_RET = 2;
+        }
+        let result = handle.seek(std::io::SeekFrom::Current(5));
+        unsafe {
+            assert!(result.is_ok());
+            assert_eq!(result.unwrap(), 2);
+            assert_eq!(SEEK_ARGS.len(), 2);
+            assert_eq!(SEEK_ARGS.last().unwrap().1, 5);
+            assert_eq!(SEEK_ARGS.last().unwrap().2, libc::SEEK_CUR);
+        }
+
+        unsafe {
+            SEEK_RET = 3;
+        }
+        let result = handle.seek(std::io::SeekFrom::End(1));
+        unsafe {
+            assert!(result.is_ok());
+            assert_eq!(result.unwrap(), 3);
+            assert_eq!(SEEK_ARGS.len(), 3);
+            assert_eq!(SEEK_ARGS.last().unwrap().1, 1);
+            assert_eq!(SEEK_ARGS.last().unwrap().2, libc::SEEK_END);
+        }
+    }
+
+    #[serial]
+    #[test]
+    fn test_seek_err() {
+        reset_tests();
+
+        let file_provider = CFileProvder {
+            file_callbacks: FileCallbacks::new(),
+        };
+        let mut handle = file_provider.open().unwrap();
+        unsafe {
+            SEEK_RET = -1;
+        }
+        let result = handle.seek(std::io::SeekFrom::Start(10));
+        assert!(result.is_err());
+        assert!(result
+            .unwrap_err()
+            .to_string()
+            .contains("CFile seek failed with error code: -1"));
+    }
+}

library/src/c_api/mod.rs

@@ -20,6 +20,10 @@ use crate::updater;
 #[cfg(test)]
 use std::{println as info, println as error}; // Workaround to use println! for logs.
 
+use self::c_file::CFileProvder;
+
+mod c_file;
+
 /// Struct containing configuration parameters for the updater.
 /// Passed to all updater functions.
 /// NOTE: If this struct is changed all language bindings must be updated.
@@ -46,6 +50,25 @@ pub struct AppParameters {
     pub code_cache_dir: *const libc::c_char,
 }
 
+#[derive(Clone, Copy, Debug)]
+#[repr(C)]
+pub struct FileCallbacks {
+    /// Opens the "file" (actually an in-memory buffer) and returns a handle.
+    pub open: extern "C" fn() -> *mut libc::c_void,
+
+    /// Reads count bytes from the file into buffer.  Returns the number of
+    /// bytes read.
+    pub read: extern "C" fn(file_handle: *mut libc::c_void, buffer: *mut u8, count: usize) -> usize,
+
+    /// Moves the file pointer to the given offset relative from whence (one of
+    /// libc::SEEK_SET, libc::SEEK_CUR, or libc::SEEK_END). Returns the new
+    /// offset relative to the start of the file.
+    pub seek: extern "C" fn(file_handle: *mut libc::c_void, offset: i64, whence: i32) -> i64,
+
+    /// Closes and frees the file handle.
+    pub close: extern "C" fn(file_handle: *mut libc::c_void),
+}
+
 /// Converts a C string to a Rust string, does not free the C string.
 fn to_rust(c_string: *const libc::c_char) -> anyhow::Result<String> {
     anyhow::ensure!(!c_string.is_null(), "Null string passed to to_rust");
@@ -107,13 +130,17 @@ where
 #[no_mangle]
 pub extern "C" fn shorebird_init(
     c_params: *const AppParameters,
+    c_file_callbacks: FileCallbacks,
     c_yaml: *const libc::c_char,
 ) -> bool {
     log_on_error(
         || {
             let config = app_config_from_c(c_params)?;
+            let file_provider = Box::new(CFileProvder {
+                file_callbacks: c_file_callbacks,
+            });
             let yaml_string = to_rust(c_yaml)?;
-            updater::init(config, &yaml_string)?;
+            updater::init(config, file_provider, &yaml_string)?;
             Ok(true)
         },
         "initializing updater",
@@ -327,7 +354,11 @@ mod test {
     fn init_with_nulls() {
         testing_reset_config();
         // Should log but not crash.
-        assert!(!shorebird_init(std::ptr::null(), std::ptr::null()));
+        assert!(!shorebird_init(
+            std::ptr::null(),
+            FileCallbacks::new(),
+            std::ptr::null()
+        ));
 
         // free_string also doesn't crash with null.
         unsafe { shorebird_free_string(std::ptr::null_mut()) }
@@ -345,7 +376,11 @@ mod test {
             original_libapp_paths: std::ptr::null(),
             original_libapp_paths_size: 0,
         };
-        assert!(!shorebird_init(&c_params, std::ptr::null()));
+        assert!(!shorebird_init(
+            &c_params,
+            FileCallbacks::new(),
+            std::ptr::null()
+        ));
     }
 
     #[serial]
@@ -355,7 +390,7 @@ mod test {
         let tmp_dir = TempDir::new("example").unwrap();
         let c_params = parameters(&tmp_dir, "/dir/lib/arm64/libapp.so");
         let c_yaml = c_string("bad yaml");
-        assert!(!shorebird_init(&c_params, c_yaml));
+        assert!(!shorebird_init(&c_params, FileCallbacks::new(), c_yaml));
         free_c_string(c_yaml);
         free_parameters(c_params);
     }
@@ -373,7 +408,7 @@ mod test {
         base_url: baz
         auto_update: false",
         );
-        assert!(shorebird_init(&c_params, c_yaml));
+        assert!(shorebird_init(&c_params, FileCallbacks::new(), c_yaml));
         free_c_string(c_yaml);
         free_parameters(c_params);
         assert!(!shorebird_should_auto_update());
@@ -387,7 +422,7 @@ mod test {
         let c_params = parameters(&tmp_dir, "/dir/lib/arm64/libapp.so");
         // app_id is required or shorebird_init will fail.
         let c_yaml = c_string("app_id: foo");
-        assert!(shorebird_init(&c_params, c_yaml));
+        assert!(shorebird_init(&c_params, FileCallbacks::new(), c_yaml));
         free_c_string(c_yaml);
         free_parameters(c_params);
 
@@ -430,7 +465,7 @@ mod test {
         let c_params = parameters(&tmp_dir, fake_libapp_path.to_str().unwrap());
         // app_id is required or shorebird_init will fail.
         let c_yaml = c_string("app_id: foo");
-        assert!(shorebird_init(&c_params, c_yaml));
+        assert!(shorebird_init(&c_params, FileCallbacks::new(), c_yaml));
         free_c_string(c_yaml);
         free_parameters(c_params);
 
@@ -498,7 +533,7 @@ mod test {
         let c_params = parameters(&tmp_dir, fake_libapp_path.to_str().unwrap());
         // app_id is required or shorebird_init will fail.
         let c_yaml = c_string("app_id: foo");
-        assert!(shorebird_init(&c_params, c_yaml));
+        assert!(shorebird_init(&c_params, FileCallbacks::new(), c_yaml));
         free_c_string(c_yaml);
         free_parameters(c_params);
 
@@ -506,7 +541,7 @@ mod test {
         let c_params = parameters(&tmp_dir, fake_libapp_path.to_str().unwrap());
         // app_id is required or shorebird_init will fail.
         let c_yaml = c_string("app_id: bar");
-        assert!(!shorebird_init(&c_params, c_yaml));
+        assert!(!shorebird_init(&c_params, FileCallbacks::new(), c_yaml));
         free_c_string(c_yaml);
         free_parameters(c_params);
     }
@@ -525,7 +560,7 @@ mod test {
         let c_params = parameters(&tmp_dir, fake_libapp_path.to_str().unwrap());
         // app_id is required or shorebird_init will fail.
         let c_yaml = c_string("app_id: foo");
-        assert!(shorebird_init(&c_params, c_yaml));
+        assert!(shorebird_init(&c_params, FileCallbacks::new(), c_yaml));
         free_c_string(c_yaml);
         free_parameters(c_params);
 

library/src/config.rs

@@ -3,7 +3,7 @@ use crate::network::NetworkHooks;
 
 use crate::updater::AppConfig;
 use crate::yaml::YamlConfig;
-use crate::UpdateError;
+use crate::{ExternalFileProvider, UpdateError};
 use std::path::PathBuf;
 
 use once_cell::sync::OnceCell;
@@ -86,10 +86,12 @@ pub struct UpdateConfig {
     pub libapp_path: PathBuf,
     pub base_url: String,
     pub network_hooks: NetworkHooks,
+    pub file_provider: Box<dyn ExternalFileProvider>,
 }
 
 pub fn set_config(
     app_config: AppConfig,
+    file_provider: Box<dyn ExternalFileProvider>,
     libapp_path: PathBuf,
     yaml: &YamlConfig,
     network_hooks: NetworkHooks,
@@ -119,6 +121,7 @@ pub fn set_config(
                 .unwrap_or(DEFAULT_BASE_URL)
                 .to_owned(),
             network_hooks,
+            file_provider,
         };
         debug!("Updater configured with: {:?}", new_config);
         *config = Some(new_config);

library/src/updater.rs

@@ -1,13 +1,13 @@
 // This file's job is to be the Rust API for the updater.
 
-use std::fmt::{Display, Formatter};
-use std::fs;
-#[cfg(any(target_os = "android", test))]
-use std::io::{Read, Seek};
+use std::fmt::{Debug, Display, Formatter};
+use std::fs::{self};
+use std::io::{Cursor, Read, Seek};
 use std::path::{Path, PathBuf};
 
 use anyhow::bail;
 use anyhow::Context;
+use dyn_clone::DynClone;
 
 use crate::cache::{PatchInfo, UpdaterState};
 use crate::config::{current_arch, current_platform, set_config, with_config, UpdateConfig};
@@ -84,6 +84,18 @@ pub struct AppConfig {
     pub original_libapp_paths: Vec<String>,
 }
 
+pub trait ReadSeek: Read + Seek {}
+
+/// Provides an interface to get an opaque ReadSeek object for a given path.
+/// This is used to provide a way to read the patch base file on iOS.
+pub trait ExternalFileProvider: Debug + Send + DynClone {
+    fn open(&self) -> anyhow::Result<Box<dyn ReadSeek>>;
+}
+
+// This is required for ExternalFileProvider to be used as a field in the Clone-able
+// UpdateConfig struct.
+dyn_clone::clone_trait_object!(ExternalFileProvider);
+
 // On Android we don't use a direct path to libapp.so, but rather a data dir
 // and a hard-coded name for the libapp file which we look up in the
 // split APKs in that datadir. On other platforms we just use a path.
@@ -103,7 +115,11 @@ fn libapp_path_from_settings(original_libapp_paths: &[String]) -> Result<PathBuf
 /// The yaml string is the contents of the `shorebird.yaml` file.
 /// The `AppConfig` struct is information about the running app and where
 /// the updater should keep its cache.
-pub fn init(app_config: AppConfig, yaml: &str) -> Result<(), UpdateError> {
+pub fn init(
+    app_config: AppConfig,
+    file_provider: Box<dyn ExternalFileProvider>,
+    yaml: &str,
+) -> Result<(), UpdateError> {
     #[cfg(any(target_os = "android", test))]
     use crate::android::libapp_path_from_settings;
 
@@ -113,8 +129,14 @@ pub fn init(app_config: AppConfig, yaml: &str) -> Result<(), UpdateError> {
 
     let libapp_path = libapp_path_from_settings(&app_config.original_libapp_paths)?;
     debug!("libapp_path: {:?}", libapp_path);
-    set_config(app_config, libapp_path, &config, NetworkHooks::default())
-        .map_err(|err| UpdateError::InvalidState(err.to_string()))
+    set_config(
+        app_config,
+        file_provider,
+        libapp_path,
+        &config,
+        NetworkHooks::default(),
+    )
+    .map_err(|err| UpdateError::InvalidState(err.to_string()))
 }
 
 pub fn should_auto_update() -> anyhow::Result<bool> {
@@ -169,32 +191,17 @@ fn check_hash(path: &Path, expected_string: &str) -> anyhow::Result<()> {
     Ok(())
 }
 
-// This is just a place to put our terrible android hacks.
-// And also avoid (for now) dealing with inflating patches on iOS.
+impl ReadSeek for Cursor<Vec<u8>> {}
+
 #[cfg(any(target_os = "android", test))]
-fn prepare_for_install(
-    config: &UpdateConfig,
-    download_path: &Path,
-    output_path: &Path,
-) -> anyhow::Result<()> {
-    // We abuse `libapp_path` to actually be the path to the data dir for now.
-    // This is an abuse because the variable name is `libapp_path`, but
-    // we're making it point to a the `app_data` directory instead.
-    let app_dir = &config.libapp_path;
-    debug!("app_dir: {:?}", app_dir);
-    let base_r = crate::android::open_base_lib(app_dir, "libapp.so")?;
-    inflate(download_path, base_r, output_path)
+fn patch_base(config: &UpdateConfig) -> anyhow::Result<Box<dyn ReadSeek>> {
+    let base_r = crate::android::open_base_lib(&config.libapp_path, "libapp.so")?;
+    Ok(Box::new(base_r))
 }
 
 #[cfg(not(any(target_os = "android", test)))]
-fn prepare_for_install(
-    _config: &UpdateConfig,
-    download_path: &Path,
-    output_path: &Path,
-) -> anyhow::Result<()> {
-    // On iOS we don't yet support compressed patches, just copy the file.
-    fs::copy(download_path, output_path)?;
-    Ok(())
+fn patch_base(config: &UpdateConfig) -> anyhow::Result<Box<dyn ReadSeek>> {
+    config.file_provider.open()
 }
 
 fn copy_update_config() -> anyhow::Result<UpdateConfig> {
@@ -265,8 +272,8 @@ fn update_internal(_: &UpdaterLockState) -> anyhow::Result<UpdateStatus> {
     download_to_path(&config.network_hooks, &patch.download_url, &download_path)?;
 
     let output_path = download_dir.join(format!("{}.full", patch.number));
-    // Should not pass config, rather should read necessary information earlier.
-    prepare_for_install(&config, &download_path, &output_path)?;
+    let patch_base_rs = patch_base(&config)?;
+    inflate(&download_path, patch_base_rs, &output_path)?;
 
     // Check the hash before moving into place.
     check_hash(&output_path, &patch.hash).with_context(|| {
@@ -304,7 +311,6 @@ pub fn update() -> anyhow::Result<UpdateStatus> {
 
 /// Given a path to a patch file, and a base file, apply the patch to the base
 /// and write the result to the output path.
-#[cfg(any(target_os = "android", test))]
 fn inflate<RS>(patch_path: &Path, base_r: RS, output_path: &Path) -> anyhow::Result<()>
 where
     RS: Read + Seek,
@@ -488,7 +494,15 @@ mod tests {
     use std::fs;
     use tempdir::TempDir;
 
-    use crate::config::testing_reset_config;
+    use crate::{config::testing_reset_config, ExternalFileProvider};
+
+    #[derive(Debug, Clone)]
+    struct FakeExternalFileProvider {}
+    impl ExternalFileProvider for FakeExternalFileProvider {
+        fn open(&self) -> anyhow::Result<Box<dyn crate::ReadSeek>> {
+            Ok(Box::new(std::io::Cursor::new(vec![])))
+        }
+    }
 
     fn init_for_testing(tmp_dir: &TempDir, base_url: Option<&str>) {
         testing_reset_config();
@@ -505,6 +519,7 @@ mod tests {
                 release_version: "1.0.0+1".to_string(),
                 original_libapp_paths: vec!["/dir/lib/arch/libapp.so".to_string()],
             },
+            Box::new(FakeExternalFileProvider {}),
             &yaml,
         )
         .unwrap();
@@ -601,6 +616,7 @@ mod tests {
                     release_version: "1.0.0+1".to_string(),
                     original_libapp_paths: vec!["original_libapp_path".to_string()],
                 },
+                Box::new(FakeExternalFileProvider {}),
                 "",
             ),
             Err(crate::UpdateError::InvalidArgument(