From 903645835bd2b2a2e7907b59795c1c23de0f2611 Mon Sep 17 00:00:00 2001 From: Nitzan Gur-Furman Date: Thu, 30 Mar 2023 12:12:46 +0200 Subject: [PATCH] PatchApplier: Check for existence of src/dest files before any operation Change-Id: Ia3ec0ce1af65114b48669157a934f70f1e22fd37 Bug: Google b/271474227 --- .../eclipse/jgit/patch/PatchApplierTest.java | 70 ++++++++++++++++- .../eclipse/jgit/internal/JGitText.properties | 3 + .../org/eclipse/jgit/internal/JGitText.java | 3 + .../org/eclipse/jgit/patch/PatchApplier.java | 78 ++++++++++++++----- 4 files changed, 131 insertions(+), 23 deletions(-) diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/patch/PatchApplierTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/patch/PatchApplierTest.java index 2a7ad2a815..eeec149871 100644 --- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/patch/PatchApplierTest.java +++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/patch/PatchApplierTest.java @@ -93,12 +93,16 @@ public class PatchApplierTest { } protected void initPreImage(String aName) throws Exception { - File f = new File(db.getWorkTree(), aName); preImage = IO .readWholeStream(getTestResource(aName + "_PreImage"), 0) .array(); + addFile(aName, preImage); + } + + protected void addFile(String aName, byte[] b) throws Exception { + File f = new File(db.getWorkTree(), aName); + Files.write(f.toPath(), b); try (Git git = new Git(db)) { - Files.write(f.toPath(), preImage); git.add().addFilepattern(aName).call(); } } @@ -372,6 +376,68 @@ public class PatchApplierTest { verifyChange(result, "ShiftDown2"); } + @Test + public void testAddAlreadyExistingFile() throws Exception { + addFile("M1", "existing content".getBytes()); + init("M1", false, false); + + Result result = applyPatch(); + + assertEquals(1, result.getErrors().size()); + assertEquals(0, result.getPaths().size()); + } + + @Test + public void testDeleteNonexistentFile() throws Exception { + init("NonASCIIDel", false, false); + + Result result = applyPatch(); + + assertEquals(1, result.getErrors().size()); + assertEquals(0, result.getPaths().size()); + } + + @Test + public void testModifyNonexistentFile() throws Exception { + init("ShiftDown", false, true); + + Result result = applyPatch(); + + assertEquals(1, result.getErrors().size()); + assertEquals(0, result.getPaths().size()); + } + + @Test + public void testRenameNonexistentFile() throws Exception { + init("RenameNoHunks", false, true); + + Result result = applyPatch(); + + assertEquals(1, result.getErrors().size()); + assertEquals(0, result.getPaths().size()); + } + + @Test + public void testCopyNonexistentFile() throws Exception { + init("CopyWithHunks", false, true); + + Result result = applyPatch(); + + assertEquals(1, result.getErrors().size()); + assertEquals(0, result.getPaths().size()); + } + + @Test + public void testCopyOnTopAlreadyExistingFile() throws Exception { + addFile("CopyResult", "existing content".getBytes()); + init("CopyWithHunks", true, false); + + Result result = applyPatch(); + + assertEquals(1, result.getErrors().size()); + assertEquals(0, result.getPaths().size()); + } + @Test public void testDoesNotAffectUnrelatedFiles() throws Exception { initPreImage("Unaffected"); diff --git a/org.eclipse.jgit/resources/org/eclipse/jgit/internal/JGitText.properties b/org.eclipse.jgit/resources/org/eclipse/jgit/internal/JGitText.properties index 1655cce1e1..78dd9bd4c3 100644 --- a/org.eclipse.jgit/resources/org/eclipse/jgit/internal/JGitText.properties +++ b/org.eclipse.jgit/resources/org/eclipse/jgit/internal/JGitText.properties @@ -20,6 +20,9 @@ applyBinaryPatchTypeNotSupported=Couldn't apply binary patch of type {0} applyTextPatchCannotApplyHunk=Hunk cannot be applied applyTextPatchSingleClearingHunk=Expected a single hunk for clearing all content applyBinaryResultOidWrong=Result of binary patch for file {0} has wrong OID +applyPatchWithoutSourceOnAlreadyExistingSource=Cannot perform {0} action on an existing file +applyPatchWithCreationOverAlreadyExistingDestination=Cannot perform {0} action which overrides an existing file +applyPatchWithSourceOnNonExistentSource=Cannot perform {0} action on a non-existent file applyTextPatchUnorderedHunkApplications=Current hunk must be applied after the last hunk applyTextPatchUnorderedHunks=Got unordered hunks applyingCommit=Applying {0} diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/JGitText.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/JGitText.java index 77377a3a4e..28634cbc4d 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/JGitText.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/JGitText.java @@ -46,6 +46,9 @@ public class JGitText extends TranslationBundle { /***/ public String applyBinaryOidTooShort; /***/ public String applyBinaryPatchTypeNotSupported; /***/ public String applyBinaryResultOidWrong; + /***/ public String applyPatchWithoutSourceOnAlreadyExistingSource; + /***/ public String applyPatchWithCreationOverAlreadyExistingDestination; + /***/ public String applyPatchWithSourceOnNonExistentSource; /***/ public String applyTextPatchCannotApplyHunk; /***/ public String applyTextPatchSingleClearingHunk; /***/ public String applyTextPatchUnorderedHunkApplications; diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/patch/PatchApplier.java b/org.eclipse.jgit/src/org/eclipse/jgit/patch/PatchApplier.java index 36420fcf9d..da698d6bf6 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/patch/PatchApplier.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/patch/PatchApplier.java @@ -9,6 +9,11 @@ */ package org.eclipse.jgit.patch; +import static org.eclipse.jgit.diff.DiffEntry.ChangeType.ADD; +import static org.eclipse.jgit.diff.DiffEntry.ChangeType.COPY; +import static org.eclipse.jgit.diff.DiffEntry.ChangeType.DELETE; +import static org.eclipse.jgit.diff.DiffEntry.ChangeType.MODIFY; +import static org.eclipse.jgit.diff.DiffEntry.ChangeType.RENAME; import static org.eclipse.jgit.lib.Constants.OBJ_BLOB; import java.io.ByteArrayInputStream; @@ -257,32 +262,33 @@ public class PatchApplier { Set modifiedPaths = new HashSet<>(); for (FileHeader fh : p.getFiles()) { ChangeType type = fh.getChangeType(); + File src = getFile(fh.getOldPath()); + File dest = getFile(fh.getNewPath()); + if (!verifyExistence(fh, src, dest, result)) { + continue; + } switch (type) { case ADD: { - File f = getFile(fh.getNewPath()); - if (f != null) { - FileUtils.mkdirs(f.getParentFile(), true); - FileUtils.createNewFile(f); + if (dest != null) { + FileUtils.mkdirs(dest.getParentFile(), true); + FileUtils.createNewFile(dest); } - apply(fh.getNewPath(), dirCache, dirCacheBuilder, f, fh, result); + apply(fh.getNewPath(), dirCache, dirCacheBuilder, dest, fh, result); } break; - case MODIFY: - apply(fh.getOldPath(), dirCache, dirCacheBuilder, - getFile(fh.getOldPath()), fh, result); + case MODIFY: { + apply(fh.getOldPath(), dirCache, dirCacheBuilder, src, fh, result); break; - case DELETE: + } + case DELETE: { if (!inCore()) { - File old = getFile(fh.getOldPath()); - if (!old.delete()) + if (!src.delete()) throw new IOException(MessageFormat.format( - JGitText.get().cannotDeleteFile, old)); + JGitText.get().cannotDeleteFile, src)); } break; + } case RENAME: { - File src = getFile(fh.getOldPath()); - File dest = getFile(fh.getNewPath()); - if (!inCore()) { /* * this is odd: we rename the file on the FS, but @@ -299,9 +305,7 @@ public class PatchApplier { break; } case COPY: { - File dest = getFile(fh.getNewPath()); if (!inCore()) { - File src = getFile(fh.getOldPath()); FileUtils.mkdirs(dest.getParentFile(), true); Files.copy(src.toPath(), dest.toPath()); } @@ -309,10 +313,10 @@ public class PatchApplier { break; } } - if (fh.getChangeType() != ChangeType.DELETE) + if (fh.getChangeType() != DELETE) modifiedPaths.add(fh.getNewPath()); - if (fh.getChangeType() != ChangeType.COPY - && fh.getChangeType() != ChangeType.ADD) + if (fh.getChangeType() != COPY + && fh.getChangeType() != ADD) modifiedPaths.add(fh.getOldPath()); } @@ -368,6 +372,38 @@ public class PatchApplier { return walk; } + private boolean fileExists(String path, @Nullable File f) + throws IOException { + if (f != null) { + return f.exists(); + } + return inCore() && TreeWalk.forPath(repo, path, beforeTree) != null; + } + + private boolean verifyExistence(FileHeader fh, File src, File dest, + Result result) throws IOException { + boolean isValid = true; + boolean srcShouldExist = List.of(MODIFY, DELETE, RENAME, COPY) + .contains(fh.getChangeType()); + boolean destShouldNotExist = List.of(ADD, RENAME, COPY) + .contains(fh.getChangeType()); + if (srcShouldExist != fileExists(fh.getOldPath(), src)) { + result.addError(MessageFormat.format(srcShouldExist + ? JGitText.get().applyPatchWithSourceOnNonExistentSource + : JGitText + .get().applyPatchWithoutSourceOnAlreadyExistingSource, + fh.getPatchType()), fh.getOldPath(), null); + isValid = false; + } + if (destShouldNotExist && fileExists(fh.getNewPath(), dest)) { + result.addError(MessageFormat.format(JGitText + .get().applyPatchWithCreationOverAlreadyExistingDestination, + fh.getPatchType()), fh.getNewPath(), null); + isValid = false; + } + return isValid; + } + private static final int FILE_TREE_INDEX = 1; /** @@ -681,7 +717,7 @@ public class PatchApplier { boolean hashOk = false; if (id != null) { hashOk = baseId.equals(id); - if (!hashOk && ChangeType.ADD.equals(type) + if (!hashOk && ADD.equals(type) && ObjectId.zeroId().equals(baseId)) { // We create a new file. The OID of an empty file is not the // zero id! -- 2.39.5