xz crate#8
Merged
youknowone merged 3 commits intomainfrom Apr 25, 2026
Merged
Conversation
WalkthroughThis PR renames the primary crate from Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/publish.yml:
- Around line 23-24: The tag condition for the "Publish xz crate" step is too
broad (startsWith(github.ref, 'refs/tags/xz')) and will match tags like
refs/tags/xz-core-*, causing wrong publishes; update the if condition around the
step that references github.ref and startsWith to either explicitly exclude
xz-core (add && !startsWith(github.ref, 'refs/tags/xz-core')) or tighten the
match to the root crate tag pattern (e.g., use a stricter startsWith like
'refs/tags/xz-' combined with a version marker or use a delimiter such as
'refs/tags/xz@' or 'refs/tags/xz-v' to only match the intended root xz tags).
In `@xz-sys/src/lib.rs`:
- Around line 70-74: The helper unsafe fn normalize_c_stream_allocator mutates
(*strm).allocator, but three query functions (lzma_memlimit_get, lzma_memusage,
lzma_get_check) currently cast their *const lzma_stream to *mut and call
normalize_c_stream_allocator, violating FFI const-correctness; fix by removing
those casts and the calls to normalize_c_stream_allocator from each of those
three query functions so they only read from the stream, or alternatively change
the call sites to use a non-mutating/read-only normalization variant if mutation
is truly required elsewhere; locate calls to normalize_c_stream_allocator in the
implementations of lzma_memlimit_get, lzma_memusage, and lzma_get_check and
eliminate the cast-to-*mut and the normalize call.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3eca78c4-1d87-46ab-aad5-0e66955b2ffc
📒 Files selected for processing (24)
.github/workflows/main.yml.github/workflows/publish.ymlCargo.lock.msrvCargo.tomlREADME.mdbenches/backend_comparison.rsdocs/performance-workflow.mdexamples/bufread_trailing_probe.rsexamples/qc_probe.rsexamples/standard_files_probe.rsperf-probe/src/main.rsscripts/compare_all_trimmed.shscripts/compare_backends.shscripts/inspect_codegen.shscripts/run_root_test_bins.shsrc/lib.rssrc/stream.rstests/drop-incomplete.rstests/xz.rsxz-core/src/alloc.rsxz-core/src/common/common.rsxz-sys/Cargo.tomlxz-sys/src/c_allocator.rsxz-sys/src/lib.rs
💤 Files with no reviewable changes (2)
- tests/xz.rs
- xz-sys/src/c_allocator.rs
Comment on lines
+23
to
+24
| - if: "!startsWith(github.ref, 'refs/tags/liblzma-sys') && !startsWith(github.ref, 'refs/tags/xz-sys') && startsWith(github.ref, 'refs/tags/xz')" | ||
| name: Publish xz crate |
There was a problem hiding this comment.
Tag condition leaks to xz-core (and any future xz* crate).
startsWith(github.ref, 'refs/tags/xz') matches refs/tags/xz-core-* (and e.g. xz-foo-*). Only xz-sys is explicitly excluded, so pushing an xz-core tag would erroneously trigger publication of the root xz crate from this step. Either exclude xz-core as well, or tighten the prefix to match only the root crate's tag scheme (e.g. xz-v / xz@).
🔧 Proposed fix (exclude xz-core explicitly)
- - if: "!startsWith(github.ref, 'refs/tags/liblzma-sys') && !startsWith(github.ref, 'refs/tags/xz-sys') && startsWith(github.ref, 'refs/tags/xz')"
+ - if: "!startsWith(github.ref, 'refs/tags/liblzma-sys') && !startsWith(github.ref, 'refs/tags/xz-sys') && !startsWith(github.ref, 'refs/tags/xz-core') && startsWith(github.ref, 'refs/tags/xz')"
name: Publish xz crateAlternatively, lock the prefix to an unambiguous root-crate tag pattern:
- - if: "!startsWith(github.ref, 'refs/tags/liblzma-sys') && !startsWith(github.ref, 'refs/tags/xz-sys') && startsWith(github.ref, 'refs/tags/xz')"
+ - if: "startsWith(github.ref, 'refs/tags/xz-v')"📝 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
| - if: "!startsWith(github.ref, 'refs/tags/liblzma-sys') && !startsWith(github.ref, 'refs/tags/xz-sys') && startsWith(github.ref, 'refs/tags/xz')" | |
| name: Publish xz crate | |
| - if: "!startsWith(github.ref, 'refs/tags/liblzma-sys') && !startsWith(github.ref, 'refs/tags/xz-sys') && !startsWith(github.ref, 'refs/tags/xz-core') && startsWith(github.ref, 'refs/tags/xz')" | |
| name: Publish xz crate |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/publish.yml around lines 23 - 24, The tag condition for
the "Publish xz crate" step is too broad (startsWith(github.ref,
'refs/tags/xz')) and will match tags like refs/tags/xz-core-*, causing wrong
publishes; update the if condition around the step that references github.ref
and startsWith to either explicitly exclude xz-core (add &&
!startsWith(github.ref, 'refs/tags/xz-core')) or tighten the match to the root
crate tag pattern (e.g., use a stricter startsWith like 'refs/tags/xz-' combined
with a version marker or use a delimiter such as 'refs/tags/xz@' or
'refs/tags/xz-v' to only match the intended root xz tags).
Comment on lines
+70
to
+74
| unsafe fn normalize_c_stream_allocator(strm: *mut lzma_stream) { | ||
| if !strm.is_null() && (*strm).allocator.is_null() { | ||
| (*strm).allocator = xz_core::alloc::c_allocator_ptr().cast(); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n -C3 'normalize_c_stream_allocator\(strm as \*mut lzma_stream\)|pub unsafe extern "C" fn lzma_(memlimit_get|memusage|get_check)' xz-sys/src/lib.rsRepository: youknowone/xz-rs
Length of output: 815
🏁 Script executed:
rg -n 'normalize_c_stream_allocator' xz-sys/src/lib.rsRepository: youknowone/xz-rs
Length of output: 1338
🏁 Script executed:
sed -n '224,230p; 268,274p; 275,280p' xz-sys/src/lib.rsRepository: youknowone/xz-rs
Length of output: 720
Don't normalize through *const lzma_stream query wrappers.
Lines 227, 271, 277 cast *const lzma_stream to *mut before calling normalize_c_stream_allocator, which writes to (*strm).allocator. This mutates read-only caller memory and violates FFI const-correctness. The three query functions (lzma_memlimit_get, lzma_memusage, lzma_get_check) only read from the stream and don't need allocator normalization.
Suggested fix
#[unsafe(no_mangle)]
pub unsafe extern "C" fn lzma_memlimit_get(strm: *const lzma_stream) -> u64 {
- normalize_c_stream_allocator(strm as *mut lzma_stream);
xz_core::common::common::lzma_memlimit_get(strm.cast())
}
#[unsafe(no_mangle)]
pub unsafe extern "C" fn lzma_memusage(strm: *const lzma_stream) -> u64 {
- normalize_c_stream_allocator(strm as *mut lzma_stream);
xz_core::common::common::lzma_memusage(strm.cast())
}
#[unsafe(no_mangle)]
pub unsafe extern "C" fn lzma_get_check(strm: *const lzma_stream) -> lzma_check {
- normalize_c_stream_allocator(strm as *mut lzma_stream);
xz_core::common::common::lzma_get_check(strm.cast())
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@xz-sys/src/lib.rs` around lines 70 - 74, The helper unsafe fn
normalize_c_stream_allocator mutates (*strm).allocator, but three query
functions (lzma_memlimit_get, lzma_memusage, lzma_get_check) currently cast
their *const lzma_stream to *mut and call normalize_c_stream_allocator,
violating FFI const-correctness; fix by removing those casts and the calls to
normalize_c_stream_allocator from each of those three query functions so they
only read from the stream, or alternatively change the call sites to use a
non-mutating/read-only normalization variant if mutation is truly required
elsewhere; locate calls to normalize_c_stream_allocator in the implementations
of lzma_memlimit_get, lzma_memusage, and lzma_get_check and eliminate the
cast-to-*mut and the normalize call.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
README.md (1)
13-23:⚠️ Potential issue | 🟡 MinorAlign the migration snippet with the renamed package version.
This README now documents the
xzcrate whose manifest version is0.4.6, but the migration example tells users to depend onxz = "0.1.7". Either update the snippet to the current series or explicitly mark it as legacy 0.1-only guidance.📝 Proposed wording if the current crate line should be advertised
-**This crate is forked from [xz2](https://crates.io/crates/xz2) and `xz = "0.1.x"` is fully compatible with `xz2 = "0.1.7"`,** -so you can migrate simply. +**This crate is forked from [xz2](https://crates.io/crates/xz2) and provides an xz2-compatible high-level API,** +so most users can migrate by changing the dependency and import path. @@ -xz2 = "0.1.7" -+xz = "0.1.7" ++xz = "0.4"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 13 - 23, The migration example in the README shows adding xz = "0.1.7" which is inconsistent with this crate's current version series; update the snippet (the Cargo.toml diff lines referencing xz2 and xz) to advertise the current crate series (e.g., replace xz = "0.1.7" with xz = "0.4.6") or, if you intend to show legacy guidance, add an explicit note that the snippet is for the 0.1.x legacy compatibility only; modify the two lines in the example that change the dependency from "xz2 = \"0.1.7\"" to the desired current version string or add the legacy clarification text.
♻️ Duplicate comments (1)
xz-sys/src/lib.rs (1)
226-278:⚠️ Potential issue | 🟠 MajorDon’t normalize through
*const lzma_streamquery wrappers.These read-only wrappers still cast
*const lzma_streamto*mutbefore callingnormalize_c_stream_allocator, which can writestrm.allocatorand violates FFI const-correctness.Suggested fix
#[unsafe(no_mangle)] pub unsafe extern "C" fn lzma_memlimit_get(strm: *const lzma_stream) -> u64 { - normalize_c_stream_allocator(strm as *mut lzma_stream); xz_core::common::common::lzma_memlimit_get(strm.cast()) } @@ #[unsafe(no_mangle)] pub unsafe extern "C" fn lzma_memusage(strm: *const lzma_stream) -> u64 { - normalize_c_stream_allocator(strm as *mut lzma_stream); xz_core::common::common::lzma_memusage(strm.cast()) } @@ #[unsafe(no_mangle)] pub unsafe extern "C" fn lzma_get_check(strm: *const lzma_stream) -> lzma_check { - normalize_c_stream_allocator(strm as *mut lzma_stream); xz_core::common::common::lzma_get_check(strm.cast()) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@xz-sys/src/lib.rs` around lines 226 - 278, The read-only C wrappers lzma_memlimit_get, lzma_memusage, and lzma_get_check (which accept *const lzma_stream) currently cast the pointer to *mut and call normalize_c_stream_allocator, which can mutate strm. Replace those calls with a non-mutating normalization helper (e.g., normalize_c_stream_allocator_const that accepts *const lzma_stream and only reads/validates without writing) or remove the normalization if unnecessary; update lzma_memlimit_get, lzma_memusage, and lzma_get_check to call that const-safe helper (or no-op) instead of normalize_c_stream_allocator to preserve FFI const-correctness.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@xz-core/src/alloc.rs`:
- Around line 371-476: The allocator callbacks must be treated as an
all-or-nothing pair: only call a custom alloc or free if both alloc and free
callbacks are present (and allocator is non-null); otherwise always use the
fallback (c_alloc_*/c_alloc_zeroed_bytes or rust_alloc_impl/rust_free_impl).
Update lzma_alloc_bytes, lzma_alloc_zeroed_bytes, lzma_free_ptr,
internal_alloc_bytes, internal_alloc_object, internal_alloc_array,
internal_alloc_zeroed_array, and internal_free so their checks require allocator
!= null && let Some(alloc) = unsafe { (*allocator).alloc } && let Some(free) =
unsafe { (*allocator).free } (or equivalent) before using the custom callbacks;
if that condition fails, fall back to the existing c_/rust_ implementations.
Ensure the same paired-check logic is used everywhere so allocations and frees
never cross allocator boundaries.
---
Outside diff comments:
In `@README.md`:
- Around line 13-23: The migration example in the README shows adding xz =
"0.1.7" which is inconsistent with this crate's current version series; update
the snippet (the Cargo.toml diff lines referencing xz2 and xz) to advertise the
current crate series (e.g., replace xz = "0.1.7" with xz = "0.4.6") or, if you
intend to show legacy guidance, add an explicit note that the snippet is for the
0.1.x legacy compatibility only; modify the two lines in the example that change
the dependency from "xz2 = \"0.1.7\"" to the desired current version string or
add the legacy clarification text.
---
Duplicate comments:
In `@xz-sys/src/lib.rs`:
- Around line 226-278: The read-only C wrappers lzma_memlimit_get,
lzma_memusage, and lzma_get_check (which accept *const lzma_stream) currently
cast the pointer to *mut and call normalize_c_stream_allocator, which can mutate
strm. Replace those calls with a non-mutating normalization helper (e.g.,
normalize_c_stream_allocator_const that accepts *const lzma_stream and only
reads/validates without writing) or remove the normalization if unnecessary;
update lzma_memlimit_get, lzma_memusage, and lzma_get_check to call that
const-safe helper (or no-op) instead of normalize_c_stream_allocator to preserve
FFI const-correctness.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: d017f921-f6ef-4b36-8ca7-234d439aa108
📒 Files selected for processing (12)
Cargo.lock.msrvCargo.tomlREADME.mddocs/performance-workflow.mdscripts/inspect_codegen.shsrc/lib.rsxz-core/Cargo.tomlxz-core/src/alloc.rsxz-core/src/common/common.rsxz-sys/Cargo.tomlxz-sys/src/c_allocator.rsxz-sys/src/lib.rs
💤 Files with no reviewable changes (1)
- xz-sys/src/c_allocator.rs
Comment on lines
+371
to
+476
| pub(crate) unsafe fn lzma_alloc_bytes( | ||
| size: size_t, | ||
| allocator: *const lzma_allocator, | ||
| ) -> *mut c_void { | ||
| let size = c_size(size); | ||
| if !allocator.is_null() | ||
| && let Some(alloc) = unsafe { (*allocator).alloc } | ||
| { | ||
| return unsafe { alloc((*allocator).opaque, 1, size) }; | ||
| } | ||
| return ptr; | ||
| unsafe { c_alloc_bytes(size) } | ||
| } | ||
| rust_alloc_impl(size, core::mem::align_of::<T>(), true) as *mut T | ||
| } | ||
|
|
||
| pub(crate) unsafe fn internal_free(ptr: *mut c_void, allocator: *const lzma_allocator) { | ||
| if !allocator.is_null() | ||
| && let Some(free) = unsafe { (*allocator).free } | ||
| { | ||
| unsafe { free((*allocator).opaque, ptr) }; | ||
| return; | ||
| pub(crate) unsafe fn lzma_alloc_zeroed_bytes( | ||
| size: size_t, | ||
| allocator: *const lzma_allocator, | ||
| ) -> *mut c_void { | ||
| let size = c_size(size); | ||
| if !allocator.is_null() | ||
| && let Some(alloc) = unsafe { (*allocator).alloc } | ||
| { | ||
| let ptr = unsafe { alloc((*allocator).opaque, 1, size) }; | ||
| if !ptr.is_null() { | ||
| unsafe { core::ptr::write_bytes(ptr as *mut u8, 0, size) }; | ||
| } | ||
| return ptr; | ||
| } | ||
| unsafe { c_alloc_zeroed_bytes(size) } | ||
| } | ||
|
|
||
| pub(crate) unsafe fn lzma_free_ptr(ptr: *mut c_void, allocator: *const lzma_allocator) { | ||
| if !allocator.is_null() | ||
| && let Some(free) = unsafe { (*allocator).free } | ||
| { | ||
| unsafe { free((*allocator).opaque, ptr) }; | ||
| return; | ||
| } | ||
| unsafe { c_free_ptr(ptr) }; | ||
| } | ||
|
|
||
| pub(crate) unsafe fn internal_alloc_bytes( | ||
| size: size_t, | ||
| allocator: *const lzma_allocator, | ||
| ) -> *mut c_void { | ||
| let size = c_size(size); | ||
| let allocator = allocator_or_rust(allocator); | ||
| if let Some(alloc) = unsafe { (*allocator).alloc } { | ||
| return unsafe { alloc((*allocator).opaque, 1, size) }; | ||
| } | ||
| rust_alloc_impl(size as usize, RUST_ALLOC_ALIGN, false) | ||
| } | ||
|
|
||
| pub(crate) unsafe fn internal_alloc_object<T>(allocator: *const lzma_allocator) -> *mut T { | ||
| if !allocator.is_null() | ||
| && let Some(alloc) = unsafe { (*allocator).alloc } | ||
| { | ||
| return unsafe { | ||
| alloc((*allocator).opaque, 1, core::mem::size_of::<T>() as size_t) as *mut T | ||
| }; | ||
| } | ||
| rust_alloc_impl(core::mem::size_of::<T>(), core::mem::align_of::<T>(), false) as *mut T | ||
| } | ||
|
|
||
| pub(crate) unsafe fn internal_alloc_array<T>( | ||
| count: size_t, | ||
| allocator: *const lzma_allocator, | ||
| ) -> *mut T { | ||
| let Some(size) = (count as usize).checked_mul(core::mem::size_of::<T>()) else { | ||
| return core::ptr::null_mut(); | ||
| }; | ||
| if !allocator.is_null() | ||
| && let Some(alloc) = unsafe { (*allocator).alloc } | ||
| { | ||
| return unsafe { alloc((*allocator).opaque, 1, size as size_t) as *mut T }; | ||
| } | ||
| rust_alloc_impl(size, core::mem::align_of::<T>(), false) as *mut T | ||
| } | ||
|
|
||
| pub(crate) unsafe fn internal_alloc_zeroed_array<T>( | ||
| count: size_t, | ||
| allocator: *const lzma_allocator, | ||
| ) -> *mut T { | ||
| let Some(size) = (count as usize).checked_mul(core::mem::size_of::<T>()) else { | ||
| return core::ptr::null_mut(); | ||
| }; | ||
| if !allocator.is_null() | ||
| && let Some(alloc) = unsafe { (*allocator).alloc } | ||
| { | ||
| let ptr = unsafe { alloc((*allocator).opaque, 1, size as size_t) as *mut T }; | ||
| if !ptr.is_null() { | ||
| unsafe { core::ptr::write_bytes(ptr as *mut u8, 0, size) }; | ||
| } | ||
| return ptr; | ||
| } | ||
| rust_alloc_impl(size, core::mem::align_of::<T>(), true) as *mut T | ||
| } | ||
|
|
||
| pub(crate) unsafe fn internal_free(ptr: *mut c_void, allocator: *const lzma_allocator) { | ||
| if !allocator.is_null() | ||
| && let Some(free) = unsafe { (*allocator).free } | ||
| { | ||
| unsafe { free((*allocator).opaque, ptr) }; | ||
| return; | ||
| } | ||
| rust_free_impl(ptr); | ||
| } |
There was a problem hiding this comment.
Treat custom allocator callbacks as an all-or-nothing pair.
The active custom path chooses alloc and free independently. With a partial allocator, memory can be allocated by a custom callback and freed by c_free/rust_free_impl, or allocated by fallback code and freed by a custom callback. That crosses allocator boundaries and can corrupt memory.
Suggested direction
- if !allocator.is_null()
- && let Some(alloc) = unsafe { (*allocator).alloc }
+ if !allocator.is_null()
+ && let (Some(alloc), Some(_free)) =
+ unsafe { ((*allocator).alloc, (*allocator).free) }
{
return unsafe { alloc((*allocator).opaque, 1, size) };
}
@@
- if !allocator.is_null()
- && let Some(free) = unsafe { (*allocator).free }
+ if !allocator.is_null()
+ && let (Some(_alloc), Some(free)) =
+ unsafe { ((*allocator).alloc, (*allocator).free) }
{
unsafe { free((*allocator).opaque, ptr) };
return;
}Apply the same complete-pair check consistently across lzma_alloc_bytes, lzma_alloc_zeroed_bytes, lzma_free_ptr, internal_alloc_*, and internal_free.
📝 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(crate) unsafe fn lzma_alloc_bytes( | |
| size: size_t, | |
| allocator: *const lzma_allocator, | |
| ) -> *mut c_void { | |
| let size = c_size(size); | |
| if !allocator.is_null() | |
| && let Some(alloc) = unsafe { (*allocator).alloc } | |
| { | |
| return unsafe { alloc((*allocator).opaque, 1, size) }; | |
| } | |
| return ptr; | |
| unsafe { c_alloc_bytes(size) } | |
| } | |
| rust_alloc_impl(size, core::mem::align_of::<T>(), true) as *mut T | |
| } | |
| pub(crate) unsafe fn internal_free(ptr: *mut c_void, allocator: *const lzma_allocator) { | |
| if !allocator.is_null() | |
| && let Some(free) = unsafe { (*allocator).free } | |
| { | |
| unsafe { free((*allocator).opaque, ptr) }; | |
| return; | |
| pub(crate) unsafe fn lzma_alloc_zeroed_bytes( | |
| size: size_t, | |
| allocator: *const lzma_allocator, | |
| ) -> *mut c_void { | |
| let size = c_size(size); | |
| if !allocator.is_null() | |
| && let Some(alloc) = unsafe { (*allocator).alloc } | |
| { | |
| let ptr = unsafe { alloc((*allocator).opaque, 1, size) }; | |
| if !ptr.is_null() { | |
| unsafe { core::ptr::write_bytes(ptr as *mut u8, 0, size) }; | |
| } | |
| return ptr; | |
| } | |
| unsafe { c_alloc_zeroed_bytes(size) } | |
| } | |
| pub(crate) unsafe fn lzma_free_ptr(ptr: *mut c_void, allocator: *const lzma_allocator) { | |
| if !allocator.is_null() | |
| && let Some(free) = unsafe { (*allocator).free } | |
| { | |
| unsafe { free((*allocator).opaque, ptr) }; | |
| return; | |
| } | |
| unsafe { c_free_ptr(ptr) }; | |
| } | |
| pub(crate) unsafe fn internal_alloc_bytes( | |
| size: size_t, | |
| allocator: *const lzma_allocator, | |
| ) -> *mut c_void { | |
| let size = c_size(size); | |
| let allocator = allocator_or_rust(allocator); | |
| if let Some(alloc) = unsafe { (*allocator).alloc } { | |
| return unsafe { alloc((*allocator).opaque, 1, size) }; | |
| } | |
| rust_alloc_impl(size as usize, RUST_ALLOC_ALIGN, false) | |
| } | |
| pub(crate) unsafe fn internal_alloc_object<T>(allocator: *const lzma_allocator) -> *mut T { | |
| if !allocator.is_null() | |
| && let Some(alloc) = unsafe { (*allocator).alloc } | |
| { | |
| return unsafe { | |
| alloc((*allocator).opaque, 1, core::mem::size_of::<T>() as size_t) as *mut T | |
| }; | |
| } | |
| rust_alloc_impl(core::mem::size_of::<T>(), core::mem::align_of::<T>(), false) as *mut T | |
| } | |
| pub(crate) unsafe fn internal_alloc_array<T>( | |
| count: size_t, | |
| allocator: *const lzma_allocator, | |
| ) -> *mut T { | |
| let Some(size) = (count as usize).checked_mul(core::mem::size_of::<T>()) else { | |
| return core::ptr::null_mut(); | |
| }; | |
| if !allocator.is_null() | |
| && let Some(alloc) = unsafe { (*allocator).alloc } | |
| { | |
| return unsafe { alloc((*allocator).opaque, 1, size as size_t) as *mut T }; | |
| } | |
| rust_alloc_impl(size, core::mem::align_of::<T>(), false) as *mut T | |
| } | |
| pub(crate) unsafe fn internal_alloc_zeroed_array<T>( | |
| count: size_t, | |
| allocator: *const lzma_allocator, | |
| ) -> *mut T { | |
| let Some(size) = (count as usize).checked_mul(core::mem::size_of::<T>()) else { | |
| return core::ptr::null_mut(); | |
| }; | |
| if !allocator.is_null() | |
| && let Some(alloc) = unsafe { (*allocator).alloc } | |
| { | |
| let ptr = unsafe { alloc((*allocator).opaque, 1, size as size_t) as *mut T }; | |
| if !ptr.is_null() { | |
| unsafe { core::ptr::write_bytes(ptr as *mut u8, 0, size) }; | |
| } | |
| return ptr; | |
| } | |
| rust_alloc_impl(size, core::mem::align_of::<T>(), true) as *mut T | |
| } | |
| pub(crate) unsafe fn internal_free(ptr: *mut c_void, allocator: *const lzma_allocator) { | |
| if !allocator.is_null() | |
| && let Some(free) = unsafe { (*allocator).free } | |
| { | |
| unsafe { free((*allocator).opaque, ptr) }; | |
| return; | |
| } | |
| rust_free_impl(ptr); | |
| } | |
| pub(crate) unsafe fn lzma_alloc_bytes( | |
| size: size_t, | |
| allocator: *const lzma_allocator, | |
| ) -> *mut c_void { | |
| let size = c_size(size); | |
| if !allocator.is_null() | |
| && let (Some(alloc), Some(_free)) = unsafe { ((*allocator).alloc, (*allocator).free) } | |
| { | |
| return unsafe { alloc((*allocator).opaque, 1, size) }; | |
| } | |
| unsafe { c_alloc_bytes(size) } | |
| } | |
| pub(crate) unsafe fn lzma_alloc_zeroed_bytes( | |
| size: size_t, | |
| allocator: *const lzma_allocator, | |
| ) -> *mut c_void { | |
| let size = c_size(size); | |
| if !allocator.is_null() | |
| && let (Some(alloc), Some(_free)) = unsafe { ((*allocator).alloc, (*allocator).free) } | |
| { | |
| let ptr = unsafe { alloc((*allocator).opaque, 1, size) }; | |
| if !ptr.is_null() { | |
| unsafe { core::ptr::write_bytes(ptr as *mut u8, 0, size) }; | |
| } | |
| return ptr; | |
| } | |
| unsafe { c_alloc_zeroed_bytes(size) } | |
| } | |
| pub(crate) unsafe fn lzma_free_ptr(ptr: *mut c_void, allocator: *const lzma_allocator) { | |
| if !allocator.is_null() | |
| && let (Some(_alloc), Some(free)) = unsafe { ((*allocator).alloc, (*allocator).free) } | |
| { | |
| unsafe { free((*allocator).opaque, ptr) }; | |
| return; | |
| } | |
| unsafe { c_free_ptr(ptr) }; | |
| } | |
| pub(crate) unsafe fn internal_alloc_bytes( | |
| size: size_t, | |
| allocator: *const lzma_allocator, | |
| ) -> *mut c_void { | |
| let size = c_size(size); | |
| let allocator = allocator_or_rust(allocator); | |
| if let (Some(alloc), Some(_free)) = unsafe { ((*allocator).alloc, (*allocator).free) } { | |
| return unsafe { alloc((*allocator).opaque, 1, size) }; | |
| } | |
| rust_alloc_impl(size as usize, RUST_ALLOC_ALIGN, false) | |
| } | |
| pub(crate) unsafe fn internal_alloc_object<T>(allocator: *const lzma_allocator) -> *mut T { | |
| if !allocator.is_null() | |
| && let (Some(alloc), Some(_free)) = unsafe { ((*allocator).alloc, (*allocator).free) } | |
| { | |
| return unsafe { | |
| alloc((*allocator).opaque, 1, core::mem::size_of::<T>() as size_t) as *mut T | |
| }; | |
| } | |
| rust_alloc_impl(core::mem::size_of::<T>(), core::mem::align_of::<T>(), false) as *mut T | |
| } | |
| pub(crate) unsafe fn internal_alloc_array<T>( | |
| count: size_t, | |
| allocator: *const lzma_allocator, | |
| ) -> *mut T { | |
| let Some(size) = (count as usize).checked_mul(core::mem::size_of::<T>()) else { | |
| return core::ptr::null_mut(); | |
| }; | |
| if !allocator.is_null() | |
| && let (Some(alloc), Some(_free)) = unsafe { ((*allocator).alloc, (*allocator).free) } | |
| { | |
| return unsafe { alloc((*allocator).opaque, 1, size as size_t) as *mut T }; | |
| } | |
| rust_alloc_impl(size, core::mem::align_of::<T>(), false) as *mut T | |
| } | |
| pub(crate) unsafe fn internal_alloc_zeroed_array<T>( | |
| count: size_t, | |
| allocator: *const lzma_allocator, | |
| ) -> *mut T { | |
| let Some(size) = (count as usize).checked_mul(core::mem::size_of::<T>()) else { | |
| return core::ptr::null_mut(); | |
| }; | |
| if !allocator.is_null() | |
| && let (Some(alloc), Some(_free)) = unsafe { ((*allocator).alloc, (*allocator).free) } | |
| { | |
| let ptr = unsafe { alloc((*allocator).opaque, 1, size as size_t) as *mut T }; | |
| if !ptr.is_null() { | |
| unsafe { core::ptr::write_bytes(ptr as *mut u8, 0, size) }; | |
| } | |
| return ptr; | |
| } | |
| rust_alloc_impl(size, core::mem::align_of::<T>(), true) as *mut T | |
| } | |
| pub(crate) unsafe fn internal_free(ptr: *mut c_void, allocator: *const lzma_allocator) { | |
| if !allocator.is_null() | |
| && let (Some(_alloc), Some(free)) = unsafe { ((*allocator).alloc, (*allocator).free) } | |
| { | |
| unsafe { free((*allocator).opaque, ptr) }; | |
| return; | |
| } | |
| rust_free_impl(ptr); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@xz-core/src/alloc.rs` around lines 371 - 476, The allocator callbacks must be
treated as an all-or-nothing pair: only call a custom alloc or free if both
alloc and free callbacks are present (and allocator is non-null); otherwise
always use the fallback (c_alloc_*/c_alloc_zeroed_bytes or
rust_alloc_impl/rust_free_impl). Update lzma_alloc_bytes,
lzma_alloc_zeroed_bytes, lzma_free_ptr, internal_alloc_bytes,
internal_alloc_object, internal_alloc_array, internal_alloc_zeroed_array, and
internal_free so their checks require allocator != null && let Some(alloc) =
unsafe { (*allocator).alloc } && let Some(free) = unsafe { (*allocator).free }
(or equivalent) before using the custom callbacks; if that condition fails, fall
back to the existing c_/rust_ implementations. Ensure the same paired-check
logic is used everywhere so allocations and frees never cross allocator
boundaries.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/performance-workflow.md (1)
97-101:⚠️ Potential issue | 🟠 MajorRemove the "mirrors tests/standard_files.rs" claim from line 97.
The file
tests/standard_files.rsdoes not exist in the repository. The "standard_files" workload is defined and driven directly byexamples/standard_files_probe.rs(as confirmed byscripts/compare_api_workloads.shwhich maps the "standard-files" workload argument to this example). Update the documentation to accurately reflect that the example defines the workload, rather than mirroring a nonexistent test file.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/performance-workflow.md` around lines 97 - 101, Remove the incorrect phrase that the example "mirrors the `tests/standard_files.rs` `standard_files` path" and update the sentence to state that `examples/standard_files_probe.rs` defines and drives the "standard_files" workload (as mapped by `scripts/compare_api_workloads.sh` / the "standard-files" argument) and that it writes reports to `target/perf-results/api-standard-files.json` and `target/perf-results/api-standard-files.md`; keep reference to `examples/standard_files_probe.rs` and the output paths intact but eliminate any mention of a nonexistent `tests/standard_files.rs`.
♻️ Duplicate comments (1)
xz-sys/src/lib.rs (1)
226-229:⚠️ Potential issue | 🟠 MajorRemove mutating allocator normalization from read-only stream query APIs.
Line 227, Line 271, and Line 277 cast
*const lzma_streamto mutable and may write through it vianormalize_c_stream_allocator. These wrappers are read-only by contract and this breaks FFI const-correctness.Proposed fix
#[unsafe(no_mangle)] pub unsafe extern "C" fn lzma_memlimit_get(strm: *const lzma_stream) -> u64 { - normalize_c_stream_allocator(strm as *mut lzma_stream); xz_core::common::common::lzma_memlimit_get(strm.cast()) } @@ #[unsafe(no_mangle)] pub unsafe extern "C" fn lzma_memusage(strm: *const lzma_stream) -> u64 { - normalize_c_stream_allocator(strm as *mut lzma_stream); xz_core::common::common::lzma_memusage(strm.cast()) } @@ #[unsafe(no_mangle)] pub unsafe extern "C" fn lzma_get_check(strm: *const lzma_stream) -> lzma_check { - normalize_c_stream_allocator(strm as *mut lzma_stream); xz_core::common::common::lzma_get_check(strm.cast()) }Also applies to: 270-273, 276-279
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@xz-sys/src/lib.rs` around lines 226 - 229, The wrapper lzma_memlimit_get currently casts the incoming *const lzma_stream to mutable and calls normalize_c_stream_allocator, which mutates the stream and breaks FFI const-correctness; remove that mutation by either calling the underlying xz_core::common::common::lzma_memlimit_get(strm.cast()) directly (no normalize call and no cast to *mut) or implement/use a non-mutating normalization helper (e.g., normalize_c_stream_allocator_const) that accepts *const lzma_stream; apply the same change pattern to the other read-only wrappers that call normalize_c_stream_allocator (the functions referenced in the diff/comments).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@xz-core/src/alloc.rs`:
- Around line 1-13: The build fails because alloc.rs declares submodules `mod
rust;`, `mod c;`, `mod custom;`, and `mod rust_only;` but the corresponding
files under xz-core/src/alloc/ are missing; either add the missing files
`rust.rs`, `c.rs`, `custom.rs`, and `rust_only.rs` in xz-core/src/alloc/ with
the expected public API used by this crate, or collapse the declarations into
inline modules inside alloc.rs (e.g., define `mod rust { ... }` etc.) and ensure
the conditional `use` alias `use custom as policy;` / `use rust_only as policy;`
still resolves to the same symbols; make sure the implementations exposed by
those modules match the functions/types referenced elsewhere so feature-gated
builds (with and without `custom_allocator`) compile.
In `@xz-core/src/common/stream_decoder_mt.rs`:
- Around line 71-95: The two functions worker_allocator and set_worker_allocator
duplicate the same feature-gated logic in stream_decoder_mt.rs and
stream_encoder_mt.rs for different worker_thread types; refactor to remove
copy/paste by extracting the shared behavior into a single reusable abstraction
(either a small trait like HasAllocator with methods allocator()/set_allocator()
implemented for each worker_thread type, or a macro that emits the two functions
for a given worker_thread type), then replace the per-file helper definitions
with calls to the common trait/macro implementation so worker_allocator,
set_worker_allocator, worker_thread and lzma_allocator usages remain identical
and the #[cfg(feature = "custom_allocator")] branches stay centralized.
---
Outside diff comments:
In `@docs/performance-workflow.md`:
- Around line 97-101: Remove the incorrect phrase that the example "mirrors the
`tests/standard_files.rs` `standard_files` path" and update the sentence to
state that `examples/standard_files_probe.rs` defines and drives the
"standard_files" workload (as mapped by `scripts/compare_api_workloads.sh` / the
"standard-files" argument) and that it writes reports to
`target/perf-results/api-standard-files.json` and
`target/perf-results/api-standard-files.md`; keep reference to
`examples/standard_files_probe.rs` and the output paths intact but eliminate any
mention of a nonexistent `tests/standard_files.rs`.
---
Duplicate comments:
In `@xz-sys/src/lib.rs`:
- Around line 226-229: The wrapper lzma_memlimit_get currently casts the
incoming *const lzma_stream to mutable and calls normalize_c_stream_allocator,
which mutates the stream and breaks FFI const-correctness; remove that mutation
by either calling the underlying
xz_core::common::common::lzma_memlimit_get(strm.cast()) directly (no normalize
call and no cast to *mut) or implement/use a non-mutating normalization helper
(e.g., normalize_c_stream_allocator_const) that accepts *const lzma_stream;
apply the same change pattern to the other read-only wrappers that call
normalize_c_stream_allocator (the functions referenced in the diff/comments).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: aac9f5eb-d00f-4af6-9168-b3b4cd72d659
📒 Files selected for processing (30)
Cargo.lock.msrvCargo.tomlREADME.mddocs/performance-workflow.mdscripts/inspect_codegen.shsrc/lib.rsxz-core/Cargo.tomlxz-core/src/alloc.rsxz-core/src/common/alone_decoder.rsxz-core/src/common/alone_encoder.rsxz-core/src/common/auto_decoder.rsxz-core/src/common/block_decoder.rsxz-core/src/common/block_encoder.rsxz-core/src/common/common.rsxz-core/src/common/file_info.rsxz-core/src/common/filter_decoder.rsxz-core/src/common/filter_encoder.rsxz-core/src/common/index_decoder.rsxz-core/src/common/index_encoder.rsxz-core/src/common/lzip_decoder.rsxz-core/src/common/microlzma_decoder.rsxz-core/src/common/microlzma_encoder.rsxz-core/src/common/stream_decoder.rsxz-core/src/common/stream_decoder_mt.rsxz-core/src/common/stream_encoder.rsxz-core/src/common/stream_encoder_mt.rsxz-core/src/types.rsxz-sys/Cargo.tomlxz-sys/src/c_allocator.rsxz-sys/src/lib.rs
💤 Files with no reviewable changes (1)
- xz-sys/src/c_allocator.rs
Comment on lines
+71
to
+95
| #[inline] | ||
| unsafe fn worker_allocator(thr: *const worker_thread) -> *const lzma_allocator { | ||
| #[cfg(feature = "custom_allocator")] | ||
| { | ||
| unsafe { (*thr).allocator } | ||
| } | ||
| #[cfg(not(feature = "custom_allocator"))] | ||
| { | ||
| let _ = thr; | ||
| core::ptr::null() | ||
| } | ||
| } | ||
| #[inline] | ||
| unsafe fn set_worker_allocator(thr: *mut worker_thread, allocator: *const lzma_allocator) { | ||
| #[cfg(feature = "custom_allocator")] | ||
| { | ||
| unsafe { | ||
| (*thr).allocator = allocator; | ||
| } | ||
| } | ||
| #[cfg(not(feature = "custom_allocator"))] | ||
| { | ||
| let _ = (thr, allocator); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Clean feature-gated indirection; consider deduping across encoder/decoder.
The worker_allocator/set_worker_allocator helpers correctly abstract the #[cfg(feature = "custom_allocator")] branches and the let _ = … absorptions avoid unused-arg warnings on the feature-off path. Usage is consistent: each worker stores its own allocator at init (Line 373) and all per-thread resources (in_0, block_decoder) are subsequently freed through worker_allocator(thr), which is strictly more robust than re-reading (*strm).allocator later.
Optional: these two helpers are duplicated verbatim in xz-core/src/common/stream_encoder_mt.rs (Lines 26–50). Only the worker_thread type differs. Factoring them through a small trait (trait HasAllocator { fn allocator(&self) -> *const lzma_allocator; … }) or a macro would remove the copy/paste without adding meaningful indirection.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@xz-core/src/common/stream_decoder_mt.rs` around lines 71 - 95, The two
functions worker_allocator and set_worker_allocator duplicate the same
feature-gated logic in stream_decoder_mt.rs and stream_encoder_mt.rs for
different worker_thread types; refactor to remove copy/paste by extracting the
shared behavior into a single reusable abstraction (either a small trait like
HasAllocator with methods allocator()/set_allocator() implemented for each
worker_thread type, or a macro that emits the two functions for a given
worker_thread type), then replace the per-file helper definitions with calls to
the common trait/macro implementation so worker_allocator, set_worker_allocator,
worker_thread and lzma_allocator usages remain identical and the #[cfg(feature =
"custom_allocator")] branches stay centralized.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
xz-core/src/alloc/custom.rs (1)
26-125:⚠️ Potential issue | 🔴 CriticalCustom
alloc/freecallbacks are still treated independently — cross-allocator free hazard.Each of
lzma_alloc,lzma_alloc_zero,lzma_free,internal_alloc_bytes,internal_alloc_object,internal_alloc_array,internal_alloc_zeroed_array, andinternal_freechecks only one of(*allocator).alloc/(*allocator).free. If a caller supplies a partiallzma_allocator(e.g.alloc=Some, free=None), allocations route through the user callback while the matching free falls back toc_free_ptr/rust_free_ptr(or vice versa), which crosses allocator boundaries and can corrupt memory.Gate every custom path on both callbacks being present, falling back to the policy backend otherwise. For example:
🔒 Suggested pair-check pattern
pub unsafe fn lzma_alloc(size: size_t, allocator: *const lzma_allocator) -> *mut c_void { let size = c_size(size); - if !allocator.is_null() - && let Some(alloc) = unsafe { (*allocator).alloc } - { - return unsafe { alloc((*allocator).opaque, 1, size) }; - } + if !allocator.is_null() + && let (Some(alloc), Some(_free)) = unsafe { ((*allocator).alloc, (*allocator).free) } + { + return unsafe { alloc((*allocator).opaque, 1, size) }; + } unsafe { c_alloc_bytes(size) } } @@ pub unsafe fn lzma_free(ptr: *mut c_void, allocator: *const lzma_allocator) { - if !allocator.is_null() - && let Some(free) = unsafe { (*allocator).free } - { - unsafe { free((*allocator).opaque, ptr) }; - return; - } + if !allocator.is_null() + && let (Some(_alloc), Some(free)) = unsafe { ((*allocator).alloc, (*allocator).free) } + { + unsafe { free((*allocator).opaque, ptr) }; + return; + } unsafe { c_free_ptr(ptr) }; }Apply the same paired check to
lzma_alloc_zero,internal_alloc_bytes,internal_alloc_object,internal_alloc_array,internal_alloc_zeroed_array, andinternal_freeso allocations and frees never cross backends.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@xz-core/src/alloc/custom.rs` around lines 26 - 125, The code currently uses a custom allocator if only one of (*allocator).alloc or (*allocator).free is present, causing cross-allocator frees; update each function (lzma_alloc, lzma_alloc_zero, lzma_free, internal_alloc_bytes, internal_alloc_object, internal_alloc_array, internal_alloc_zeroed_array, internal_free) to require the allocator to be non-null and both callbacks present before taking the custom path (i.e., check that unsafe { (*allocator).alloc }.is_some() AND unsafe { (*allocator).free }.is_some() for allocation and the same paired presence for frees), otherwise fall back to the existing policy backend calls (c_alloc_bytes / c_alloc_zeroed_bytes / c_free_ptr or rust_alloc_impl / rust_free_ptr) so allocations and frees always use the same backend.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@xz-core/src/alloc/custom.rs`:
- Around line 26-125: The code currently uses a custom allocator if only one of
(*allocator).alloc or (*allocator).free is present, causing cross-allocator
frees; update each function (lzma_alloc, lzma_alloc_zero, lzma_free,
internal_alloc_bytes, internal_alloc_object, internal_alloc_array,
internal_alloc_zeroed_array, internal_free) to require the allocator to be
non-null and both callbacks present before taking the custom path (i.e., check
that unsafe { (*allocator).alloc }.is_some() AND unsafe { (*allocator).free
}.is_some() for allocation and the same paired presence for frees), otherwise
fall back to the existing policy backend calls (c_alloc_bytes /
c_alloc_zeroed_bytes / c_free_ptr or rust_alloc_impl / rust_free_ptr) so
allocations and frees always use the same backend.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6e4bf12e-359f-47a6-9d76-69e342c769d4
📒 Files selected for processing (6)
xz-core/src/alloc.rsxz-core/src/alloc/c.rsxz-core/src/alloc/custom.rsxz-core/src/alloc/rust.rsxz-core/src/alloc/rust_only.rsxz-core/src/common/common.rs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
liblzmatoxzwith updated project metadata and documentation.xz-core(default),xz-sys, with fallback support forliblzma-sys.Documentation
xzcrate naming.Chores