Functions suppressing not_unsafe_ptr_arg_deref clippy lint are unsound

[nico-abram]

All of these functions supressing the clippy lint clippy::not_unsafe_ptr_arg_deref seem to be trivially unsound:

redismodule-rs/src/raw.rs

Lines 185 to 591 in 56a8082

	#[allow(clippy::not_unsafe_ptr_arg_deref)]
	pub fn call_reply_type(reply: *mut RedisModuleCallReply) -> ReplyType {
	unsafe {
	// TODO: Cache the unwrapped functions and use them instead of unwrapping every time?
	RedisModule_CallReplyType.unwrap()(reply).into()
	}
	}
	#[allow(clippy::not_unsafe_ptr_arg_deref)]
	pub fn free_call_reply(reply: *mut RedisModuleCallReply) {
	unsafe { RedisModule_FreeCallReply.unwrap()(reply) }
	}
	#[allow(clippy::not_unsafe_ptr_arg_deref)]
	pub fn call_reply_integer(reply: *mut RedisModuleCallReply) -> c_longlong {
	unsafe { RedisModule_CallReplyInteger.unwrap()(reply) }
	}
	#[allow(clippy::not_unsafe_ptr_arg_deref)]
	pub fn call_reply_array_element(
	reply: *mut RedisModuleCallReply,
	idx: usize,
	) -> *mut RedisModuleCallReply {
	unsafe { RedisModule_CallReplyArrayElement.unwrap()(reply, idx) }
	}
	#[allow(clippy::not_unsafe_ptr_arg_deref)]
	pub fn call_reply_length(reply: *mut RedisModuleCallReply) -> usize {
	unsafe { RedisModule_CallReplyLength.unwrap()(reply) }
	}
	#[allow(clippy::not_unsafe_ptr_arg_deref)]
	pub fn call_reply_string_ptr(reply: *mut RedisModuleCallReply, len: *mut size_t) -> *const c_char {
	unsafe { RedisModule_CallReplyStringPtr.unwrap()(reply, len) }
	}
	#[allow(clippy::not_unsafe_ptr_arg_deref)]
	pub fn call_reply_string(reply: *mut RedisModuleCallReply) -> String {
	unsafe {
	let mut len: size_t = 0;
	let reply_string: *mut u8 =
	RedisModule_CallReplyStringPtr.unwrap()(reply, &mut len) as *mut u8;
	String::from_utf8(
	slice::from_raw_parts(reply_string, len)
	.iter()
	.copied()
	.collect(),
	)
	.unwrap()
	}
	}
	#[allow(clippy::not_unsafe_ptr_arg_deref)]
	pub fn close_key(kp: *mut RedisModuleKey) {
	unsafe { RedisModule_CloseKey.unwrap()(kp) }
	}
	#[allow(clippy::not_unsafe_ptr_arg_deref)]
	pub fn open_key(
	ctx: *mut RedisModuleCtx,
	keyname: *mut RedisModuleString,
	mode: KeyMode,
	) -> *mut RedisModuleKey {
	unsafe { RedisModule_OpenKey.unwrap()(ctx, keyname, mode.bits).cast::<RedisModuleKey>() }
	}
	#[allow(clippy::not_unsafe_ptr_arg_deref)]
	pub fn reply_with_array(ctx: *mut RedisModuleCtx, len: c_long) -> Status {
	unsafe { RedisModule_ReplyWithArray.unwrap()(ctx, len).into() }
	}
	#[allow(clippy::not_unsafe_ptr_arg_deref)]
	pub fn reply_with_error(ctx: *mut RedisModuleCtx, err: *const c_char) {
	unsafe {
	let msg = Context::str_as_legal_resp_string(CStr::from_ptr(err).to_str().unwrap());
	RedisModule_ReplyWithError.unwrap()(ctx, msg.as_ptr());
	}
	}
	#[allow(clippy::not_unsafe_ptr_arg_deref)]
	pub fn reply_with_long_long(ctx: *mut RedisModuleCtx, ll: c_longlong) -> Status {
	unsafe { RedisModule_ReplyWithLongLong.unwrap()(ctx, ll).into() }
	}
	#[allow(clippy::not_unsafe_ptr_arg_deref)]
	pub fn reply_with_double(ctx: *mut RedisModuleCtx, f: c_double) -> Status {
	unsafe { RedisModule_ReplyWithDouble.unwrap()(ctx, f).into() }
	}
	#[allow(clippy::not_unsafe_ptr_arg_deref)]
	pub fn reply_with_string(ctx: *mut RedisModuleCtx, s: *mut RedisModuleString) -> Status {
	unsafe { RedisModule_ReplyWithString.unwrap()(ctx, s).into() }
	}
	// Sets the expiry on a key.
	//
	// Expire is in milliseconds.
	#[allow(clippy::not_unsafe_ptr_arg_deref)]
	pub fn set_expire(key: *mut RedisModuleKey, expire: c_longlong) -> Status {
	unsafe { RedisModule_SetExpire.unwrap()(key, expire).into() }
	}
	#[allow(clippy::not_unsafe_ptr_arg_deref)]
	pub fn string_dma(key: *mut RedisModuleKey, len: *mut size_t, mode: KeyMode) -> *const c_char {
	unsafe { RedisModule_StringDMA.unwrap()(key, len, mode.bits) }
	}
	#[allow(clippy::not_unsafe_ptr_arg_deref)]
	pub fn hash_get_multi<T>(
	key: *mut RedisModuleKey,
	fields: &[T],
	values: &mut [*mut RedisModuleString],
	) -> Result<(), RedisError>
	where
	T: Into<Vec<u8>> + Clone,
	{
	assert_eq!(fields.len(), values.len());
	let mut fi = fields.iter();
	let mut vi = values.iter_mut();
	macro_rules! rm {
	() => { unsafe {
	RedisModule_HashGet.unwrap()(key, REDISMODULE_HASH_CFIELDS as i32,
	ptr::null::<c_char>())
	}};
	($($args:expr)*) => { unsafe {
	RedisModule_HashGet.unwrap()(
	key, REDISMODULE_HASH_CFIELDS as i32,
	$($args),*,
	ptr::null::<c_char>()
	)
	}};
	}
	macro_rules! f {
	() => {
	CString::new((*fi.next().unwrap()).clone())
	.unwrap()
	.as_ptr()
	};
	}
	macro_rules! v {
	() => {
	vi.next().unwrap()
	};
	}
	// This convoluted code is necessary since Redis only exposes a varargs API for HashGet
	// to modules. Unfortunately there's no straightforward or portable way of calling a
	// a varargs function with a variable number of arguments that is determined at runtime.
	// See also the following Redis ticket: https://github.com/redis/redis/issues/7860
	let res = Status::from(match fields.len() {
	0 => rm! {},
	1 => rm! {f!() v!()},
	2 => rm! {f!() v!() f!() v!()},
	3 => rm! {f!() v!() f!() v!() f!() v!()},
	4 => rm! {f!() v!() f!() v!() f!() v!() f!() v!()},
	5 => rm! {f!() v!() f!() v!() f!() v!() f!() v!() f!() v!()},
	6 => rm! {f!() v!() f!() v!() f!() v!() f!() v!() f!() v!() f!() v!()},
	7 => rm! {
	f!() v!() f!() v!() f!() v!() f!() v!() f!() v!() f!() v!()
	f!() v!()
	},
	8 => rm! {
	f!() v!() f!() v!() f!() v!() f!() v!() f!() v!() f!() v!()
	f!() v!() f!() v!()
	},
	9 => rm! {
	f!() v!() f!() v!() f!() v!() f!() v!() f!() v!() f!() v!()
	f!() v!() f!() v!() f!() v!()
	},
	10 => rm! {
	f!() v!() f!() v!() f!() v!() f!() v!() f!() v!() f!() v!()
	f!() v!() f!() v!() f!() v!() f!() v!()
	},
	11 => rm! {
	f!() v!() f!() v!() f!() v!() f!() v!() f!() v!() f!() v!()
	f!() v!() f!() v!() f!() v!() f!() v!() f!() v!()
	},
	12 => rm! {
	f!() v!() f!() v!() f!() v!() f!() v!() f!() v!() f!() v!()
	f!() v!() f!() v!() f!() v!() f!() v!() f!() v!() f!() v!()
	},
	_ => panic!("Unsupported length"),
	});
	match res {
	Status::Ok => Ok(()),
	Status::Err => Err(RedisError::Str("ERR key is not a hash value")),
	}
	}
	#[allow(clippy::not_unsafe_ptr_arg_deref)]
	pub fn hash_set(key: *mut RedisModuleKey, field: &str, value: *mut RedisModuleString) -> Status {
	let field = CString::new(field).unwrap();
	unsafe {
	RedisModule_HashSet.unwrap()(
	key,
	REDISMODULE_HASH_CFIELDS as i32,
	field.as_ptr(),
	value,
	ptr::null::<c_char>(),
	)
	.into()
	}
	}
	#[allow(clippy::not_unsafe_ptr_arg_deref)]
	pub fn hash_del(key: *mut RedisModuleKey, field: &str) -> Status {
	let field = CString::new(field).unwrap();
	// TODO: Add hash_del_multi()
	// Support to pass multiple fields is desired but is complicated.
	// See hash_get_multi() and https://github.com/redis/redis/issues/7860
	unsafe {
	RedisModule_HashSet.unwrap()(
	key,
	REDISMODULE_HASH_CFIELDS as i32,
	field.as_ptr(),
	REDISMODULE_HASH_DELETE,
	ptr::null::<c_char>(),
	)
	.into()
	}
	}
	#[allow(clippy::not_unsafe_ptr_arg_deref)]
	pub fn list_push(
	key: *mut RedisModuleKey,
	list_where: Where,
	element: *mut RedisModuleString,
	) -> Status {
	unsafe { RedisModule_ListPush.unwrap()(key, list_where as i32, element).into() }
	}
	#[allow(clippy::not_unsafe_ptr_arg_deref)]
	pub fn list_pop(key: *mut RedisModuleKey, list_where: Where) -> *mut RedisModuleString {
	unsafe { RedisModule_ListPop.unwrap()(key, list_where as i32) }
	}
	// Returns pointer to the C string, and sets len to its length
	#[allow(clippy::not_unsafe_ptr_arg_deref)]
	pub fn string_ptr_len(s: *const RedisModuleString, len: *mut size_t) -> *const c_char {
	unsafe { RedisModule_StringPtrLen.unwrap()(s, len) }
	}
	#[allow(clippy::not_unsafe_ptr_arg_deref)]
	pub fn string_retain_string(ctx: *mut RedisModuleCtx, s: *mut RedisModuleString) {
	unsafe { RedisModule_RetainString.unwrap()(ctx, s) }
	}
	#[allow(clippy::not_unsafe_ptr_arg_deref)]
	pub fn string_to_longlong(s: *const RedisModuleString, len: *mut i64) -> Status {
	unsafe { RedisModule_StringToLongLong.unwrap()(s, len).into() }
	}
	#[allow(clippy::not_unsafe_ptr_arg_deref)]
	pub fn string_to_double(s: *const RedisModuleString, len: *mut f64) -> Status {
	unsafe { RedisModule_StringToDouble.unwrap()(s, len).into() }
	}
	#[allow(clippy::not_unsafe_ptr_arg_deref)]
	pub fn string_set(key: *mut RedisModuleKey, s: *mut RedisModuleString) -> Status {
	unsafe { RedisModule_StringSet.unwrap()(key, s).into() }
	}
	#[allow(clippy::not_unsafe_ptr_arg_deref)]
	pub fn replicate_verbatim(ctx: *mut RedisModuleCtx) -> Status {
	unsafe { RedisModule_ReplicateVerbatim.unwrap()(ctx).into() }
	}
	fn load<F, T>(rdb: *mut RedisModuleIO, f: F) -> Result<T, Error>
	where
	F: FnOnce(*mut RedisModuleIO) -> T,
	{
	let res = f(rdb);
	if is_io_error(rdb) {
	Err(RedisError::short_read().into())
	} else {
	Ok(res)
	}
	}
	#[allow(clippy::not_unsafe_ptr_arg_deref)]
	pub fn load_unsigned(rdb: *mut RedisModuleIO) -> Result<u64, Error> {
	unsafe { load(rdb, |rdb| RedisModule_LoadUnsigned.unwrap()(rdb)) }
	}
	#[allow(clippy::not_unsafe_ptr_arg_deref)]
	pub fn load_signed(rdb: *mut RedisModuleIO) -> Result<i64, Error> {
	unsafe { load(rdb, |rdb| RedisModule_LoadSigned.unwrap()(rdb)) }
	}
	#[allow(clippy::not_unsafe_ptr_arg_deref)]
	pub fn load_string(rdb: *mut RedisModuleIO) -> Result<RedisString, Error> {
	let p = unsafe { load(rdb, |rdb| RedisModule_LoadString.unwrap()(rdb))? };
	let ctx = unsafe { RedisModule_GetContextFromIO.unwrap()(rdb) };
	Ok(RedisString::from_redis_module_string(ctx, p))
	}
	#[allow(clippy::not_unsafe_ptr_arg_deref)]
	pub fn load_string_buffer(rdb: *mut RedisModuleIO) -> Result<RedisBuffer, Error> {
	unsafe {
	let mut len = 0;
	let buffer = load(rdb, |rdb| {
	RedisModule_LoadStringBuffer.unwrap()(rdb, &mut len)
	})?;
	Ok(RedisBuffer::new(buffer, len))
	}
	}
	#[allow(clippy::not_unsafe_ptr_arg_deref)]
	pub fn replicate(ctx: *mut RedisModuleCtx, command: &str, args: &[&str]) -> Status {
	let terminated_args: Vec<RedisString> =
	args.iter().map(|s| RedisString::create(ctx, s)).collect();
	let inner_args: Vec<*mut RedisModuleString> = terminated_args.iter().map(|s| s.inner).collect();
	let cmd = CString::new(command).unwrap();
	unsafe {
	RedisModule_Replicate.unwrap()(
	ctx,
	cmd.as_ptr(),
	FMT,
	inner_args.as_ptr() as *mut c_char,
	terminated_args.len(),
	)
	.into()
	}
	}
	#[allow(clippy::not_unsafe_ptr_arg_deref)]
	pub fn load_double(rdb: *mut RedisModuleIO) -> Result<f64, Error> {
	unsafe { load(rdb, |rdb| RedisModule_LoadDouble.unwrap()(rdb)) }
	}
	#[allow(clippy::not_unsafe_ptr_arg_deref)]
	pub fn load_float(rdb: *mut RedisModuleIO) -> Result<f32, Error> {
	unsafe { load(rdb, |rdb| RedisModule_LoadFloat.unwrap()(rdb)) }
	}
	#[allow(clippy::not_unsafe_ptr_arg_deref)]
	pub fn save_string(rdb: *mut RedisModuleIO, buf: &str) {
	unsafe { RedisModule_SaveStringBuffer.unwrap()(rdb, buf.as_ptr().cast::<c_char>(), buf.len()) };
	}
	#[allow(clippy::not_unsafe_ptr_arg_deref)]
	pub fn save_double(rdb: *mut RedisModuleIO, val: f64) {
	unsafe { RedisModule_SaveDouble.unwrap()(rdb, val) };
	}
	#[allow(clippy::not_unsafe_ptr_arg_deref)]
	pub fn save_signed(rdb: *mut RedisModuleIO, val: i64) {
	unsafe { RedisModule_SaveSigned.unwrap()(rdb, val) };
	}
	#[allow(clippy::not_unsafe_ptr_arg_deref)]
	pub fn save_float(rdb: *mut RedisModuleIO, val: f32) {
	unsafe { RedisModule_SaveFloat.unwrap()(rdb, val) };
	}
	#[allow(clippy::not_unsafe_ptr_arg_deref)]
	pub fn save_unsigned(rdb: *mut RedisModuleIO, val: u64) {
	unsafe { RedisModule_SaveUnsigned.unwrap()(rdb, val) };
	}
	#[allow(clippy::not_unsafe_ptr_arg_deref)]
	pub fn string_append_buffer(
	ctx: *mut RedisModuleCtx,
	s: *mut RedisModuleString,
	buff: &str,
	) -> Status {
	unsafe {
	RedisModule_StringAppendBuffer.unwrap()(ctx, s, buff.as_ptr() as *mut c_char, buff.len())
	.into()
	}
	}
	#[allow(clippy::not_unsafe_ptr_arg_deref)]
	pub fn subscribe_to_server_event(
	ctx: *mut RedisModuleCtx,
	event: RedisModuleEvent,
	callback: RedisModuleEventCallback,
	) -> Status {
	unsafe { RedisModule_SubscribeToServerEvent.unwrap()(ctx, event, callback).into() }
	}
	#[allow(clippy::not_unsafe_ptr_arg_deref)]
	pub fn register_info_function(ctx: *mut RedisModuleCtx, callback: RedisModuleInfoFunc) -> Status {
	unsafe { RedisModule_RegisterInfoFunc.unwrap()(ctx, callback).into() }
	}
	#[allow(clippy::not_unsafe_ptr_arg_deref)]
	pub fn add_info_section(
	ctx: *mut RedisModuleInfoCtx,
	name: Option<&str>, // assume NULL terminated
	) -> Status {
	let name = name.map(|n| CString::new(n).unwrap());
	if let Some(n) = name {
	unsafe { RedisModule_InfoAddSection.unwrap()(ctx, n.as_ptr() as *mut c_char).into() }
	} else {
	unsafe { RedisModule_InfoAddSection.unwrap()(ctx, ptr::null_mut()).into() }
	}
	}

There are more instances of supressions of this lint in this repository: https://github.com/RedisLabsModules/redismodule-rs/search?q=not_unsafe_ptr_arg_deref

Even if they were not exposed in the crate's public API, having unsound functions like that internally would be error prone.

At least one of these is available in the public API. Example safe rust code that triggers Undefined Behaviour:

redis_module::raw::call_reply_type(0usize as *mut _);

I found these after reading rust-lang/rust-clippy#7666 and searching all of github for code supressing this clippy lint.

[gavrie]

Agreed, good find.
The right thing to do is probably to remove the #[allow(clippy::not_unsafe_ptr_arg_deref)] and mark the functions as unsafe instead. Also, functions like call_reply_type should definitely not be in the public API and we should mark them as pub(crate).