Skip to content

[FIX] Reject malformed extraction times before they abort the parser - #2374

Merged
cfsmp3 merged 2 commits into
CCExtractor:masterfrom
sovopr:codex/validate-extraction-time
Oct 18, 2026
Merged

[FIX] Reject malformed extraction times before they abort the parser#2374
cfsmp3 merged 2 commits into
CCExtractor:masterfrom
sovopr:codex/validate-extraction-time

Conversation

@sovopr

@sovopr sovopr commented Oct 13, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Reason for this PR:

  • This PR fixes a reproduced bug with a sample and regression tests.

Sanity check:

  • Read the contributors guide.
  • Checked for an existing PR addressing extraction-time validation.
  • No changelog entry for this bug fix.
  • No new C code.

--startat and --endat currently accept malformed strings through clap, then unwrap a failed stringztoms() result. In the C-facing ccxr_parse_parameters entry point, that panic cannot unwind across the ABI boundary and aborts the process.

Parse both options as Timestamp values with the existing time parser during clap validation. Options::parse_parameters then copies the validated values directly. Valid seconds, MM:SS, and HH:MM:SS inputs keep their existing meaning and range restrictions.

Reproduction sample

Create a minimal SCC input:

printf 'Scenarist_SCC V1.0\n\n' > input.scc
ccextractor input.scc --startat invalid
ccextractor input.scc --endat invalid

The file content does not affect this failure: it happens during argument parsing, before the input is opened. The added test_extraction_time_ffi_returns_error calls the same production C-facing entry point with these argument samples. With the original argument types and unwraps restored, it aborts with SIGABRT at the Option::unwrap() in parser.rs. With the fix, both options return an argument error instead. This validation uses the actual FFI parser.

Tests also cover empty, nonnumeric, out-of-range, missing-component, and extra-component values for both options, plus valid seconds, MM:SS, and HH:MM:SS values propagated into the extraction boundaries.

Validation

Using the repository's pinned Rust 1.99.0:

  • Main crate: 408 tests passed.
  • lib_ccxr: 48 unit tests and 45 documentation tests passed.
  • Formatting checks and cargo clippy -- -D warnings passed for both crates.
  • git diff --check passed.
  • The argument-validation regression fails on the original implementation; the FFI regression reproduces its process abort.

AI assistance: OpenAI Codex assisted with implementation and testing.

Upstream CI follow-up

The Linux and Windows media comparisons report zero regressions against master: all 66 approved-output mismatches also occur on master, byte for byte. Rust tests, formatting, and the other reported build checks passed. Three macOS CMake jobs failed at linking against SDK stub libraries with an unknown architecture error. The maintainer merged this change after review.

@sovopr
sovopr marked this pull request as ready for review October 13, 2026 10:09
kaihere14
kaihere14 suggested changes Oct 17, 2026
View reviewed changes

@kaihere14 kaihere14 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for this. The unwrap() on user input in the FFI path is a real crash, and moving the check into clap is the right place for it.

I checked it out locally and ran the suite. cargo test passes (408 in the main crate, 48 + 45 in lib_ccxr), and fmt, clippy and git diff --check are clean.

A few things I'd like fixed in this PR, not a follow-up:

  • 255-hour cap: --startat 256:00:00 is rejected because of the u8 parse, but the doc comment doesn't say so. Could you add a line about the limit to the --startat doc in args.rs?
  • Typo: the error text is error ocurred during parsing (lib_ccxr/src/time/units.rs). It's shared with other timestamp code, but this PR is what now surfaces it to users. Could you fix it to occurred here?

Two smaller things, optional in this PR:

  • --startat +5 is accepted as 5 seconds, because u8 parsing allows a leading +. Fine to leave, just worth a conscious call.
  • --endat earlier than --startat isn't checked. That was already true before this PR, so I don't think it should block.

Otherwise this looks good to me.

@ccextractor-bot

ccextractor-bot commented Oct 17, 2026

Copy link
Copy Markdown
Collaborator
CCExtractor CI platform finished running the test files on linux. 171/237 tests matched the approved output:
Report Name Tests Passed
Broken 9/13
CEA-708 2/14
DVB 0/7
DVD 3/3
DVR-MS 2/2
General 25/27
Hardsubx 1/1
Hauppage 3/3
MP4 3/3
NoCC 10/10
Options 69/86
Teletext 0/21
WTV 12/13
XDS 32/34

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 9642, commit 97a0d24:

  • 0 pass there and fail here
  • 0 fail there and pass here
  • 0 fail on both, with different output
  • 66 fail on both, byte for byte the same

Compared with the commit this branch was cut from: the same run as the tip of master (test 9642), 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.

@ccextractor-bot

ccextractor-bot commented Oct 17, 2026

Copy link
Copy Markdown
Collaborator
CCExtractor CI platform finished running the test files on windows. 171/237 tests matched the approved output:
Report Name Tests Passed
Broken 9/13
CEA-708 2/14
DVB 0/7
DVD 3/3
DVR-MS 2/2
General 25/27
Hardsubx 1/1
Hauppage 3/3
MP4 3/3
NoCC 10/10
Options 69/86
Teletext 0/21
WTV 12/13
XDS 32/34

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 9643, commit 97a0d24:

  • 0 pass there and fail here
  • 0 fail there and pass here
  • 0 fail on both, with different output
  • 66 fail on both, byte for byte the same

Compared with the commit this branch was cut from: the same run as the tip of master (test 9643), 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.

@cfsmp3
cfsmp3 merged commit 1ed9819 into CCExtractor:master Oct 18, 2026
23 of 28 checks passed
kaihere14 added a commit to kaihere14/ccextractor that referenced this pull request Oct 18, 2026
@kaihere14
Merge origin/master into fix-time-option-unwrap
d229365 
CCExtractor#2374 already validates --startat and --endat in clap. Do the same for
the six start/end credits timing options and drop the helper this
branch added.
@kaihere14 kaihere14 mentioned this pull request Oct 18, 2026
Merged
@cfsmp3 cfsmp3 mentioned this pull request Oct 18, 2026
Merged
5 of 7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer
@kaihere14 kaihere14 kaihere14 requested changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@sovopr @ccextractor-bot @kaihere14 @cfsmp3