Merge pull request #1358 from hannobraun/version

Fix potential soundness hole in version comparison

crates/fj-app/Cargo.toml

@@ -32,7 +32,7 @@ fj-window.workspace = true
 
 [dependencies.clap]
 version = "4.0.23"
-features = ["derive"]
+features = ["derive", "string"]
 
 [dependencies.figment]
 version = "0.10.8"

crates/fj-app/src/args.rs

@@ -7,7 +7,7 @@ use fj_math::Scalar;
 
 /// Fornjot - Experimental CAD System
 #[derive(clap::Parser)]
-#[command(version = fj::version::VERSION_FULL)]
+#[command(version = fj::version::VERSION_FULL.to_string())]
 pub struct Args {
     /// The model to open
     pub model: Option<PathBuf>,

crates/fj-app/src/model_crate.rs

@@ -27,7 +27,8 @@ fn postprocess_model_files(
             ),
             (
                 r#"path = "../../crates/fj""#.to_owned(),
-                ["version = \"", fj::version::VERSION_PKG, "\""].concat(),
+                ["version = \"", &fj::version::VERSION_PKG.to_string(), "\""]
+                    .concat(),
             ),
         ],
     )?;

crates/fj-host/src/model.rs

@@ -5,8 +5,8 @@ use std::{
     str,
 };
 
-use fj::{abi, version::RawVersion};
-use tracing::warn;
+use fj::{abi, version::Version};
+use tracing::{debug, warn};
 
 use crate::{platform::HostPlatform, Parameters};
 
@@ -111,30 +111,40 @@ impl Model {
                     https://github.com/hannobraun/Fornjot/issues/1307)"
                 );
             } else {
-                let version_pkg: libloading::Symbol<fn() -> RawVersion> =
-                    lib.get(b"version_pkg").map_err(Error::LoadingVersion)?;
+                let version_pkg_host = fj::version::VERSION_PKG.to_string();
 
-                let version_pkg = version_pkg().to_string();
-                if fj::version::VERSION_PKG != version_pkg {
-                    let host = String::from_utf8_lossy(
-                        fj::version::VERSION_PKG.as_bytes(),
-                    )
-                    .into_owned();
-                    let model = version_pkg;
+                let version_pkg_model: libloading::Symbol<*const Version> =
+                    lib.get(b"VERSION_PKG").map_err(Error::LoadingVersion)?;
+                let version_pkg_mode = (**version_pkg_model).to_string();
+
+                debug!(
+                    "Comparing package versions (host: {}, model: {})",
+                    version_pkg_host, version_pkg_mode
+                );
+                if version_pkg_host != version_pkg_mode {
+                    let host =
+                        String::from_utf8_lossy(version_pkg_host.as_bytes())
+                            .into_owned();
+                    let model = version_pkg_mode;
 
                     return Err(Error::VersionMismatch { host, model });
                 }
 
-                let version_full: libloading::Symbol<fn() -> RawVersion> =
-                    lib.get(b"version_full").map_err(Error::LoadingVersion)?;
+                let version_full_host = fj::version::VERSION_FULL.to_string();
 
-                let version_full = version_full().to_string();
-                if fj::version::VERSION_FULL != version_full {
-                    let host = String::from_utf8_lossy(
-                        fj::version::VERSION_FULL.as_bytes(),
-                    )
-                    .into_owned();
-                    let model = version_full;
+                let version_full_model: libloading::Symbol<*const Version> =
+                    lib.get(b"VERSION_FULL").map_err(Error::LoadingVersion)?;
+                let version_full_model = (**version_full_model).to_string();
+
+                debug!(
+                    "Comparing full versions (host: {}, model: {})",
+                    version_full_host, version_full_model
+                );
+                if version_full_host != version_full_model {
+                    let host =
+                        String::from_utf8_lossy(version_full_host.as_bytes())
+                            .into_owned();
+                    let model = version_full_model;
 
                     warn!("{}", Error::VersionMismatch { host, model });
                 }

crates/fj/src/version.rs

@@ -1,6 +1,6 @@
 //! API for checking compatibility between the Fornjot app and a model
 
-use core::slice;
+use std::{fmt, slice};
 
 /// The Fornjot package version
 ///
@@ -11,53 +11,58 @@ use core::slice;
 /// constant between releases, even though changes are made throughout. A match
 /// of this version does not conclusively determine that the app and a model are
 /// compatible.
-pub static VERSION_PKG: &str = env!("FJ_VERSION_PKG");
+#[no_mangle]
+pub static VERSION_PKG: Version =
+    Version::from_static_str(env!("FJ_VERSION_PKG"));
 
 /// The full Fornjot version
 ///
 /// Can be used to check for compatibility between a model and the Fornjot app
 /// that runs it.
-pub static VERSION_FULL: &str = env!("FJ_VERSION_FULL");
+#[no_mangle]
+pub static VERSION_FULL: Version =
+    Version::from_static_str(env!("FJ_VERSION_FULL"));
 
 /// C-ABI-compatible representation of a version
 ///
 /// Used by the Fornjot application to check for compatibility between a model
 /// and the app.
+#[derive(Clone, Copy, Debug)]
 #[repr(C)]
-pub struct RawVersion {
-    /// The pointer to the `str`
-    pub ptr: *const u8,
-
-    /// The length of the `str`
-    pub len: usize,
+pub struct Version {
+    ptr: *const u8,
+    len: usize,
 }
 
-impl RawVersion {
-    /// Convert the `RawVersion` into a string
-    ///
-    /// # Safety
-    ///
-    /// Must be a `RawVersion` returned from one of the hidden version functions
-    /// in this module.
-    #[allow(clippy::inherent_to_string)]
-    pub unsafe fn to_string(&self) -> String {
-        let slice = slice::from_raw_parts(self.ptr, self.len);
-        String::from_utf8_lossy(slice).into_owned()
+impl Version {
+    const fn from_static_str(s: &'static str) -> Self {
+        Self {
+            ptr: s.as_ptr(),
+            len: s.len(),
+        }
     }
 }
 
-#[no_mangle]
-extern "C" fn version_pkg() -> RawVersion {
-    RawVersion {
-        ptr: VERSION_PKG.as_ptr(),
-        len: VERSION_PKG.len(),
-    }
-}
+impl fmt::Display for Version {
+    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+        // This is sound. We only ever create `ptr` and `len` from static
+        // strings.
+        let slice = unsafe { slice::from_raw_parts(self.ptr, self.len) };
 
-#[no_mangle]
-extern "C" fn version_full() -> RawVersion {
-    RawVersion {
-        ptr: VERSION_FULL.as_ptr(),
-        len: VERSION_FULL.len(),
+        write!(f, "{}", String::from_utf8_lossy(slice).into_owned())
     }
 }
+
+// The only reason this is not derived automatically, is that `Version` contains
+// a `*const u8`. `Version` can still safely be `Send`, for the following
+// reasons:
+// - The field is private, and no code in this module uses it for any write
+//   access, un-synchronized or not.
+// - `Version` can only be constructed from strings with a static lifetime, so
+//   it's guaranteed that the pointer is valid over the whole lifetime of the
+//   program.
+unsafe impl Send for Version {}
+
+// There is no reason why a `&Version` wouldn't be `Send`, so per definition,
+// `Version` can be `Sync`.
+unsafe impl Sync for Version {}