Skip to content

fix(rust): harden ccxr_process_cc_data against excessive cc_count#2046

Closed
Harsh-Sahu43 wants to merge 2 commits intoCCExtractor:masterfrom
Harsh-Sahu43:fix/ccxr-process-cc-data-limit
Closed

fix(rust): harden ccxr_process_cc_data against excessive cc_count#2046
Harsh-Sahu43 wants to merge 2 commits intoCCExtractor:masterfrom
Harsh-Sahu43:fix/ccxr-process-cc-data-limit

Conversation

@Harsh-Sahu43
Copy link
Copy Markdown
Contributor

@Harsh-Sahu43 Harsh-Sahu43 commented Jan 19, 2026

In raising this pull request, I confirm the following (please check boxes):

  • I have read and understood the contributors guide.
  • I have checked that another pull request for this purpose does not exist.
  • I have considered, and confirmed that this submission will be valuable to others.
  • I accept that this submission may not be used, and the pull request closed at the will of the maintainer.
  • I give this submission freely, and claim no ownership to its content.
  • I have mentioned this change in the changelog.

My familiarity with the project is as follows (check one):

  • I have never used CCExtractor.
  • I have used CCExtractor just a couple of times.
  • I absolutely love CCExtractor, but have not contributed previously.
  • I am an active contributor to CCExtractor.

{pull request content here}

Summary

Adds a defensive upper bound on cc_count in ccxr_process_cc_data to prevent
excessive allocations or misuse at the FFI boundary.

Details

  • Rejects calls where cc_count exceeds a sane limit
  • Logs a warning instead of attempting allocation
  • Adds a unit test to validate the guard
  • No behavior change for valid inputs

Copy link
Copy Markdown
Contributor

@cfsmp3 cfsmp3 left a comment

Choose a reason for hiding this comment

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

const MAX_CC_BLOCKS_PER_CALL: c_int = 10_000;

Where did you get this number from?
Do you have any sample in which this is an actual problem?

@ccextractor-bot
Copy link
Copy Markdown
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 d494286...:
Report Name Tests Passed
Broken 13/13
CEA-708 14/14
DVB 6/7
DVD 3/3
DVR-MS 2/2
General 27/27
Hardsubx 1/1
Hauppage 3/3
MP4 3/3
NoCC 10/10
Options 85/86
Teletext 21/21
WTV 13/13
XDS 34/34

Your PR breaks these cases:

Congratulations: Merging this PR would fix the following tests:

  • ccextractor --autoprogram --out=ttxt --latin1 --ucla dab1c1bd65..., Last passed: Never
  • ccextractor --out=srt --latin1 --autoprogram 29e5ffd34b..., Last passed: Never
  • ccextractor --startcreditstext "CCextractor Start crdit Testing" c4dd893cb9..., Last passed: Never
  • ccextractor --startcreditsnotbefore 1 --startcreditstext "CCextractor Start crdit Testing" c4dd893cb9..., Last passed: Never
  • ccextractor --startcreditsnotafter 2 --startcreditstext "CCextractor Start crdit Testing" c4dd893cb9..., Last passed: Never
  • ccextractor --startcreditsforatleast 1 --startcreditstext "CCextractor Start crdit Testing" c4dd893cb9..., Last passed: Never
  • ccextractor --startcreditsforatmost 2 --startcreditstext "CCextractor Start crdit Testing" c4dd893cb9..., Last passed: Never

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.

@ccextractor-bot
Copy link
Copy Markdown
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 d494286...:
Report Name Tests Passed
Broken 13/13
CEA-708 14/14
DVB 6/7
DVD 3/3
DVR-MS 2/2
General 25/27
Hardsubx 1/1
Hauppage 3/3
MP4 3/3
NoCC 10/10
Options 81/86
Teletext 21/21
WTV 13/13
XDS 34/34

Your PR breaks these cases:

  • ccextractor --autoprogram --out=srt --latin1 --quant 0 85271be4d2...
  • ccextractor --autoprogram --out=ttxt --latin1 --ucla dab1c1bd65...
  • ccextractor --out=srt --latin1 --autoprogram 29e5ffd34b...
  • ccextractor --startcreditstext "CCextractor Start crdit Testing" c4dd893cb9...
  • ccextractor --startcreditsnotbefore 1 --startcreditstext "CCextractor Start crdit Testing" c4dd893cb9...
  • ccextractor --startcreditsnotafter 2 --startcreditstext "CCextractor Start crdit Testing" c4dd893cb9...
  • ccextractor --startcreditsforatleast 1 --startcreditstext "CCextractor Start crdit Testing" c4dd893cb9...
  • ccextractor --startcreditsforatmost 2 --startcreditstext "CCextractor Start crdit Testing" c4dd893cb9...

Congratulations: Merging this PR would fix the following tests:


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.

@Harsh-Sahu43
Copy link
Copy Markdown
Contributor Author

Harsh-Sahu43 commented Jan 20, 2026

Good point — you’re right to question the constant.

Per CEA-708/ATSC, CC bandwidth is very small (~9.6 kbps total), so under normal conditions only a few bytes of CC data appear per frame CTA-708 on Wikipedia. That said, this PR wasn’t based on a failing sample, and the 10_000 value is not spec-derived.

The motivation was defensive hardening: ccxr_process_cc_data sits at the C→Rust boundary and allocates proportional to cc_count, which in some demuxer paths is derived as len / 3 without validation.

I agree a fixed numeric limit is arbitrary. I can switch this to overflow / buffer-size guarding instead of a fixed limit.
Happy to adjust based on what you think is the right direction here.

@cfsmp3
Copy link
Copy Markdown
Contributor

cfsmp3 commented Jan 21, 2026

God, I'm definitely not going to be dealing with this AI slop.

@cfsmp3 cfsmp3 closed this Jan 21, 2026
@Harsh-Sahu43
Copy link
Copy Markdown
Contributor Author

Understood — thanks for the feedback. I agree this change isn’t justified without a concrete failure case, and I’ll adjust my approach accordingly.

I’ll focus future contributions on well-scoped, test-backed issues aligned with existing CCExtractor patterns.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants