Skip to content

[FIX] file_functions: three integer conversions in the read path (truncated seek, unsigned underflow, signed/unsigned compare) - #2325

Merged
cfsmp3 merged 1 commit into
masterfrom
fix/file-functions-integer-truncation
Aug 15, 2026
Merged

[FIX] file_functions: three integer conversions in the read path (truncated seek, unsigned underflow, signed/unsigned compare)#2325
cfsmp3 merged 1 commit into
masterfrom
fix/file-functions-integer-truncation

Conversation

@cfsmp3

@cfsmp3 cfsmp3 commented Aug 15, 2026

Copy link
Copy Markdown
Contributor

Follow-up to #2322. While reading the MSVC narrowing warnings on that PR I checked each one in file_functions.c; most are benign, three are not. All three are the same family as #2322 — an integer conversion that quietly turns a check into something other than what it reads as.

1. get_file_size() truncates a 64-bit offset

int ret = 0;                                 // LSEEK returns LLONG
...
ret = LSEEK(in, current, SEEK_SET);
if (ret < 0) return -1;

A successful seek back to a position at or beyond 2 GB truncates to a negative int, so the function reports failure for a seek that worked, and the caller sees the file size as -1.

ccx_demuxer_get_file_size() (ccx_demuxer.c:236) is the same routine written correctly with LLONG ret — this copy had drifted.

Latent, not observable today: the only caller, get_total_file_size(), opens each file fresh, so current is always 0. Fixed because the next caller has no way to know that.

2. Same unsigned underflow #2322 fixed, one layer down

size_t ready = ctx->bytesinbuffer - ctx->filebuffer_pos;

Both operands are unsigned int. This is exactly the subtraction that made the file_buffer.h bounds checks pass when they should have failed. #2322 added buffered_bytes_left() for it; this site was missed.

3. End-of-input test compares signed against unsigned

((struct lib_ccx_ctx *)ctx->parent)->inputsize <= origin_buffer_size

inputsize is LLONG, origin_buffer_size is size_t, so the usual arithmetic conversions make the comparison unsigned. get_filesize() returns −1 when the size cannot be determined (ccx_demuxer.c:246), and that −1 becomes SIZE_MAX — the test silently reads as "the input is enormous".

Now spelled out:

LLONG parent_inputsize = ((struct lib_ccx_ctx *)ctx->parent)->inputsize;
int input_fits_in_one_read = parent_inputsize >= 0 &&
			     (uint64_t)parent_inputsize <= (uint64_t)origin_buffer_size;

Behaviour is unchanged. For inputsize >= 0 the unsigned comparison is identical to before. For inputsize < 0 both the old and new forms yield false, so an unknown size still does not count as fitting in one read. The hoisted expression is a pure field read and comparison, so the short-circuit order is preserved and switch_to_next_file() — the one clause with side effects — is still called exactly when it was.

Testing

This file sits on every input path, so the changes were A/B'd against a binary built from current master (128175ea):

path result
99 output files across the sample library, --out=srt --latin1 byte-identical, exit codes match
--bufferinput (10 samples) byte-identical
multi-file input (binary concat / switch_to_next_file) byte-identical
stdin (-stdin, where inputsize can be negative) byte-identical

Builds clean with no compiler messages; clang-format reports no changes.

Noted, not changed

buffered_read_opt() has a vacuous guard at what is now line 374:

if (op + bytes < 0)   // Would mean moving beyond start of file: Not supported
	return 0;

op is LLONG and bytes is size_t, so the sum is unsigned and can never be negative — and every caller passes a non-negative bytes anyway, so it could not fire even with correct types. It is dead rather than wrong, and removing it is a separate judgement call about whether the intended check should exist at all. Flagging it here rather than widening this PR.

@cfsmp3
fix(file_functions): correct three integer conversions in the read path
aee9693 
get_file_size() stored an LSEEK result in an int. LSEEK returns a 64-bit
offset, so a successful seek back to a position at or beyond 2 GB truncated
to a negative int and the function reported failure for a seek that worked.
ccx_demuxer_get_file_size() is the same routine written correctly with an
LLONG; this copy had drifted. Latent today -- the only caller opens each file
fresh, so the position is always 0 -- but it is a trap for the next caller.

buffered_read_opt() computed the bytes left in the file buffer as
"bytesinbuffer - filebuffer_pos" on two unsigned ints. That is the underflow
that made the bounds checks in file_buffer.h pass when they should have
failed; the helper added to fix those, buffered_bytes_left(), belongs here too.

The end-of-input test compared inputsize, a signed 64-bit size, against
origin_buffer_size, a size_t. The usual arithmetic conversions turned a
negative inputsize -- what get_filesize() returns when the size cannot be
determined -- into a huge unsigned value, so the test silently read as "the
input is enormous". The comparison is now spelled out. An unknown size still
does not count as fitting in one read, and the short-circuit order is
unchanged, so switch_to_next_file() is called exactly when it was before.

Verified against the current master binary: byte-identical output on 99 files
from the sample library, and on the --bufferinput, multi-file concat and
stdin paths, with matching exit codes throughout.
@ccextractor-bot

ccextractor-bot commented Aug 15, 2026

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 128175e...:
Report Name Tests Passed
Broken 9/13
CEA-708 2/14
DVB 1/7
DVD 3/3
DVR-MS 2/2
General 22/27
Hardsubx 1/1
Hauppage 3/3
MP4 3/3
NoCC 10/10
Options 69/86
Teletext 0/21
WTV 12/13
XDS 31/34

Your PR breaks these cases:

NOTE: The following tests have been failing on the master branch as well as the PR:

Congratulations: Merging this PR would fix the following tests:

  • ccextractor --autoprogram --out=srt --latin1 f1422b8bfe..., 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

ccextractor-bot commented Aug 15, 2026

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 128175e...:
Report Name Tests Passed
Broken 9/13
CEA-708 2/14
DVB 1/7
DVD 3/3
DVR-MS 2/2
General 22/27
Hardsubx 1/1
Hauppage 3/3
MP4 3/3
NoCC 10/10
Options 69/86
Teletext 0/21
WTV 12/13
XDS 31/34

Your PR breaks these cases:

NOTE: The following tests have been failing on the master branch as well as the PR:

Congratulations: Merging this PR would fix the following tests:

  • ccextractor --autoprogram --out=srt --latin1 f1422b8bfe..., 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.

@cfsmp3
cfsmp3 merged commit 3af3fc2 into master Aug 15, 2026
46 of 48 checks passed
@cfsmp3
cfsmp3 deleted the fix/file-functions-integer-truncation branch August 15, 2026 15:48
@cfsmp3 cfsmp3 mentioned this pull request Aug 17, 2026
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@cfsmp3 @ccextractor-bot