[FIX] Segfault on files carrying both teletext and DVB subtitles - #2319
Merged
Conversation
update_encoder_list_cinfo() declares enc_ctx uninitialised and uses it as the
list_for_each_entry() iterator. For a DVB stream carrying a language, the search
loop continues past every encoder whose dvb_lang does not match, so it can finish
without returning. When the encoder list is non-empty none of the creation
branches below run either, and the function falls through to
enc_ctx->prev = NULL;
with enc_ctx still holding the value the loop terminated on: the list head
reinterpreted as an encoder_ctx. That pointer is written to, and later
general_loop.c does dvb_enc->timing = dvb_dec->timing on it, which segfaults.
It reproduces on any recording that carries both a teletext stream and a DVB
subtitle stream, in every output format, with no extra flags:
ccextractor --out=srt sample.mpg -> SIGSEGV
ccextractor --out=webvtt sample.ts -> SIGSEGV
Restricting to one PID (--datapid) avoids it, which is why the regression suite
never caught it: the only affected sample it runs passes --datapid.
Clear the iterator after the loop so "not found" is distinguishable, and give
such a stream its own per-language encoder, which is what the multi-DVB path
already does. The existing dvb_pid_count >= 2 test stays as an OR so a recording
whose only caption stream is a single DVB PID keeps its plain output filename --
that case reaches the standard path with an empty encoder list. A NULL guard
before the tail keeps the fall-through honest if some future path adds one.
Both streams are now extracted into separate, well-formed files (out.srt plus
out_eng.srt) rather than one file with two interleaved numberings.
Collaborator
CCExtractor CI platform finished running the test files on linux. Below is a summary of the test results, when compared to test for commit 6077cf5...:
Your PR breaks these cases:
NOTE: The following tests have been failing on the master branch as well as the PR:
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. |
Collaborator
CCExtractor CI platform finished running the test files on windows. Below is a summary of the test results, when compared to test for commit c328108...:
Your PR breaks these cases:
NOTE: The following tests have been failing on the master branch as well as the PR:
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. |
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.
ccextractor --out=srt sample.mpgsegfaults on any recording that carries both a teletext stream and a DVB subtitle stream. No special flags are needed and every output format is affected.Root cause
update_encoder_list_cinfo()declaresenc_ctxuninitialised and then uses it as thelist_for_each_entry()iterator. For a DVB stream carrying a language, the search loopcontinues past every encoder whosedvb_langdoes not match, so it can run to completion without returning. When the encoder list is non-empty, none of the creation branches below run either, and the function falls through towith
enc_ctxstill holding the value the loop terminated on — the list head reinterpreted as anencoder_ctx. That bogus pointer is written to and then returned.general_loop.c:1358later doesdvb_enc->timing = dvb_dec->timingon it and faults.Under gdb the faulting address is exactly
&dvb_enc->timing, and the instruction is a store:The condition arises whenever a DVB PID is not the stream the main path selected — i.e. a DVB stream sitting alongside teletext.
Why CI never caught it
Restricting to a single PID avoids it entirely, and the one affected sample in the regression suite (
85c7fc1ad7, sample 19) is only ever run as--datapid 5603 …. Plain extraction on the same file crashes.Fix
Clear the iterator after the search loop so "not found" is distinguishable from "found", and give such a stream its own per-language encoder — which is what the multi-DVB path already does for extra DVB PIDs. The existing
dvb_pid_count >= 2test is kept as anOR, so a recording whose only caption stream is a single DVB PID still reaches the standard path with an empty encoder list and keeps its plain output filename. ANULLguard before the tail keeps the fall-through honest.Both streams are now extracted into separate, well-formed files rather than being merged into one file with two interleaved cue numberings.
Verification
Scanned all 162 local media samples with a release build of master and of this branch:
85c7fc1ad7…mpgout.srt+out_eng.srtf1422b8bfe…tsout.srt+out_spa.srtEach produced file has monotonic, unique cue indices and monotonic timestamps.
Regression checks:
--out=srt, whole output directory compared).04e47919de59…tsstill yieldsout_chi.srt+out_chs.srt, byte-identical to master.out.srton both master and this branch.init_encoder(ccx_encoders_common.c:818) that master also has; this branch shows it twice simply because it now creates a second encoder.Not fixed here
A third sample,
5f6dfe831e35…wtv, also segfaults on master but through an unrelated path — an unsigned underflow inbuffered_seek()reachingbuffered_read(). That is a separate bug and will get its own PR.