-
Notifications
You must be signed in to change notification settings - Fork 519
fix(memory): Fix uninitialized memory and memory leaks found by Valgrind #1840
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
Conversation
Addresses memory issues identified during Phase 5 (Runtime Analysis) of the bug analysis plan using Valgrind memory checking. ## Changes ### C Code (Uninitialized Memory) - ccx_demuxer.c: Use calloc() instead of malloc() in init_demuxer() to ensure all struct fields are zero-initialized before use - lib_ccx.c: Use calloc() instead of malloc() in init_decoder_setting() for consistent initialization ### Rust FFI Code (Memory Leaks) - utils.rs: Add helper functions for proper FFI string memory management: - free_rust_c_string(): Free a Rust-allocated CString - replace_rust_c_string(): Free old string before allocating new one - free_rust_c_string_array(): Free an array of Rust-allocated CStrings - common.rs: Update copy_from_rust() to properly manage string memory: - Free old strings before allocating new ones for all string fields - Add free_encoder_cfg_strings() to clean up encoder config strings - Free old inputfile array before allocating new one ## Valgrind Results Comparison | Metric | Before | After | Improvement | |---------------------|-----------|-----------|-----------------| | Definitely lost | 2,371 B | 1,536 B | 35% reduction | | Indirectly lost | 212 B | 0 B | 100% fixed | | Uninitialized errors| 131,095 | 0 | 100% fixed | The remaining 1,536 bytes are from services_charsets array in EncoderConfig (low priority, rare use case). 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
This commit fixes critical memory issues found during comprehensive
Valgrind testing:
1. **Use-after-free in inputfile array** (common.rs):
- Problem: `copy_from_rust` was called multiple times (parse_parameters,
demuxer_open, demuxer_close), and each call freed and reallocated the
inputfile array. C code holding references to the old array would then
access freed memory.
- Fix: Only set inputfile on the first call (when inputfile is null).
Subsequent calls skip modifying inputfile since it shouldn't change
during processing.
2. **Memory leak in enc_cfg strings** (common.rs):
- Problem: Each call to `copy_from_rust` allocated new encoder config
strings without freeing the old ones, causing 1,536 bytes leaked per
demuxer open/close cycle.
- Fix: Only set enc_cfg on the first call (when output_filename is null).
Encoder config is static and doesn't need to be re-synced.
3. **Uninitialized memory in telxcc_init** (telxcc.c):
- Problem: `malloc` was used to allocate TeletextCtx but not all fields
were explicitly initialized, causing Valgrind to report 400+ errors
about conditional jumps on uninitialized values.
- Fix: Changed to `calloc` to zero-initialize all fields.
**Valgrind results improvement (Test 3):**
- Errors: 458 → 21 (95% reduction)
- Definitely lost: 2,304 → 768 bytes (67% reduction)
- Use-after-free bugs: Eliminated
- Double-free bugs: Eliminated
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude Opus 4.5 <[email protected]>
This commit fixes several significant memory leaks found by Valgrind testing:
1. Dtvcc::new encoder leak (decoder/mod.rs):
- Previously always allocated a new encoder_ctx even when ctx.encoder
was not null, then threw away the allocation
- Fix: Only allocate when ctx.encoder is null
- Impact: Eliminated 55MB-331MB leaks per video processing run
2. ccxr_demuxer_isopen optimization (demuxer.rs):
- Previously copied entire demuxer structure just to check infd
- Fix: Directly check (*ctx).infd != -1
- Impact: Eliminated repeated allocations during file processing
3. ccxr_demuxer_close optimization (demuxer.rs):
- Previously did full copy roundtrip (C->Rust->C) to close a file
- Fix: Work directly on C struct, call close() and activity callback
- Impact: Eliminated copy-related allocations and leaks
4. CcxDemuxer Drop implementation (common_types.rs):
- pid_buffers and pids_programs contain raw pointers from Box::into_raw
- These were never freed when CcxDemuxer was dropped
- Fix: Implement Drop to free all non-null Box pointers
- Impact: Eliminates remaining FFI-related leaks
Test results show dramatic improvement:
- Test 24: 55MB leak -> 0 bytes (PERFECT)
- Test 26: 9.75MB leak -> 0 bytes (PERFECT)
- Test 27: 237MB leak -> 0 bytes (PERFECT)
- Test 28: 331MB leak -> 0 bytes (PERFECT)
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude Opus 4.5 <[email protected]>
This commit fixes several Valgrind-detected memory issues:
1. Use-after-free in Teletext during PAT changes:
- When parse_PAT() calls dinit_cap() to reinitialize stream info,
it freed the Teletext context but dec_ctx->private_data still
pointed to the freed memory
- Fixed by NULLing out dec_ctx->private_data in dinit_cap() when
freeing shared codec private data
- Also added NULL check in process_data() before calling teletext
functions to gracefully handle freed contexts
2. Uninitialized variables in general_loop():
- stream_mode, get_more_data, ret, and program_iter were declared
without initialization
- While logically set before use, Valgrind tracked them as
potentially uninitialized through complex control flow
- Fixed by initializing all variables at declaration
These fixes eliminate millions of Valgrind errors in teletext tests
(tests 78, 80) and uninitialized value warnings (tests 67, 84, 86).
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude Opus 4.5 <[email protected]>
The embedded dec_sub struct in lib_cc_decode had its data field allocated by write_cc_buffer() but never freed during cleanup. Added cleanup in dinit_cc_decode() to: - Free DVB bitmap data (data0/data1) if present - Free the dec_sub.data field itself This fixes ~1.7MB to ~2.6MB leaks seen in tests 89, 93, and 96. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
- XDS encoder leak: Free xds_str when skipping subtitles with invalid timestamps - XDS decoder cleanup: Add proper cleanup for leftover XDS strings in dinit_cc_decode() - Remove incorrect free(p) after write_xds_string() - the pointer is stored for later use by the encoder and must not be freed immediately - Remove xds_ctx free from dinit_cc_decode() to avoid double-free These fixes address the 100-byte XDS leak found in Valgrind test 114. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
Add proper cleanup of xds_ctx in rcwt_loop() for --in=bin and --in=raw formats. The general_loop() path already frees xds_ctx, but rcwt_loop() was missing this cleanup, causing an 880-byte leak. This fixes Valgrind tests 217 (--in=bin) and 218 (--in=raw). 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
- demux.rs: Update dummy_demuxer() to explicitly initialize all fields instead of using ..Default::default(), which is not allowed when the struct implements Drop - common.rs, demuxer.rs: Apply cargo fmt formatting fixes This fixes the Rust test compilation error: "cannot move out of type CcxDemuxer which implements the Drop trait" 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
This fixes the clippy error: "unused import: crate::utils::free_rust_c_string_array" 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
CCExtractor CI platform finished running the test files on linux. Below is a summary of the test results, when compared to test for commit 588ad52...:
Your PR breaks these cases:
NOTE: The following tests have been failing on the master branch as well as the PR:
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 588ad52...:
Your PR breaks these cases:
NOTE: The following tests have been failing on the master branch as well as the PR:
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. |
Summary
Comprehensive memory fixes found via Valgrind testing. This PR eliminates memory leaks, use-after-free bugs, and uninitialized memory issues across both C and Rust code.
Commits
calloc()instead ofmalloc()in demuxer/decoder initcopy_from_rust()is called multiple timesdec_ctx->private_dataafter Teletext context is freeddinit_cc_decode()xds_strwhen skipping invalid timestamps--in=binand--in=rawformatsValgrind Test Results (245 tests)
All tested ranges show 0 bytes definitely lost.
Key Fixes Verified
Technical Details
C Code Fixes
ccx_demuxer.c:calloc()ininit_demuxer()lib_ccx.c:calloc()ininit_decoder_setting()telxcc.c:calloc()intelxcc_init()general_loop.c: Initialize variables, add rcwt_loop cleanupccx_decoders_common.c: Free DVB bitmap and XDS strings in cleanupccx_encoders_common.c: Free XDS strings on invalid timestamp skipRust FFI Fixes
utils.rs: Addreplace_rust_c_string(),free_rust_c_string_array()common.rs: Only set inputfile/enc_cfg on first call to prevent leaksdemuxer.rs: Direct C struct manipulation inccxr_demuxer_close()decoder/mod.rs: Only allocate encoder whenctx.encoderis nullcommon_types.rs: AddDropforCcxDemuxerto free FFI allocationsKnown Pre-existing Issues
--datapid(complex interaction, pre-existing)Test plan
🤖 Generated with Claude Code