Reject a chunk that ends exactly at the end of the target - #232
Open
youdie006 wants to merge 1 commit into
Open
Conversation
verifyChunk compares last, an index, against target.size(), a count:
if (position + fuzz > target.size() || last - fuzz > target.size())
The loop below reads target.get(last - fuzz), so it needs
last - fuzz < target.size(). Rejecting only > lets the exactly-one-past
case through, and List.get throws:
chunk covering target indices 7..10, target of length 10
-> IndexOutOfBoundsException: Index 10 out of bounds for length 10
applyTo and DiffUtils.patch document PatchFailedException for a patch
that cannot be applied, and the neighbouring case (target two short)
already returns POSITION_OUT_OF_TARGET, so a target one short should
too.
The first disjunct stays >: an InsertDelta appending at end of file has
an empty source chunk with position == target.size(), and >= there would
reject a valid append. An empty chunk has last == position - 1, so the
second disjunct is already correct for it.
No test referenced POSITION_OUT_OF_TARGET; ChunkTest covered OK and
CONTENT_DOES_NOT_MATCH_TARGET only.
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
1 participant
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.
The problem
Chunk.verifyChunklets a chunk that ends exactly at the end of the target through its bounds check, soList.getthrows instead of the documentedPatchFailedException.With a chunk covering target indices 7..10:
last2 past)POSITION_OUT_OF_TARGETPOSITION_OUT_OF_TARGETlast1 past)POSITION_OUT_OF_TARGETIndexOutOfBoundsException: Index 10 out of bounds for length 10OKOKReached through the public API as well —
Patch.applyTo/DiffUtils.patchon a target one line shorter than the patch expects raisesIndexOutOfBoundsExceptionrather thanPatchFailedException, so a caller'scatch (PatchFailedException)does not hold.Cause
java-diff-utils/src/main/java/com/github/difflib/patch/Chunk.java:116lastis an index (last = position + size() - 1, line 114) but is compared against a count. The loop at 119-120 reads up totarget.get(last - fuzz), which requireslast - fuzz < target.size(), so the second disjunct has to reject>=, not just>.Why this is the code and not the doc
Patch.java:50-56documents@throws PatchFailedException if the patch cannot be applied, repeated atPatch.java:68,DiffUtils.java:203,DiffUtils.java:215, andChunk.java:95/:108.Patch.java:229also defaultsconflictOutputtoCONFLICT_PRODUCES_EXCEPTION, so surfacing a failure asPatchFailedExceptionis the intended path. The two-short case already does exactly that; only the one-short boundary leaks.Scope of the change
Only the second disjunct. The first stays
>deliberately: anInsertDeltaappending at end of file has an empty source chunk withposition == target.size(), and>=there would reject a valid append. An empty chunk haslast == position - 1, so the second disjunct is already correct for that case.Tests
No test referenced
POSITION_OUT_OF_TARGET,verifyChunk's bounds, or "could not apply patch" —ChunkTestcoveredOKandCONTENT_DOES_NOT_MATCH_TARGETonly, and the apply-failure tests inPatchWithMyerDiffTest/PatchWithAllDiffAlgorithmsTestall usecatch (PatchFailedException e) { fail(...) }around successful applies. AddedverifyChunkAtTheEndOfTheTargetcovering the boundary and one past it, plus the length that must still succeed.Verification
mvn -B packageon JDK 17: BUILD SUCCESS, 148 tests + 2 in the jgit module, 0 failures, 0 errors — spotless and checkstyle both clean.Reverting only
Chunk.javaand keeping the test fails withIndexOutOfBoundsException: Index 10 out of bounds for length 10, so the new test exercises this bug rather thanverifyChunkin general.(Note for anyone reproducing:
mvnunder JDK 21 fails before running tests — spotless 2.30.0 / palantir-java-format hitsNoSuchMethodError: JCTree$JCImport.getQualifiedIdentifier(). JDK 17 is fine.)Disclosure: this patch was prepared with AI assistance. The reproduction, the red/green check and the build above were executed against this branch; happy to adjust anything on request.