Skip to content
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
2 changes: 2 additions & 0 deletions .github/workflows/main.yml
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,12 @@ on:
branches:
- 'master'
- 'main'
- 'nightly'
push:
branches:
- 'master'
- 'main'
- 'nightly'

jobs:
test:
Expand Down
2 changes: 1 addition & 1 deletion .gitmodules
Original file line number Diff line number Diff line change
@@ -1,3 +1,3 @@
[submodule "jemalloc-sys/jemalloc"]
path = jemalloc-sys/jemalloc
url = https://github.com/tikv/jemalloc
url = https://github.com/jemalloc/jemalloc
7 changes: 4 additions & 3 deletions jemalloc-ctl/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -192,7 +192,8 @@ option! {
/// `jemalloc` epoch.
///
/// Many of the statistics tracked by `jemalloc` are cached. The epoch
/// controls when they are refreshed.
/// controls when they are refreshed. Both `write` and `update` are the
/// same as calling `advance()` and ignore any provided value.
///
/// # Example
///
Expand All @@ -217,14 +218,14 @@ option! {
}

impl epoch {
/// Advances the epoch returning its old value - see [`epoch`].
/// Advances the epoch returning its latest value - see [`epoch`].
pub fn advance() -> crate::error::Result<u64> {
Self::update(1)
}
}

impl epoch_mib {
/// Advances the epoch returning its old value - see [`epoch`].
/// Advances the epoch returning its latest value - see [`epoch`].
pub fn advance(self) -> crate::error::Result<u64> {
self.0.update(1)
}
Expand Down
118 changes: 62 additions & 56 deletions jemalloc-ctl/src/macros.rs
Original file line number Diff line number Diff line change
Expand Up @@ -60,31 +60,6 @@ macro_rules! r {
self.0.read()
}
}

#[cfg(test)]
#[test]
#[allow(unused)]
fn [<$id _read_test>]() {
match stringify!($id) {
"background_thread" |
"max_background_threads"
if cfg!(target_os = "macos") => return,
_ => (),
}

let a = $id::read().unwrap();

let mib = $id::mib().unwrap();
let b = mib.read().unwrap();

#[cfg(feature = "use_std")]
println!(
concat!(
stringify!($id),
" (read): \"{}\" - \"{}\""),
a, b
);
}
}
};
}
Expand All @@ -108,31 +83,6 @@ macro_rules! w {
self.0.write(value)
}
}

#[cfg(test)]
#[test]
fn [<$id _write_test>]() {
match stringify!($id) {
"background_thread" |
"max_background_threads"
if cfg!(target_os = "macos") => return,
_ => (),
}

let _ = $id::write($ret_ty::default()).unwrap();

let mib = $id::mib().unwrap();
let _ = mib.write($ret_ty::default()).unwrap();

#[cfg(feature = "use_std")]
println!(
concat!(
stringify!($id),
" (write): \"{}\""),
$ret_ty::default()
);

}
}
};
}
Expand All @@ -156,29 +106,83 @@ macro_rules! u {
self.0.update(value)
}
}
}
};
}

