Repository navigation
Conversation
parse_dvb_description() derived the language count from the descriptor length byte without limiting it to the size of the dvb_config arrays, and advanced the read cursor by a growing stride instead of a fixed entry width. - Clamp n_language to MAX_LANGUAGE_PER_DESC after the existing warning. Previously a descriptor longer than 40 bytes made the loop write past lang_index/sub_type/composition_id/ancillary_id, which are each five elements, corrupting the caller's stack frame in parse_PMT(). - Change the loop increment from data_ptr += i * 8 to data_ptr += 8. Because the comma operator evaluates i++ before the increment, the old form advanced the cursor quadratically (4*i*(i+1) bytes) and read past the descriptor buffer from the ninth entry onwards. Verified with an AddressSanitizer build of this translation unit: a 248-byte descriptor (n_language = 31) produced a stack-buffer-overflow write at the ancillary_id[6] store before the change and no diagnostics after it, with n_language clamped to 5.
5 of 7 tasks
Collaborator
CCExtractor CI platform finished running the test files on linux. 77/237 tests matched the approved output:
160 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 9744, commit 0bf2a94:
Pass there, fail here:
Fail on both but produce different output — behaviour moved even though the verdict did not:
Compared with the commit this branch was cut from — test 9750, commit d7eb132:
Pass there, fail here:
Fail on both but produce different output — behaviour moved even though the verdict did not:
This branch changes the behaviour of 38 test(s) relative to the tip of master. Those are the ones worth looking at; anything else in the list fails the same way on both sides. |
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 9693, commit af4b28d:
Fail on both but produce different output — behaviour moved even though the verdict did not:
Fail there, pass here:
Compared with the commit this branch was cut from: the same run as the tip of master (test 9693), so the comparison above already covers it. This branch changes the behaviour of 42 test(s) relative to the tip of master. Those are the ones worth looking at; anything else in the list fails the same way on both sides. |
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
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.
What
Two defects in
parse_dvb_description()(src/lib_ccx/dvb_subtitle_decoder.c), both reported in #2417 by @Haind03 with a full technical write-up: https://hackmd.io/@haindpwn/ccextractor-parse-dvb-description-stack-oob-write1. Unbounded language count → stack out-of-bounds write
cfg->n_language = len / 8was taken straight from the descriptor length byte, so with a length byte of 56 or moren_languagereached 7..31, whilelang_index,sub_type,composition_idandancillary_idare eachMAX_LANGUAGE_PER_DESC(5) elements. The existing check only printed a warning. The struct is a stack local inparse_PMT(), so this wrote past the 52-bytestruct dvb_configinto the caller's frame.Fix: clamp
n_languagetoMAX_LANGUAGE_PER_DESCafter the warning.2. Quadratic read stride → out-of-bounds read
The comma operator evaluates
i++first, so the cursor advanced by4*i*(i+1)bytes instead of 8 per entry, reading past the descriptor buffer from the ninth entry on.Fix:
data_ptr += 8.Verification
Built this translation unit with
-fsanitize=addressand droveparse_dvb_description()directly with a 248-byte descriptor (n_language = 31).Before:
After:
n_languageis clamped to 5, all four arrays stay in bounds, and reads stay inside the descriptor. Thematroska.ccaller passeslen = 8, which is unaffected.I could not run the repo's
tests/harness here because it links the full object set and this machine lacks the OCR dependencies (tesseract,leptonica); the check above is a targeted AddressSanitizer run against the affected function. A regression transport stream with descriptor length bytes 56 and 248 would be worth adding totests/separately.Credit
Both defects and the suggested fix come from @Haind03's write-up in #2417. This PR just applies the two changes and confirms them under ASan.