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

fix custom_key_cmp_wrapper being able to unwind to C code (ub) #275

Merged
merged 2 commits into from
Aug 18, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 0 additions & 9 deletions heed-traits/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -42,11 +42,6 @@ pub trait BytesDecode<'a> {
/// with shorter keys collating before longer keys.
pub trait Comparator {
/// Compares the raw bytes representation of two keys.
///
/// # Safety
///
/// This function must never crash, this is the reason why it takes raw bytes as parameter,
/// to let you define the recovery method you want in case of a decoding error.
fn compare(a: &[u8], b: &[u8]) -> Ordering;
}

Expand All @@ -61,10 +56,6 @@ pub trait Comparator {
pub trait LexicographicComparator: Comparator {
/// Compare a single byte; this function is used to implement [`Comparator::compare`]
/// by definition of lexicographic ordering.
///
/// # Safety
///
/// This function must never crash.
fn compare_elem(a: u8, b: u8) -> Ordering;

/// Advances the given `elem` to its immediate lexicographic successor, if possible.
Expand Down
19 changes: 14 additions & 5 deletions heed/src/env.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,9 @@ use std::os::unix::{
ffi::OsStrExt,
io::{AsRawFd, BorrowedFd, RawFd},
};
use std::panic::catch_unwind;
use std::path::{Path, PathBuf};
use std::process::abort;
use std::sync::{Arc, RwLock};
use std::time::Duration;
#[cfg(windows)]
Expand Down Expand Up @@ -371,16 +373,23 @@ impl Drop for EnvInner {

/// A helper function that transforms the LMDB types into Rust types (`MDB_val` into slices)
/// and vice versa, the Rust types into C types (`Ordering` into an integer).
extern "C" fn custom_key_cmp_wrapper<C: Comparator>(
///
/// # Safety
///
/// `a` and `b` should both properly aligned, valid for reads and should point to a valid
/// [`MDB_val`][ffi::MDB_val]. An [`MDB_val`][ffi::MDB_val] (consists of a pointer and size) is
/// valid when its pointer (`mv_data`) is valid for reads of `mv_size` bytes and is not null.
unsafe extern "C" fn custom_key_cmp_wrapper<C: Comparator>(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why have you added unsafe to this function? Isn't it safe now?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It still dereferences raw pointers. Should I add a /// # Safety doc comment with some explanation? It is a private function though.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ho! Yeah! You're right. It's a good idea to add a safety note.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added the safety note

a: *const ffi::MDB_val,
b: *const ffi::MDB_val,
) -> i32 {
let a = unsafe { ffi::from_val(*a) };
let b = unsafe { ffi::from_val(*b) };
match C::compare(a, b) {
Ordering::Less => -1,
Ordering::Equal => 0,
Ordering::Greater => 1,
match catch_unwind(|| C::compare(a, b)) {
Ok(Ordering::Less) => -1,
Ok(Ordering::Equal) => 0,
Ok(Ordering::Greater) => 1,
Err(_) => abort(),
}
}

Expand Down
Loading