Free filebuffer to prevent 16MB memory leak in ccxr_demuxer_close #1994
Conversation
|
@cfsmp3 pls review this too |
There's no need to tag me in every PR, I review them when I can... tagging me just adds noise, no need. |
cfsmp3
left a comment
cfsmp3
left a comment
There was a problem hiding this comment.
Review: Changes Requested
Thanks for identifying this memory leak! Your analysis is correct - the filebuffer is allocated with Rust's Box but never freed, causing a 16MB leak per demuxer instance.
However, the current fix is incomplete and leaves a path where undefined behavior can occur.
The Problem
There are two cleanup paths in the codebase:
- Normal path:
ccxr_demuxer_close()→ laterccx_demuxer_delete() - Error/early exit path:
ccx_demuxer_delete()directly (without close being called)
Your PR fixes path 1, but path 2 still has a critical bug.
In ccx_demuxer.c:325, the ccx_demuxer_delete() function does:
freep(&lctx->filebuffer);This calls C's free() on memory that was allocated by Rust's Box. This is undefined behavior - you cannot free Rust-allocated memory with C's free() because they may use different allocators.
Current State With Your PR
| Cleanup Path | What Happens | Result |
|---|---|---|
| Normal (close → delete) | Rust frees in close, C sees NULL in delete | ✓ Works |
| Error (delete without close) | C tries to free() Rust memory |
❌ Undefined behavior |
The Complete Solution
You need to ensure Rust-allocated memory is always freed by Rust, in all code paths.
Step 1: Create ccxr_demuxer_delete in Rust
Add this to src/rust/src/libccxr_exports/demuxer.rs:
/// # Safety
/// This function must be called before the C code frees the demuxer struct.
/// It frees Rust-allocated resources that C cannot safely free.
#[unsafe(no_mangle)]
pub unsafe extern "C" fn ccxr_demuxer_delete(ctx: *mut ccx_demuxer) {
if ctx.is_null() {
return;
}
let c = &mut *ctx;
// Free Rust-allocated filebuffer
if \!c.filebuffer.is_null() {
use crate::file_functions::file::FILEBUFFERSIZE;
let _ = Box::from_raw(std::ptr::slice_from_raw_parts_mut(
c.filebuffer,
FILEBUFFERSIZE,
));
c.filebuffer = std::ptr::null_mut();
}
}Step 2: Modify ccx_demuxer_delete in C
In src/lib_ccx/ccx_demuxer.c, add the declaration and modify ccx_demuxer_delete:
// Add declaration near top of file (around line 8)
void ccxr_demuxer_delete(struct ccx_demuxer *ctx);
// Modify ccx_demuxer_delete function
void ccx_demuxer_delete(struct ccx_demuxer **ctx)
{
struct ccx_demuxer *lctx = *ctx;
int i;
#ifndef DISABLE_RUST
// Let Rust free any memory it allocated
ccxr_demuxer_delete(lctx);
#endif
dinit_cap(lctx);
freep(&lctx->last_pat_payload);
// ... existing cleanup code ...
#ifdef DISABLE_RUST
// Only free filebuffer in pure C mode - Rust handles its own memory
freep(&lctx->filebuffer);
#else
// Rust already freed this in ccxr_demuxer_delete, just clear the pointer
lctx->filebuffer = NULL;
#endif
freep(ctx);
}Step 3: Remove filebuffer cleanup from ccxr_demuxer_close
The filebuffer should be freed in ccxr_demuxer_delete, not in ccxr_demuxer_close. The close function should only close the file descriptor. Remove the filebuffer cleanup code you added to ccxr_demuxer_close.
Why This Matters
The delete function is called during program cleanup. If there's an error or early exit, close might not be called, but delete will be. By putting the Rust memory cleanup in ccxr_demuxer_delete, we ensure:
- All code paths are safe - whether close is called or not
- No allocator mismatch - Rust always frees Rust-allocated memory
- No double-free - the pointer is set to NULL after freeing
- Backwards compatible - pure C mode (DISABLE_RUST) still works
Summary
The fix needs to be in ccxr_demuxer_delete, not ccxr_demuxer_close, because delete is the guaranteed cleanup path that always runs.
|
@cfsmp3 The PR now makes sure that Rust-allocated memory is properly freed in ccxr_demuxer_delete for all code paths, which prevents memory leaks and avoids undefined behavior when Rust and C interact as you suggested |
CCExtractor CI platform finished running the test files on linux. Below is a summary of the test results, when compared to test for commit 2028754...:
Your PR breaks these cases:
Congratulations: Merging this PR would fix the following tests:
It seems that not all tests were passed completely. This is an indication that the output of some files is not as expected (but might be according to you). Check the result page for more info. |
CCExtractor CI platform finished running the test files on windows. Below is a summary of the test results, when compared to test for commit 2028754...:
Your PR breaks these cases:
Congratulations: Merging this PR would fix the following tests:
It seems that not all tests were passed completely. This is an indication that the output of some files is not as expected (but might be according to you). Check the result page for more info. |
3 participants
In raising this pull request, I confirm the following (please check boxes):
My familiarity with the project is as follows (check one):
Description
fixes a critical memory leak in CCExtractor where the file buffer allocated for each demuxer instance is never freed, causing 16MB of memory to leak per instance. This occurs because Rust allocated the buffer and then "forgot" it so C code could use it, leaving it unmanaged.
Changes made
ccxr_demuxer_close, the raw file buffer pointer is reclaimed as a boxed slice and dropped to allow Rust to free the memory.NULLto prevent double-free issues if C code tries to clean it up.Fixes: #1993
Verification