Skip to content

[FIX] Fix memory leak in free_sub_track() for blockaddition#2249

Closed
Shiv0087 wants to merge 1 commit intoCCExtractor:masterfrom
Shiv0087:patch-1
Closed

[FIX] Fix memory leak in free_sub_track() for blockaddition#2249
Shiv0087 wants to merge 1 commit intoCCExtractor:masterfrom
Shiv0087:patch-1

Conversation

@Shiv0087
Copy link
Copy Markdown

[FIX] Fix memory leak in free_sub_track() for blockaddition

Summary

Fixes memory leak in free_sub_track() where
sentence->blockaddition and its associated memory
were not freed.

Fixes #2247

Root Cause

block_addition structures allocated during parsing
were not released during cleanup, leading to memory leaks
for WebVTT subtitle tracks with BlockAdditions.

Changes

  • Added null-checked free for sentence->blockaddition
  • Ensured proper cleanup inside sentence destruction loop

Testing

-- Verified logic via code inspection

  • Ensured all allocated memory paths are freed in cleanup

In raising this pull request, I confirm the following:

Reason for this PR:

  • This PR adds new functionality.
  • This PR fixes a bug that I have personally experienced or that a real user has reported and for which a sample exists.
  • This PR is porting code from C to Rust.

Sanity check:

  • I have read and understood the contributors guide.
  • I have checked that another pull request for this purpose does not exist.
  • If the PR adds new functionality, I've added it to the changelog. If it's just a bug fix, I have NOT added it to the changelog.
  • I am NOT adding new C code unless it's to fix an existing, reproducible bug.

Repro instructions

Process an .mkv file containing WebVTT subtitle tracks with BlockAdditions.
Run with a memory analysis tool (e.g., Valgrind) and observe memory leaks
before the fix. After applying the fix, the allocated memory for blockaddition
structures is properly released.

Copilot AI review requested due to automatic review settings March 31, 2026 17:19
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Fixes a memory leak in Matroska/WebVTT subtitle track cleanup by attempting to free sentence->blockaddition and its backing buffer when destroying a subtitle track (related to issue #2247).

Changes:

  • Updates free_sub_track() to free per-sentence blockaddition data (and its message buffer) during cleanup.
  • Adds null checks around the new frees to avoid freeing null pointers.
Comments suppressed due to low confidence (1)

src/lib_ccx/matroska.c:1922

  • The for loop opened at line 1901 is never closed before freeing track->sentences and track, so track->sentences/track are freed on the first iteration and the function’s braces become unbalanced (this should also fail to compile). Close the for loop before freeing the track-level allocations, and ensure there is a final } to close free_sub_track() after the loop/cleanup.

	if (track->sentences != NULL)
		free(track->sentences);
	free(track);
}

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +1907 to +1916
if (sentence->blockaddition != NULL)
{
/* cue_settings_list is the base of the message buffer;
* cue_identifier and comment are pointers into it */
if (sentence->blockaddition->cue_settings_list != NULL)
{
free(sentence->blockaddition->cue_settings_list);
}
free(sentence->blockaddition);
}
Copy link

Copilot AI Mar 31, 2026

Choose a reason for hiding this comment

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

Freeing sentence->blockaddition->cue_settings_list assumes that pointer always equals the start of the allocated message buffer. In parse_segment_cluster_block_group_block_additions(), cue_settings_list is only set when the first item has non-zero length; if it’s empty, the allocated message buffer has no owning pointer and this cleanup will still leak (and you can’t safely free() cue_identifier/comment because they may be offset into the buffer). Consider storing the original message pointer in struct block_addition (e.g., char *raw_message) and freeing that unconditionally, or always retaining message base even when the first field is empty.

Copilot uses AI. Check for mistakes.
@cfsmp3
Copy link
Copy Markdown
Contributor

cfsmp3 commented Apr 4, 2026

Closing — the diff has a missing closing brace for the for loop, which means free(track->sentences) and free(track) are no longer inside the correct scope. This is a logic error / build break.

Also, the indentation changed from tabs to spaces and the loop body lost its indentation level entirely.

The idea of freeing blockaddition and cue_settings_list is correct — feel free to resubmit with:

  1. The missing } restored
  2. Consistent tab indentation matching the rest of the file
  3. Verify it compiles cleanly before submitting

@cfsmp3 cfsmp3 closed this Apr 4, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] Memory leak in free_sub_track(): blockaddition and message buffer never freed for WebVTT tracks

3 participants