Merge pull request #435 from RedisLabsModules/push-qowsolyxxttn

fix: add `to_raw_parts` method & fix unsoundness bug

Author: JonasKruckenberg

examples/scan_keys.rs

@@ -17,8 +17,8 @@ fn scan_keys(ctx: &Context, _args: Vec<RedisString>) -> RedisResult {
     let cursor = KeysCursor::new();
     let mut res = Vec::new();
 
-    let scan_callback = |_ctx: &Context, key_name: RedisString, _key: Option<&RedisKey>| {
-        res.push(RedisValue::BulkRedisString(key_name));
+    let scan_callback = |ctx: &Context, key_name: &RedisString, _key: Option<&RedisKey>| {
+        res.push(RedisValue::BulkRedisString(key_name.safe_clone(ctx)));
     };
 
     while cursor.scan(ctx, &scan_callback) {

src/context/key_cursor.rs

@@ -1,5 +1,6 @@
 use std::{
     ffi::c_void,
+    mem,
     ptr::{self},
 };
 
@@ -24,7 +25,7 @@ use crate::{key::RedisKey, raw, RedisString};
 /// fn example_scan_key_for_each(ctx: &Context) -> RedisResult {
 ///    let key = ctx.open_key_with_flags("user:123", KeyFlags::NOEFFECTS | KeyFlags::NOEXPIRE | KeyFlags::ACCESS_EXPIRED );
 ///    let cursor  = ScanKeyCursor::new(key);
-///    
+///
 ///    let res = RefCell::new(Vec::new());
 ///    cursor.for_each(|_key, field, value| {
 ///        let mut res = res.borrow_mut();
@@ -92,11 +93,10 @@ impl ScanKeyCursor {
             let callback = unsafe { &mut *(data.cast::<F>()) };
             callback(&key, &field, &value);
 
-            // we're not the owner of field and value strings
-            field.take();
-            value.take();
-
-            key.take(); // we're not the owner of the key either
+            // We don't own any of the passed in pointers, so we must ensure we don't run their destructors here
+            mem::forget(field);
+            mem::forget(value);
+            mem::forget(key);
         }
 
         // Safety: The c-side initialized the function ptr and it is is never changed,

src/context/keys_cursor.rs

@@ -3,13 +3,14 @@ use crate::key::RedisKey;
 use crate::raw;
 use crate::redismodule::RedisString;
 use std::ffi::c_void;
+use std::mem;
 use std::ptr::NonNull;
 
 pub struct KeysCursor {
     inner_cursor: *mut raw::RedisModuleScanCursor,
 }
 
-extern "C" fn scan_callback<C: FnMut(&Context, RedisString, Option<&RedisKey>)>(
+extern "C" fn scan_callback<C: FnMut(&Context, &RedisString, Option<&RedisKey>)>(
     ctx: *mut raw::RedisModuleCtx,
     key_name: *mut raw::RedisModuleString,
     key: *mut raw::RedisModuleKey,
@@ -20,13 +21,17 @@ extern "C" fn scan_callback<C: FnMut(&Context, RedisString, Option<&RedisKey>)>(
     let redis_key = if key.is_null() {
         None
     } else {
-        Some(RedisKey::from_raw_parts(ctx, key))
+        // Safety: The returned `RedisKey` does not outlive this callbacks and so by necessity
+        // the pointers passed in as parameters are valid for its entire lifetime.
+        Some(unsafe { RedisKey::from_raw_parts(ctx, key) })
     };
     let callback = unsafe { &mut *(private_data.cast::<C>()) };
-    callback(&context, key_name, redis_key.as_ref());
+    callback(&context, &key_name, redis_key.as_ref());
 
-    // we are not the owner of the key, so we must take the underline *mut raw::RedisModuleKey so it will not be freed.
-    redis_key.map(|v| v.take());
+    // We don't own any of the passed in pointers and have just created "temporary RAII types".
+    // We must ensure we don't run their destructors here.
+    mem::forget(redis_key);
+    mem::forget(key_name);
 }
 
 impl KeysCursor {
@@ -35,7 +40,7 @@ impl KeysCursor {
         Self { inner_cursor }
     }
 
-    pub fn scan<F: FnMut(&Context, RedisString, Option<&RedisKey>)>(
+    pub fn scan<F: FnMut(&Context, &RedisString, Option<&RedisKey>)>(
         &self,
         ctx: &Context,
         callback: &F,

src/key.rs

@@ -59,12 +59,6 @@ pub struct RedisKey {
 }
 
 impl RedisKey {
-    pub(crate) fn take(mut self) -> *mut raw::RedisModuleKey {
-        let res = self.key_inner;
-        self.key_inner = std::ptr::null_mut();
-        res
-    }
-
     pub fn open(ctx: *mut raw::RedisModuleCtx, key: &RedisString) -> Self {
         let key_inner = raw::open_key(ctx, key.inner, to_raw_mode(KeyMode::Read));
         Self { ctx, key_inner }
@@ -80,13 +74,29 @@ impl RedisKey {
         Self { ctx, key_inner }
     }
 
-    pub const fn from_raw_parts(
+    /// Construct a new `RedisKey` from a pointer to a redismodule context and a key.
+    ///
+    /// # Safety
+    ///
+    /// The caller must ensure:
+    /// 1. The `ctx` pointer remains valid for the lifetime of the `RedisKey`.
+    /// 2. The `key_inner` pointer remains valid for the lifetime of the `RedisKey`.
+    pub const unsafe fn from_raw_parts(
         ctx: *mut raw::RedisModuleCtx,
         key_inner: *mut raw::RedisModuleKey,
     ) -> Self {
         Self { ctx, key_inner }
     }
 
+    /// Decomposes a `RedisKey` into its raw components: `(redismodule context pointer, key pointer)`.
+    ///
+    /// After calling this function, the caller is responsible for cleaning up the raw key previously managed by the `RedisKey`.
+    /// The only way to do this safely is to convert the raw redismodule context and key pointers back into a `RedisKey` with
+    /// the [`from_raw_parts`][Self::from_raw_parts] function, allowing the destructor to perform the cleanup.
+    pub fn to_raw_parts(self) -> (*mut raw::RedisModuleCtx, *mut raw::RedisModuleKey) {
+        (self.ctx, self.key_inner)
+    }
+
     /// # Panics
     ///
     /// Will panic if `RedisModule_ModuleTypeGetValue` is missing in redismodule.h