Core: Some pointer shenanigans. (#2895)

* Use C-string literals instead of allocating CStrings. * Use the `from_mut` function where relevant. Signed-off-by: Shachar Langbeheim <[email protected]>
valkey-io · Jan 8, 2025 · c112155 · c112155
1 parent 4902f80
commit c112155
Show file tree

Hide file tree

Showing 9 changed files with 70 additions and 74 deletions.
diff --git a/csharp/lib/src/lib.rs b/csharp/lib/src/lib.rs
@@ -51,7 +51,7 @@ fn create_client_internal(
     success_callback: unsafe extern "C" fn(usize, *const c_char) -> (),
     failure_callback: unsafe extern "C" fn(usize) -> (),
 ) -> RedisResult<Client> {
-    let host_cstring = unsafe { CStr::from_ptr(host as *mut c_char) };
+    let host_cstring = unsafe { CStr::from_ptr(host) };
     let host_string = host_cstring.to_str()?.to_string();
     let request = create_connection_request(host_string, port, use_tls);
     let runtime = Builder::new_multi_thread()

diff --git a/glide-core/benches/rotating_buffer_benchmark.rs b/glide-core/benches/rotating_buffer_benchmark.rs
@@ -1,6 +1,6 @@
 // Copyright Valkey GLIDE Project Contributors - SPDX Identifier: Apache-2.0
 
-use std::io::Write;
+use std::{io::Write, ptr::from_mut};
 
 use bytes::BufMut;
 use criterion::{black_box, criterion_group, criterion_main, Criterion};
@@ -169,9 +169,9 @@ fn create_request(args: Vec<bytes::Bytes>, args_pointer: bool) -> CommandRequest
     let mut command = Command::new();
     command.request_type = RequestType::CustomCommand.into();
     if args_pointer {
-        command.args = Some(command::Args::ArgsVecPointer(Box::leak(Box::new(args))
-            as *mut Vec<bytes::Bytes>
-            as u64));
+        command.args = Some(command::Args::ArgsVecPointer(
+            from_mut(Box::leak(Box::new(args))) as u64,
+        ));
     } else {
         let mut args_array = command::ArgsArray::new();
         args_array.args = args;

diff --git a/glide-core/src/rotating_buffer.rs b/glide-core/src/rotating_buffer.rs
@@ -62,6 +62,8 @@ impl RotatingBuffer {
 
 #[cfg(test)]
 mod tests {
+    use std::ptr::from_mut;
+
     use super::*;
     use crate::command_request::{command, command_request};
     use crate::command_request::{Command, CommandRequest, RequestType};
@@ -87,9 +89,9 @@ mod tests {
         let mut command = Command::new();
         command.request_type = request_type.into();
         if args_pointer {
-            command.args = Some(command::Args::ArgsVecPointer(Box::leak(Box::new(args))
-                as *mut Vec<Bytes>
-                as u64));
+            command.args = Some(command::Args::ArgsVecPointer(
+                from_mut(Box::leak(Box::new(args))) as u64,
+            ));
         } else {
             let mut args_array = command::ArgsArray::new();
             args_array.args.clone_from(&args);

diff --git a/glide-core/src/socket_listener.rs b/glide-core/src/socket_listener.rs
@@ -22,6 +22,7 @@ use redis::cluster_routing::{ResponsePolicy, Routable};
 use redis::{ClusterScanArgs, Cmd, PushInfo, RedisError, ScanStateRC, Value};
 use std::cell::Cell;
 use std::collections::HashSet;
+use std::ptr::from_mut;
 use std::rc::Rc;
 use std::sync::RwLock;
 use std::{env, str};
@@ -191,8 +192,8 @@ async fn write_result(
             if value != Value::Nil {
                 // Since null values don't require any additional data, they can be sent without any extra effort.
                 // Move the value to the heap and leak it. The wrapper should use `Box::from_raw` to recreate the box, use the value, and drop the allocation.
-                let pointer = Box::leak(Box::new(value));
-                let raw_pointer = pointer as *mut redis::Value;
+                let reference = Box::leak(Box::new(value));
+                let raw_pointer = from_mut(reference);
                 Some(response::response::Value::RespPointer(raw_pointer as u64))
             } else {
                 None
@@ -639,8 +640,8 @@ async fn push_manager_loop(mut push_rx: mpsc::UnboundedReceiver<PushInfo>, write
                         kind: (push_msg.kind),
                         data: (push_msg.data),
                     };
-                    let pointer = Box::leak(Box::new(push_val));
-                    let raw_pointer = pointer as *mut redis::Value;
+                    let reference = Box::leak(Box::new(push_val));
+                    let raw_pointer = from_mut(reference);
                     Some(response::response::Value::RespPointer(raw_pointer as u64))
                 };
 

diff --git a/go/api/response_handlers.go b/go/api/response_handlers.go
@@ -14,7 +14,6 @@ import (
 func checkResponseType(response *C.struct_CommandResponse, expectedType C.ResponseType, isNilable bool) error {
 	expectedTypeInt := uint32(expectedType)
 	expectedTypeStr := C.get_response_type_string(expectedTypeInt)
-	defer C.free_response_type_string(expectedTypeStr)
 
 	if !isNilable && response == nil {
 		return &RequestError{
@@ -34,7 +33,6 @@ func checkResponseType(response *C.struct_CommandResponse, expectedType C.Respon
 	}
 
 	actualTypeStr := C.get_response_type_string(response.response_type)
-	defer C.free_response_type_string(actualTypeStr)
 	return &RequestError{
 		fmt.Sprintf(
 			"Unexpected return type from Valkey: got %s, expected %s",

diff --git a/go/src/lib.rs b/go/src/lib.rs
@@ -258,31 +258,21 @@ pub unsafe extern "C" fn free_connection_response(
 }
 
 /// Provides the string mapping for the ResponseType enum.
-#[no_mangle]
-pub extern "C" fn get_response_type_string(response_type: ResponseType) -> *mut c_char {
-    let s = match response_type {
-        ResponseType::Null => "Null",
-        ResponseType::Int => "Int",
-        ResponseType::Float => "Float",
-        ResponseType::Bool => "Bool",
-        ResponseType::String => "String",
-        ResponseType::Array => "Array",
-        ResponseType::Map => "Map",
-        ResponseType::Sets => "Sets",
-    };
-    let c_str = CString::new(s).unwrap_or_default();
-    c_str.into_raw()
-}
-
-/// Deallocates a string generated via get_response_type_string.
 ///
-/// # Safety
-/// free_response_type_string can be called only once per response_string.
+/// Important: the returned pointer is a pointer to a constant string and should not be freed.
 #[no_mangle]
-pub extern "C" fn free_response_type_string(response_string: *mut c_char) {
-    if !response_string.is_null() {
-        drop(unsafe { CString::from_raw(response_string as *mut c_char) });
-    }
+pub extern "C" fn get_response_type_string(response_type: ResponseType) -> *const c_char {
+    let c_str = match response_type {
+        ResponseType::Null => c"Null",
+        ResponseType::Int => c"Int",
+        ResponseType::Float => c"Float",
+        ResponseType::Bool => c"Bool",
+        ResponseType::String => c"String",
+        ResponseType::Array => c"Array",
+        ResponseType::Map => c"Map",
+        ResponseType::Sets => c"Sets",
+    };
+    c_str.as_ptr()
 }
 
 /// Deallocates a `CommandResponse`.

diff --git a/java/src/ffi_test.rs b/java/src/ffi_test.rs
@@ -7,14 +7,15 @@ use jni::{
     JNIEnv,
 };
 use redis::Value;
+use std::ptr::from_mut;
 
 #[no_mangle]
 pub extern "system" fn Java_glide_ffi_FfiTest_createLeakedNil<'local>(
     _env: JNIEnv<'local>,
     _class: JClass<'local>,
 ) -> jlong {
     let resp_value = Value::Nil;
-    Box::leak(Box::new(resp_value)) as *mut Value as jlong
+    from_mut(Box::leak(Box::new(resp_value))) as jlong
 }
 
 #[no_mangle]
@@ -25,7 +26,7 @@ pub extern "system" fn Java_glide_ffi_FfiTest_createLeakedSimpleString<'local>(
 ) -> jlong {
     let value: String = env.get_string(&value).unwrap().into();
     let resp_value = Value::SimpleString(value);
-    Box::leak(Box::new(resp_value)) as *mut Value as jlong
+    from_mut(Box::leak(Box::new(resp_value))) as jlong
 }
 
 #[no_mangle]
@@ -34,7 +35,7 @@ pub extern "system" fn Java_glide_ffi_FfiTest_createLeakedOkay<'local>(
     _class: JClass<'local>,
 ) -> jlong {
     let resp_value = Value::Okay;
-    Box::leak(Box::new(resp_value)) as *mut Value as jlong
+    from_mut(Box::leak(Box::new(resp_value))) as jlong
 }
 
 #[no_mangle]
@@ -44,7 +45,7 @@ pub extern "system" fn Java_glide_ffi_FfiTest_createLeakedInt<'local>(
     value: jlong,
 ) -> jlong {
     let resp_value = Value::Int(value);
-    Box::leak(Box::new(resp_value)) as *mut Value as jlong
+    from_mut(Box::leak(Box::new(resp_value))) as jlong
 }
 
 #[no_mangle]
@@ -56,7 +57,7 @@ pub extern "system" fn Java_glide_ffi_FfiTest_createLeakedBulkString<'local>(
     let value = env.convert_byte_array(&value).unwrap();
     let value = value.into_iter().collect::<Vec<u8>>();
     let resp_value = Value::BulkString(value);
-    Box::leak(Box::new(resp_value)) as *mut Value as jlong
+    from_mut(Box::leak(Box::new(resp_value))) as jlong
 }
 
 #[no_mangle]
@@ -67,7 +68,7 @@ pub extern "system" fn Java_glide_ffi_FfiTest_createLeakedLongArray<'local>(
 ) -> jlong {
     let array = java_long_array_to_value(&mut env, &value);
     let resp_value = Value::Array(array);
-    Box::leak(Box::new(resp_value)) as *mut Value as jlong
+    from_mut(Box::leak(Box::new(resp_value))) as jlong
 }
 
 #[no_mangle]
@@ -81,7 +82,7 @@ pub extern "system" fn Java_glide_ffi_FfiTest_createLeakedMap<'local>(
     let values_vec = java_long_array_to_value(&mut env, &values);
     let map: Vec<(Value, Value)> = keys_vec.into_iter().zip(values_vec).collect();
     let resp_value = Value::Map(map);
-    Box::leak(Box::new(resp_value)) as *mut Value as jlong
+    from_mut(Box::leak(Box::new(resp_value))) as jlong
 }
 
 #[no_mangle]
@@ -91,7 +92,7 @@ pub extern "system" fn Java_glide_ffi_FfiTest_createLeakedDouble<'local>(
     value: jdouble,
 ) -> jlong {
     let resp_value = Value::Double(value.into());
-    Box::leak(Box::new(resp_value)) as *mut Value as jlong
+    from_mut(Box::leak(Box::new(resp_value))) as jlong
 }
 
 #[no_mangle]
@@ -101,7 +102,7 @@ pub extern "system" fn Java_glide_ffi_FfiTest_createLeakedBoolean<'local>(
     value: jboolean,
 ) -> jlong {
     let resp_value = Value::Boolean(value != 0);
-    Box::leak(Box::new(resp_value)) as *mut Value as jlong
+    from_mut(Box::leak(Box::new(resp_value))) as jlong
 }
 
 #[no_mangle]
@@ -116,7 +117,7 @@ pub extern "system" fn Java_glide_ffi_FfiTest_createLeakedVerbatimString<'local>
         format: VerbatimFormat::Text,
         text: value,
     };
-    Box::leak(Box::new(resp_value)) as *mut Value as jlong
+    from_mut(Box::leak(Box::new(resp_value))) as jlong
 }
 
 #[no_mangle]
@@ -127,7 +128,7 @@ pub extern "system" fn Java_glide_ffi_FfiTest_createLeakedLongSet<'local>(
 ) -> jlong {
     let set = java_long_array_to_value(&mut env, &value);
     let resp_value = Value::Set(set);
-    Box::leak(Box::new(resp_value)) as *mut Value as jlong
+    from_mut(Box::leak(Box::new(resp_value))) as jlong
 }
 
 fn java_long_array_to_value<'local>(

diff --git a/node/rust-client/src/lib.rs b/node/rust-client/src/lib.rs
@@ -24,6 +24,7 @@ use num_traits::sign::Signed;
 use redis::{aio::MultiplexedConnection, AsyncCommands, Value};
 #[cfg(feature = "testing_utilities")]
 use std::collections::HashMap;
+use std::ptr::from_mut;
 use std::str;
 use tokio::runtime::{Builder, Runtime};
 #[napi]
@@ -315,15 +316,15 @@ fn split_pointer<T>(pointer: *mut T) -> [u32; 2] {
 #[cfg(feature = "testing_utilities")]
 pub fn create_leaked_string(message: String) -> [u32; 2] {
     let value = Value::SimpleString(message);
-    let pointer = Box::leak(Box::new(value)) as *mut Value;
+    let pointer = from_mut(Box::leak(Box::new(value)));
     split_pointer(pointer)
 }
 
 #[napi(ts_return_type = "[number, number]")]
 pub fn create_leaked_string_vec(message: Vec<Uint8Array>) -> [u32; 2] {
     // Convert the string vec -> Bytes vector
     let bytes_vec: Vec<Bytes> = message.iter().map(|v| Bytes::from(v.to_vec())).collect();
-    let pointer = Box::leak(Box::new(bytes_vec)) as *mut Vec<Bytes>;
+    let pointer = from_mut(Box::leak(Box::new(bytes_vec)));
     split_pointer(pointer)
 }
 
@@ -332,11 +333,11 @@ pub fn create_leaked_string_vec(message: Vec<Uint8Array>) -> [u32; 2] {
 /// Should NOT be used in production.
 #[cfg(feature = "testing_utilities")]
 pub fn create_leaked_map(map: HashMap<String, String>) -> [u32; 2] {
-    let pointer = Box::leak(Box::new(Value::Map(
+    let pointer = from_mut(Box::leak(Box::new(Value::Map(
         map.into_iter()
             .map(|(key, value)| (Value::SimpleString(key), Value::SimpleString(value)))
             .collect(),
-    ))) as *mut Value;
+    ))));
     split_pointer(pointer)
 }
 
@@ -345,9 +346,9 @@ pub fn create_leaked_map(map: HashMap<String, String>) -> [u32; 2] {
 /// Should NOT be used in production.
 #[cfg(feature = "testing_utilities")]
 pub fn create_leaked_array(array: Vec<String>) -> [u32; 2] {
-    let pointer = Box::leak(Box::new(Value::Array(
+    let pointer = from_mut(Box::leak(Box::new(Value::Array(
         array.into_iter().map(Value::SimpleString).collect(),
-    ))) as *mut Value;
+    ))));
     split_pointer(pointer)
 }
 
@@ -356,13 +357,13 @@ pub fn create_leaked_array(array: Vec<String>) -> [u32; 2] {
 /// Should NOT be used in production.
 #[cfg(feature = "testing_utilities")]
 pub fn create_leaked_attribute(message: String, attribute: HashMap<String, String>) -> [u32; 2] {
-    let pointer = Box::leak(Box::new(Value::Attribute {
+    let pointer = from_mut(Box::leak(Box::new(Value::Attribute {
         data: Box::new(Value::SimpleString(message)),
         attributes: attribute
             .into_iter()
             .map(|(key, value)| (Value::SimpleString(key), Value::SimpleString(value)))
             .collect(),
-    })) as *mut Value;
+    })));
     split_pointer(pointer)
 }
 
@@ -371,21 +372,23 @@ pub fn create_leaked_attribute(message: String, attribute: HashMap<String, Strin
 /// Should NOT be used in production.
 #[cfg(feature = "testing_utilities")]
 pub fn create_leaked_bigint(big_int: BigInt) -> [u32; 2] {
-    let pointer = Box::leak(Box::new(Value::BigNumber(num_bigint::BigInt::new(
-        if big_int.sign_bit {
-            num_bigint::Sign::Minus
-        } else {
-            num_bigint::Sign::Plus
-        },
-        big_int
-            .words
-            .into_iter()
-            .flat_map(|word| {
-                let bytes = u64::to_le_bytes(word);
-                unsafe { std::mem::transmute::<[u8; 8], [u32; 2]>(bytes) }
-            })
-            .collect(),
-    )))) as *mut Value;
+    let pointer = from_mut(Box::leak(Box::new(Value::BigNumber(
+        num_bigint::BigInt::new(
+            if big_int.sign_bit {
+                num_bigint::Sign::Minus
+            } else {
+                num_bigint::Sign::Plus
+            },
+            big_int
+                .words
+                .into_iter()
+                .flat_map(|word| {
+                    let bytes = u64::to_le_bytes(word);
+                    unsafe { std::mem::transmute::<[u8; 8], [u32; 2]>(bytes) }
+                })
+                .collect(),
+        ),
+    ))));
     split_pointer(pointer)
 }
 
@@ -394,7 +397,7 @@ pub fn create_leaked_bigint(big_int: BigInt) -> [u32; 2] {
 /// Should NOT be used in production.
 #[cfg(feature = "testing_utilities")]
 pub fn create_leaked_double(float: f64) -> [u32; 2] {
-    let pointer = Box::leak(Box::new(Value::Double(float))) as *mut Value;
+    let pointer = from_mut(Box::leak(Box::new(Value::Double(float))));
     split_pointer(pointer)
 }
 

diff --git a/python/src/lib.rs b/python/src/lib.rs
@@ -11,6 +11,7 @@ use pyo3::types::{PyAny, PyBool, PyBytes, PyDict, PyFloat, PyList, PySet, PyStri
 use pyo3::Python;
 use redis::Value;
 use std::collections::HashMap;
+use std::ptr::from_mut;
 use std::sync::Arc;
 
 pub const DEFAULT_TIMEOUT_IN_MILLISECONDS: u32 =
@@ -263,7 +264,7 @@ fn glide(_py: Python, m: &Bound<PyModule>) -> PyResult<()> {
     /// Should NOT be used in production.
     pub fn create_leaked_value(message: String) -> usize {
         let value = Value::SimpleString(message);
-        Box::leak(Box::new(value)) as *mut Value as usize
+        from_mut(Box::leak(Box::new(value))) as usize
     }
 
     #[pyfunction]
@@ -276,7 +277,7 @@ fn glide(_py: Python, m: &Bound<PyModule>) -> PyResult<()> {
                 Bytes::from(bytes.to_vec())
             })
             .collect();
-        Box::leak(Box::new(bytes_vec)) as *mut Vec<Bytes> as usize
+        from_mut(Box::leak(Box::new(bytes_vec))) as usize
     }
     Ok(())
 }