Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

New VPriv support #73

Open
nyurik opened this issue Sep 14, 2024 · 3 comments · May be fixed by #91
Open

New VPriv support #73

nyurik opened this issue Sep 14, 2024 · 3 comments · May be fixed by #91

Comments

@nyurik
Copy link
Collaborator

nyurik commented Sep 14, 2024

I came up with this model - showing the changes needed in the varnish crate, changes to the generated code, and how it will be used by the user. Note that for the user it will probably need some attributes too.

Most important change: this completelly gets rid of VPriv and VPrivInt structs.

//
// This is part of the Varnish core Rust lib
//
mod varnish {
    use std::ffi::c_void;
    use std::ptr;
    use std::ptr::null;

    use crate::ffi;

    /// SAFETY: ensured by Varnish itself
    unsafe impl Sync for ffi::vmod_priv_methods {}

    /// Take ownership of the object of type `T` and return it as a `Box<T>`.
    /// The original pointer is set to null.
    ///
    /// SAFETY: `priv_` must reference a valid `T` object pointer or `NULL`
    unsafe fn get_owned_bbox<T>(priv_: &mut *mut c_void) -> Option<Box<T>> {
        let obj = ptr::replace(priv_, ptr::null_mut());
        if obj.is_null() {
            None
        } else {
            Some(Box::from_raw(obj.cast::<T>()))
        }
    }

    /// A Varnish callback function to free a `vmod_priv` object.
    /// Here we take the ownership and immediately drop the object of type `T`.
    /// Note that here we get `*priv_` directly, not the `*vmod_priv`
    ///
    /// SAFETY: `priv_` must be a valid pointer to a `T` object or `NULL`.
    pub unsafe extern "C" fn free_vmod_priv<T>(_ctx: *const ffi::vrt_ctx, mut priv_: *mut c_void) {
        // we get the ownership and don't do anything with it, so it gets dropped at the end of this fn
        get_owned_bbox::<T>(&mut priv_);
    }

    impl ffi::vmod_priv {
        /// Transfer ownership of the object to the caller, cleaning up the internal state.
        ///
        /// SAFETY: `priv_` must reference a valid `T` object pointer or `NULL`
        pub unsafe fn take<T>(&mut self) -> Option<Box<T>> {
            self.methods = null();
            get_owned_bbox(&mut self.priv_)
        }

        /// Set the object and methods for the `vmod_priv`, and the corresponding static methods.
        ///
        /// SAFETY: The type of `obj` must match the type of the function pointers in `methods`.
        pub unsafe fn put<T>(&mut self, obj: Box<T>, methods: &'static ffi::vmod_priv_methods) {
            self.priv_ = Box::into_raw(obj).cast();
            self.methods = methods;
        }

        /// Use the object as a reference, without taking ownership.
        ///
        /// SAFETY:
        /// * `priv_` must reference a valid `T` object pointer or `NULL`
        /// * Assumes the pointer stored in `priv_` can be re-interpreted as a &Option<Box<T>>
        /// * `take()` must not be called on the same `vmod_priv` object until the returned reference is dropped
        /// * cleanup must not be done on the object until the returned reference is dropped
        pub unsafe fn get_ref<T>(&self) -> Option<&T> {
            Some(self.priv_.cast::<Box<T>>().as_ref()?)
        }
    }
}

//
// This is generated with proc macro based on the user code (see below)
//
pub mod generated {
    use super::user_code;
    use crate::ffi;
    use crate::vpriv_demo::varnish::free_vmod_priv;

    /// Static methods to clean up the `vmod_priv` object's `user_code::UserType`
    static MY_TYPE_METHODS: ffi::vmod_priv_methods = ffi::vmod_priv_methods {
        magic: ffi::VMOD_PRIV_METHODS_MAGIC,
        type_: c"user_code::UserType".as_ptr(),
        fini: Some(free_vmod_priv::<user_code::UserType>),
    };

    /// Called by Varnish as a wrapper around the user's mutable function
    pub unsafe fn user_mut_fn_wrapper(vp: *mut ffi::vmod_priv) {
        // Take ownership of the user's object and call the user's function with it
        let mut obj = (*vp).take();
        user_code::user_mut_func(&mut obj);
        if let Some(obj) = obj {
            // Release ownership back to Varnish
            (*vp).put(obj, &MY_TYPE_METHODS);
        }
    }

    /// Called by Varnish in a read-only context
    pub unsafe fn user_fn_wrapper(vp: *const ffi::vmod_priv) -> i32 {
        // Do not take ownership - multiple simultaneous calls are allowed
        user_code::user_func((*vp).get_ref())
    }
}

//
// This is what a user could write
//
pub mod user_code {
    /// Example user type
    pub struct UserType {
        pub data: i32,
    }

    /// Mutable access to the data field of UserType, instantiating it if needed
    /// Rust ensures this function is the only one that owns UserType
    pub fn user_mut_func(obj: &mut Option<Box<UserType>>) {
        if let Some(obj) = obj {
            obj.data += 1;
        } else {
            *obj = Some(Box::new(UserType { data: 1 }));
        }
    }

    /// Read-only access to the data field of UserType.
    /// Multiple refs to the same Box could exist, and can contain Mutex or RwLock if needed.
    pub fn user_func(obj: Option<&UserType>) -> i32 {
        obj.as_ref().map_or(0, |obj| obj.data)
    }
}
@gquintard
Copy link
Owner

that looks pretty similar to the existing code to be honest, except you are forcing the user to reason about the Box, which can be pretty dangerous?

@nyurik
Copy link
Collaborator Author

nyurik commented Sep 15, 2024

I think it is actually the opposite -- I added a lot of comments to explain, and improved the code a bit. Main difference:

  • User's user_func function cannot accidentally modify a readonly object, e.g. in PRIV_VCL handler
  • User's user_mut_func gets guaranteed exclusive access to the object (mut ref to an owned object) - and any manipulations with it, e.g. *obj = Some(Box::new(UserType { data: 1 })); guarantees to drop the old one.
  • Internals are far far simpler, and do not rely on any lifetime magic
  • VPriv/VprivInt are riddled with a potential of security holes, whereas Box has been tested for 15+ years by entire community
  • there is no copying of stack-created object on the stack - it happens in the user code on the heap from the start
  • other reasons i can't think of :)

@nyurik
Copy link
Collaborator Author

nyurik commented Sep 15, 2024

P.S. Note that readonly access is actually simpler because Option<&Box<T>> and Option<&T> are effectively equivalent

@nyurik nyurik linked a pull request Oct 7, 2024 that will close this issue
23 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants