[FIX] scc: don't prefix special characters with a base char, and fix out-of-bounds control code at column 0 - #2309
Open
cfsmp3 wants to merge 1 commit into
Open
[FIX] scc: don't prefix special characters with a base char, and fix out-of-bounds control code at column 0#2309cfsmp3 wants to merge 1 commit into
cfsmp3 wants to merge 1 commit into
Conversation
… at column 0 Two defects in the SCC/CCD writer. 1. Special characters gained a spurious leading space. #2301 emits a fallback base character before every internal code >= 0x80, but only EXTENDED characters (0x90-0xcf, hi 0x12/0x13) backspace-replace the cell before them -- handle_extended() decrements cursor_column. SPECIAL characters (0x80-0x8f, hi 0x11) are stand-alone: handle_double() writes them without moving the cursor back, so the base character stays on screen and every one of them came back one column to the right on re-decode. Only emit the base character for extended codes. check_padding() still runs for both so the two-byte code starts on an even offset and lands inside a single SCC word. Verified with an SCC exercising all 80 codes in 0x80-0xcf, re-encoded and decoded again: 79/80 wrong before #2301, 16/80 after it, 0/80 now. On real samples, 725a49f871 (15 music-note rows) and c032183ef0 (3) round-trip with no altered text; the apostrophe from #2098 is unchanged at "a7 80 92 29". 2. Out-of-bounds control code index when a style change starts at column 0. get_preamble_code() and get_tab_offset_code() take unsigned char, so the column - 1 used to place the preamble one cell left wrapped to 255 at column 0, yielding 255 / 4 = 63 and an index far past the end of control_codes[]. Row 12 produced code 186 against CONTROL_CODE_MAX 147. In --out=ccd that garbage entry is passed to strlen() and segfaults; in --out=scc it silently emits whatever ints follow the array. The read has been there since before #2301 -- 2 of 20 local samples hit it -- but the layout change from #2301 moved the garbage pointer into unmapped memory, so it now crashes rather than misbehaving quietly. Clamp the preamble column to 0, matching what the adjacent space branch already does. Non-SCC output (txt, sami, srt, ttxt, webvtt, g608) is byte-identical to master across 20 samples. Of those 20, SCC output changes on 12 with special characters and on 1 that hit the out-of-bounds index; the other 7 are unchanged. Valgrind reports no invalid reads, only the pre-existing 32-byte init_encoder leak that master has too.
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.
Follow-up to #2301. Two defects in the SCC/CCD writer, both found while deep-reviewing that PR.
1. Special characters gained a spurious leading space
#2301 emits a fallback base character before every internal code
>= 0x80. That is right for extended characters (0x90-0xcf, hi0x12/0x13) —handle_extended()decrementscursor_column, so they destructively replace the cell before them and the stream has to carry something for them to overwrite.It is wrong for special characters (
0x80-0x8f, hi0x11).handle_double()writes those without moving the cursor back, so the base character stays on screen and every special character comes back one column further right.Fix: only emit the base character for extended codes.
check_padding()still runs in both cases so the two-byte code starts on an even offset and lands inside a single SCC word.Testing
Generated an SCC exercising all 80 codes in
0x80-0xcf, re-encoded it and decoded it again:Real samples:
725a49f871(15 music-note rows) andc032183ef0(3) now round-trip with no altered text. The apostrophe from #2098 is unchanged — stilla7 80 92 29.2. Out-of-bounds control code index when a style change starts at column 0
get_preamble_code()andget_tab_offset_code()takeunsigned char. Thecolumn - 1used to place the preamble one cell to the left wraps to 255 at column 0, giving255 / 4 = 63and an index far past the end ofcontrol_codes[]— row 12 produces code 186 againstCONTROL_CODE_MAX147.In
--out=ccdthat entry'sassemblypointer is handed tostrlen()and segfaults. In--out=sccit silently emits whatever ints happen to follow the array.This read predates #2301 — 2 of 20 local samples hit it both before and after — but #2301 shifted the binary layout so the garbage pointer now lands in unmapped memory. It went from quietly wrong to a hard crash:
Fix: clamp the preamble column to 0, which is what the adjacent space branch already does.
Regression scope
txt,sami,srt,ttxt,webvtt,g608) byte-identical to master across 20 samples.init_encoderleak, which master has too.clang-formatclean.