[FIX] webvtt: don't leak basefilename or skip the header flag when CSS creation fails - #2339
Open
pranayr710 wants to merge 1 commit into
Open
[FIX] webvtt: don't leak basefilename or skip the header flag when CSS creation fails#2339pranayr710 wants to merge 1 commit into
pranayr710 wants to merge 1 commit into
Conversation
…failure
write_webvtt_header() is called once per cue and guards re-entry with
context->wrote_webvtt_header, which it sets on its last line. The
--webvtt-create-css path returned early when the .css file could not be
created, bypassing that assignment, so the whole header block was written
again before every subsequent cue and the resulting .vtt was not valid
WebVTT. The existing trailing comment ("Do it even if couldn't write the
header, because it won't be possible anyway") already stated the intent.
Replace the early return with an else branch so the flag is always
reached, and move free(css_file_name) after it so both paths release it.
basefilename came from get_basename(), which allocates, but was never
freed on any path. Every other caller in the tree frees it. On its own
that is one small leak per output file; together with the early return it
became a leak per cue.
Also guard get_basename() returning NULL, matching the existing pattern in
ccx_encoders_common.c:636-641. This one is hardening rather than a
demonstrated crash: the Rust parser leaves first_input_file as an empty
string rather than NULL, so get_basename() currently returns "".
Refs CCExtractor#2338
Collaborator
CCExtractor CI platform finished running the test files on windows. 171/237 tests matched the approved output:
66 tests do not match the approved output. That is the pass/fail verdict. Whether this branch caused it is a separate question, answered below.
Compared with the tip of master — test 9525, commit 3875e84:
Compared with the commit this branch was cut from: the same run as the tip of master (test 9525), so the comparison above already covers it. No test changes behaviour relative to the tip of master: every failure above fails there too, byte for byte. The approved output for those tests is out of date, which is a baseline to review rather than a regression in this branch. |
Collaborator
CCExtractor CI platform finished running the test files on linux. 171/237 tests matched the approved output:
66 tests do not match the approved output. That is the pass/fail verdict. Whether this branch caused it is a separate question, answered below.
Compared with the tip of master — test 9528, commit 205cfb0:
Compared with the commit this branch was cut from: the same run as the tip of master (test 9528), so the comparison above already covers it. No test changes behaviour relative to the tip of master: every failure above fails there too, byte for byte. The approved output for those tests is out of date, which is a baseline to review rather than a regression in this branch. |
Author
|
Update: I've now executed the failure path, so the caveat in the PR description can go. It reproduces. I built master in Docker and ran it against a 153-byte RCWT input with three cues, with the base64 -d > sample.rcwt <<'EOF'
zMztzABQAAEAAADoAwAAAAAAAAsABJQgBJSuBJRwBEZJBFLTBFQgBEPBBNBUBElPBM6ABJQvuAsA
AAAAAAALAASUIASUrgSUcATTRQRDTwTOxAQgQwTB0ARUSQRPzgSUL4gTAAAAAAAACwAElCAElK4E
lHAEVMgESVIExCAEQ8EE0FQESU8EzoAElC9YGwAAAAAAAAEABJQs
EOF
docker run --rm -v $PWD/ro:/ro:ro -v $PWD/out:/out ccx:master /ro/sample.rcwt --out=webvtt --webvtt-create-css -o /out/ev.vttFour warnings for a three-cue file — one per cue plus one from The visible damage in the output, with the blank lines marked: Each re-entry runs the One correction to my own report while I'm here: I originally described the corruption as the header block repeating. On the default path the header block is just a newline, so what a user actually sees is doubled blank lines rather than a repeated The |
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
2 participants
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.
Fixes #2338
Reason for this PR:
Sanity check:
free.What was wrong
write_webvtt_header()is called once per cue (fromwrite_stringz_as_webvtt,write_cc_bitmap_as_webvttandwrite_cc_buffer_as_webvtt) and guards re-entry withcontext->wrote_webvtt_header, which it sets on its final line:The
--webvtt-create-csspath returned early when the.cssfile could not be opened, skipping that assignment:So on a CSS-creation failure the header block gets written again before every later cue, and the output stops being valid WebVTT. The comment quoted above already says the flag was meant to be set regardless.
Separately,
basefilenamecame fromget_basename(), whichmallocs, and was never freed on any path. Every other caller in the tree frees it (ccx_encoders_common.c:607,:636,:1369,:1437). Alone that's one small leak per output file; combined with the early return it became a leak per cue.The change
return→elsebranch, sowrote_webvtt_header = 1is always reached and there is still only one assignment to it.free(css_file_name)moved after the branch so both paths release it.free(basefilename)added once the name has been formatted.get_basename()NULL return guarded, mirroringccx_encoders_common.c:636-641.No signature, struct, option or output-format change. Behaviour on the success path is unchanged.
On the NULL guard
I want to be straight rather than oversell it: I could not establish that
first_input_fileis ever actuallyNULLat runtime —parser.rs:1618-1620leaves it as an empty string, soget_basename()returns"", notNULL. I included the guard because the identical call inccx_encoders_common.calready has one and the crash would be a plainstrlen(NULL), but it is hardening, not a fixed crash. Say the word and I'll drop it, leaving just the leak and the flag.Verification
gcc -Wall -fsyntax-only),clang-formatclean.returnthat skips an assignment, and an allocation with no matchingfree— so they're checkable by reading the twenty lines in the diff.fopen-failure path. I don't have a full local build, so I have not watched the duplicated header appear. Repro steps are in [BUG] write_webvtt_header(): --webvtt-create-css leaks basefilename, and re-emits the header before every cue when the CSS file can't be created #2338 (a read-only CWD is enough, since the.cssfopenuses the working directory rather than the output path). I'd rather flag that than imply I ran it.