macro_rules! make_test {
($id:ident, $ret_ty:ty, ()) => {};
(max_background_threads, $ret_ty:ty, ($($ops:ident),+)) => {
make_test!(max_background_threads, $ret_ty, |_| 1, $($ops),+);
};
(epoch, $ret_ty:ty, ($($ops:ident),+)) => {
make_test!(epoch, $ret_ty, |k| k + 1, $($ops),+);
};
($id:ident, $ret_ty:ty, ($($ops:ident),+)) => {
make_test!($id, $ret_ty, |_| Default::default(), $($ops),+);
};
($id:ident, $ret_ty:ty, $test_val:expr, r,w,u) => {
paste::paste! {
#[cfg(test)]
#[test]
#[allow(unused)]
fn [<$id _update_test>]() {
fn [<$id _read_write_update_test>]() {
match stringify!($id) {
"background_thread" |
"max_background_threads"
if cfg!(target_os = "macos") => return,
_ => (),
}

let a = $id::update($ret_ty::default()).unwrap();
let a = $id::read().unwrap();
let b = $test_val(a);
let _ = $id::write(b).unwrap();
let c = $id::read().unwrap();
assert_eq!(b, c);
let d = $id::update(a).unwrap();
let e = $id::read().unwrap();
if stringify!($id) == "epoch" {
assert_eq!(d, e);
assert_ne!(a, e);
} else {
assert_eq!(d, c);
assert_eq!(a, e);
}

let mib = $id::mib().unwrap();
let b = mib.update($ret_ty::default()).unwrap();
let f = mib.read().unwrap();
assert_eq!(e, f);
let g = $test_val(f);
let _ = mib.write(g).unwrap();
let h = mib.read().unwrap();
assert_eq!(g, h);
let i = mib.update(f).unwrap();
let j = mib.read().unwrap();
if stringify!($id) == "epoch" {
assert_eq!(i, j);
assert_ne!(f, j);
} else {
assert_eq!(i, h);
assert_eq!(f, j);
}
}
}
};
($id:ident, $ret_ty:ty, $test_val:expr, r) => {
paste::paste! {
#[cfg(test)]
#[test]
fn [<$id _read_test>]() {
let a = $id::read().unwrap();
let mib = $id::mib().unwrap();
let b = mib.read().unwrap();
assert_eq!(a, b);

#[cfg(feature = "use_std")]
println!(
concat!(
stringify!($id),
" (update): (\"{}\", \"{}\") - \"{}\""),
a, b, $ret_ty::default()
" (read): \"{}\" - \"{}\""),
a, b
);
}
}
Expand All @@ -202,6 +206,8 @@ macro_rules! option {
$(
$ops!($id => $ret_ty);
)*

make_test!($id, $ret_ty, ($($ops),*));
};
// Non-string option:
($id:ident[ str: $byte_string:expr, non_str: $mib_len:expr ] => $ret_ty:ty |
Expand Down
50 changes: 25 additions & 25 deletions jemalloc-ctl/src/raw.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,10 @@
//! Raw `unsafe` access to the `malloctl` API.

use crate::error::{cvt, Result};
use crate::{mem, ptr, slice};
use crate::{
mem::{self, MaybeUninit},
ptr, slice,
};
use libc::c_char;

/// Translates `name` to a `mib` (Management Information Base)
Expand Down Expand Up @@ -64,18 +67,18 @@ pub fn name_to_mib(name: &[u8], mib: &mut [usize]) -> Result<()> {
/// sizes of `bool` and `u8` match, but `bool` cannot represent all values that
/// `u8` can.
pub unsafe fn read_mib<T: Copy>(mib: &[usize]) -> Result<T> {
let mut value = MaybeUninit { init: () };
let mut value = MaybeUninit::<T>::uninit();
let mut len = mem::size_of::<T>();
cvt(tikv_jemalloc_sys::mallctlbymib(
mib.as_ptr(),
mib.len(),
&mut value.init as *mut _ as *mut _,
value.as_mut_ptr() as *mut _,
&mut len,
ptr::null_mut(),
0,
))?;
assert_eq!(len, mem::size_of::<T>());
Ok(value.maybe_uninit)
Ok(value.assume_init())
}

/// Uses the null-terminated string `name` as key to the _MALLCTL NAMESPACE_ and
Expand All @@ -90,17 +93,17 @@ pub unsafe fn read_mib<T: Copy>(mib: &[usize]) -> Result<T> {
pub unsafe fn read<T: Copy>(name: &[u8]) -> Result<T> {
validate_name(name);

let mut value = MaybeUninit { init: () };
let mut value = MaybeUninit::<T>::uninit();
let mut len = mem::size_of::<T>();
cvt(tikv_jemalloc_sys::mallctl(
name as *const _ as *const c_char,
&mut value.init as *mut _ as *mut _,
value.as_mut_ptr() as *mut _,
&mut len,
ptr::null_mut(),
0,
))?;
assert_eq!(len, mem::size_of::<T>());
Ok(value.maybe_uninit)
Ok(value.assume_init())
}

/// Uses the MIB `mib` as key to the _MALLCTL NAMESPACE_ and writes its `value`.
Expand Down Expand Up @@ -158,18 +161,19 @@ pub unsafe fn write<T>(name: &[u8], mut value: T) -> Result<()> {
/// invalid `T`, for example, by passing `T=u8` for a key expecting `bool`. The
/// sizes of `bool` and `u8` match, but `bool` cannot represent all values that
/// `u8` can.
pub unsafe fn update_mib<T>(mib: &[usize], mut value: T) -> Result<T> {
let mut len = mem::size_of::<T>();
pub unsafe fn update_mib<T: Copy>(mib: &[usize], mut value: T) -> Result<T> {
let mut old_len = mem::size_of::<T>();
let mut old_value = MaybeUninit::<T>::uninit();
cvt(tikv_jemalloc_sys::mallctlbymib(
mib.as_ptr(),
mib.len(),
old_value.as_mut_ptr() as *mut _,
&mut old_len,
&mut value as *mut _ as *mut _,
&mut len,
&mut value as *mut _ as *mut _,
len,
mem::size_of::<T>(),
))?;
assert_eq!(len, mem::size_of::<T>());
Ok(value)
assert_eq!(old_len, mem::size_of::<T>());
Ok(old_value.assume_init())
}

/// Uses the null-terminated string `name` as key to the _MALLCTL NAMESPACE_ and
Expand All @@ -184,16 +188,17 @@ pub unsafe fn update_mib<T>(mib: &[usize], mut value: T) -> Result<T> {
pub unsafe fn update<T>(name: &[u8], mut value: T) -> Result<T> {
validate_name(name);

let mut len = mem::size_of::<T>();
let mut old_len = mem::size_of::<T>();
let mut old_value = MaybeUninit::<T>::uninit();
cvt(tikv_jemalloc_sys::mallctl(
name as *const _ as *const c_char,
old_value.as_mut_ptr() as *mut _,
&mut old_len,
&mut value as *mut _ as *mut _,
&mut len,
&mut value as *mut _ as *mut _,
len,
mem::size_of::<T>(),
))?;
assert_eq!(len, mem::size_of::<T>());
Ok(value)
assert_eq!(old_len, mem::size_of::<T>());
Ok(old_value.assume_init())
Comment on lines 188 to +201
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Inspect the two update signatures side-by-side.
sed -n '160,205p' jemalloc-ctl/src/raw.rs

# Scan for obvious explicit instantiations that would rely on a non-Copy T.
rg -nP --type=rust '\bupdate::<\s*(String|Vec<|Box<|PathBuf|CString)' .

Repository: tikv/jemallocator

Length of output: 1770


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Find all calls to the update() function
rg -nP --type=rust '\bupdate\(' jemalloc-ctl/src/ | head -20

# Also check for update::<T> generic instantiations with any type
rg -nP --type=rust 'update::<' . | head -20

Repository: tikv/jemallocator

Length of output: 1742


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Look at the full keys.rs file to understand type parameters
cat -n jemalloc-ctl/src/keys.rs

Repository: tikv/jemallocator

Length of output: 16436


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Get the complete raw.rs file
wc -l jemalloc-ctl/src/raw.rs

# Then read it in sections
head -400 jemalloc-ctl/src/raw.rs | tail -250

Repository: tikv/jemallocator

Length of output: 10115


Add the T: Copy bound to the string-path update function to match update_mib.

The update function on line 188 currently lacks the T: Copy bound that update_mib (line 164) enforces. Both functions perform the same unsafe operation via mallctl/mallctlbymib and share identical safety documentation regarding construction of arbitrary types from raw bytes. The two entry points should enforce the same bounds.

All current call sites pass Copy types (u32, u64, isize, usize, u8, and pointers), so adding this bound is not breaking.

Suggested fix
-pub unsafe fn update<T>(name: &[u8], mut value: T) -> Result<T> {
+pub unsafe fn update<T: Copy>(name: &[u8], mut value: T) -> Result<T> {
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
pub unsafe fn update<T>(name: &[u8], mut value: T) -> Result<T> {
validate_name(name);
let mut len = mem::size_of::<T>();
let mut old_len = mem::size_of::<T>();
let mut old_value = MaybeUninit::<T>::uninit();
cvt(tikv_jemalloc_sys::mallctl(
name as *const _ as *const c_char,
old_value.as_mut_ptr() as *mut _,
&mut old_len,
&mut value as *mut _ as *mut _,
&mut len,
&mut value as *mut _ as *mut _,
len,
mem::size_of::<T>(),
))?;
assert_eq!(len, mem::size_of::<T>());
Ok(value)
assert_eq!(old_len, mem::size_of::<T>());
Ok(old_value.assume_init())
pub unsafe fn update<T: Copy>(name: &[u8], mut value: T) -> Result<T> {
validate_name(name);
let mut old_len = mem::size_of::<T>();
let mut old_value = MaybeUninit::<T>::uninit();
cvt(tikv_jemalloc_sys::mallctl(
name as *const _ as *const c_char,
old_value.as_mut_ptr() as *mut _,
&mut old_len,
&mut value as *mut _ as *mut _,
mem::size_of::<T>(),
))?;
assert_eq!(old_len, mem::size_of::<T>());
Ok(old_value.assume_init())
}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@jemalloc-ctl/src/raw.rs` around lines 188 - 201, The update function should
enforce the same T: Copy bound as update_mib to prevent constructing non-Copy
types from raw bytes; modify the signature of pub unsafe fn update<T>(...) to
pub unsafe fn update<T: Copy>(...) and ensure any uses of T in the function
(old_value, value, sizes) remain the same so the unsafe mallctl call and the
assert_eq!(old_len, mem::size_of::<T>()) still compile; this makes update
consistent with update_mib and the function’s safety contract.

}

/// Uses the MIB `mib` as key to the _MALLCTL NAMESPACE_ and reads its value.
Expand Down Expand Up @@ -376,11 +381,6 @@ fn validate_name(name: &[u8]) {
);
}

union MaybeUninit<T: Copy> {
init: (),
maybe_uninit: T,
}

#[cfg(test)]
mod tests {
use super::*;
Expand Down
2 changes: 1 addition & 1 deletion jemalloc-sys/Cargo.toml
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
[package]
name = "tikv-jemalloc-sys"
version = "0.6.1+5.3.0-1-ge13ca993e8ccb9ba9847cc330696e02839f328f7"
version = "0.6.1+5.3.1-0-g81034ce1f1373e37dc865038e1bc8eeecf559ce8"
authors = [
"Alex Crichton <alex@alexcrichton.com>",
"Gonzalo Brito Gadeschi <gonzalobg88@gmail.com>",
Expand Down
2 changes: 1 addition & 1 deletion jemalloc-sys/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@
[jemalloc_docs]: http://jemalloc.net/jemalloc.3.html
[jemalloc_wiki]: https://github.com/jemalloc/jemalloc/wiki

**Current jemalloc version**: 5.2.1.
**Current jemalloc version**: 5.3.1.

## Platform support

Expand Down
Loading
Loading