[FIX] file_functions: check the seek that can fail instead of one that cannot - #2326
Open
cfsmp3 wants to merge 1 commit into
Open
[FIX] file_functions: check the seek that can fail instead of one that cannot#2326cfsmp3 wants to merge 1 commit into
cfsmp3 wants to merge 1 commit into
Conversation
… cannot buffered_read_opt()'s seek branch guarded against moving before the start of the file with "op + bytes < 0". That could not fire: bytes is unsigned, so the sum was evaluated unsigned and never went negative, and every caller passes a forward distance anyway. What can fail is LSEEK itself. On error it returns -1, and the code then computed "np - op" from two error values and reported the result as a byte count. Today that lands on 0 and the loop exits, so the observable behaviour is unchanged -- but only by arithmetic accident, and the next reader has no way to tell that the guard above it was inert. Check what actually fails, and return the bytes copied so far rather than inventing a distance from two failures. Noted while reviewing the MSVC narrowing warnings for #2325, which flagged this line as dead rather than wrong and left it out of that change deliberately.
Collaborator
CCExtractor CI platform finished running the test files on linux. 167/237 tests matched the approved output:
70 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 9494, commit 3af3fc2:
Pass there, fail here:
Compared with the commit this branch was cut from: the same run as the tip of master (test 9494), so the comparison above already covers it. This branch changes the behaviour of 1 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. 167/237 tests matched the approved output:
70 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 9495, commit 3af3fc2:
Pass there, fail here:
Compared with the commit this branch was cut from: the same run as the tip of master (test 9495), so the comparison above already covers it. This branch changes the behaviour of 1 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
2 participants
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 #2325, which flagged this line and deliberately left it alone.
The guard could not fire
bytesis asize_t, soop + bytesis evaluated unsigned and never goes negative — the same family as the underflow #2322 fixed. And it could not matter anyway: every caller reaches here with a forward distance, so a seek from a valid position cannot land before the start of the file.What can actually fail
LSEEKreturns −1 on error. Neither result was checked, sonp - opwas computed from two error values and reported as a byte count.Today that lands on
(-1) - (-1) == 0, thedo/whilecondition then ends the loop, and the function returnscopied— so the observable behaviour is correct by arithmetic accident. Nothing tells the next reader that, and the guard sitting above it implies the failure is already handled.copiedrather than0: it is what the normal path returns a few lines down, and bytes already handed to the caller should not be forgotten because a later seek failed.Testing
A/B against a binary built from current master (
3af3fc22):--autoprogram --out=srt --latin1(99 output files)--startat/--endat,--no-bufferinput,--druBuilds with no compiler messages;
clang-formatclean.Worth stating how that second row was nearly misread: comparing the two runs with
cmpalone reported "6 differences", becausecmpfails the same way whether the contents differ or the file is absent — and with those options neither side writes one. The counts above separate identical, both produced nothing, one-sided, and content differs, because only the last is evidence of anything.