From c11215575f54b924ddc546bd8d83dcbde9c6edc0 Mon Sep 17 00:00:00 2001 From: Shachar Langbeheim Date: Wed, 8 Jan 2025 13:04:24 +0200 Subject: [PATCH] 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 --- csharp/lib/src/lib.rs | 2 +- .../benches/rotating_buffer_benchmark.rs | 8 +-- glide-core/src/rotating_buffer.rs | 8 +-- glide-core/src/socket_listener.rs | 9 ++-- go/api/response_handlers.go | 2 - go/src/lib.rs | 36 +++++-------- java/src/ffi_test.rs | 23 +++++---- node/rust-client/src/lib.rs | 51 ++++++++++--------- python/src/lib.rs | 5 +- 9 files changed, 70 insertions(+), 74 deletions(-) diff --git a/csharp/lib/src/lib.rs b/csharp/lib/src/lib.rs index c497410e31..88da043f03 100644 --- 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 { - 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 index 7f543a21d3..055702035c 100644 --- 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, 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 - 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 index 1bebb33c65..d207f3419b 100644 --- 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 - 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 index 9d137d21bf..0b034e48c3 100644 --- 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, 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 index 4a5056c0c6..9f788f507d 100644 --- 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 index 376da58dfa..f1eb794d31 100644 --- 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 index fb54fc3b5b..141d569fcb 100644 --- a/java/src/ffi_test.rs +++ b/java/src/ffi_test.rs @@ -7,6 +7,7 @@ use jni::{ JNIEnv, }; use redis::Value; +use std::ptr::from_mut; #[no_mangle] pub extern "system" fn Java_glide_ffi_FfiTest_createLeakedNil<'local>( @@ -14,7 +15,7 @@ pub extern "system" fn Java_glide_ffi_FfiTest_createLeakedNil<'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::>(); 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 index ffa5b5c47f..584eab16de 100644 --- 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,7 +316,7 @@ fn split_pointer(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) } @@ -323,7 +324,7 @@ pub fn create_leaked_string(message: String) -> [u32; 2] { pub fn create_leaked_string_vec(message: Vec) -> [u32; 2] { // Convert the string vec -> Bytes vector let bytes_vec: Vec = message.iter().map(|v| Bytes::from(v.to_vec())).collect(); - let pointer = Box::leak(Box::new(bytes_vec)) as *mut Vec; + 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) -> [u32; 2] { /// Should NOT be used in production. #[cfg(feature = "testing_utilities")] pub fn create_leaked_map(map: HashMap) -> [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) -> [u32; 2] { /// Should NOT be used in production. #[cfg(feature = "testing_utilities")] pub fn create_leaked_array(array: Vec) -> [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) -> [u32; 2] { /// Should NOT be used in production. #[cfg(feature = "testing_utilities")] pub fn create_leaked_attribute(message: String, attribute: HashMap) -> [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 [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 index 09914c2c59..5e33ab8bd3 100644 --- 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) -> 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) -> PyResult<()> { Bytes::from(bytes.to_vec()) }) .collect(); - Box::leak(Box::new(bytes_vec)) as *mut Vec as usize + from_mut(Box::leak(Box::new(bytes_vec))) as usize } Ok(()) }