From ced658c445b9087c57eaa3d470f46afec4a4995f Mon Sep 17 00:00:00 2001 From: Han-Wen Nienhuys Date: Tue, 10 Oct 2017 16:40:12 +0200 Subject: [PATCH] Avoid loading and merging binary data in ResolveMerger Signed-off-by: Han-Wen Nienhuys Change-Id: Ide4b68872d426aa262142f224acf636c776b35d3 --- .../eclipse/jgit/merge/ResolveMergerTest.java | 142 ++++++++++++++++++ .../org/eclipse/jgit/merge/ResolveMerger.java | 30 +++- 2 files changed, 165 insertions(+), 7 deletions(-) diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/merge/ResolveMergerTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/merge/ResolveMergerTest.java index d8b8750ba..77ecf19fb 100644 --- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/merge/ResolveMergerTest.java +++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/merge/ResolveMergerTest.java @@ -52,6 +52,7 @@ import java.io.ByteArrayOutputStream; import java.io.File; import java.io.FileInputStream; import java.io.IOException; +import java.nio.charset.StandardCharsets; import java.util.Arrays; import org.eclipse.jgit.api.Git; @@ -65,8 +66,12 @@ import org.eclipse.jgit.errors.NoMergeBaseException; import org.eclipse.jgit.errors.NoMergeBaseException.MergeBaseFailureReason; import org.eclipse.jgit.junit.RepositoryTestCase; import org.eclipse.jgit.junit.TestRepository; +import org.eclipse.jgit.lib.AnyObjectId; import org.eclipse.jgit.lib.ObjectId; import org.eclipse.jgit.lib.ObjectInserter; +import org.eclipse.jgit.lib.ObjectLoader; +import org.eclipse.jgit.lib.ObjectReader; +import org.eclipse.jgit.lib.ObjectStream; import org.eclipse.jgit.merge.ResolveMerger.MergeFailureReason; import org.eclipse.jgit.revwalk.RevCommit; import org.eclipse.jgit.revwalk.RevObject; @@ -689,6 +694,143 @@ public class ResolveMergerTest extends RepositoryTestCase { } } + + /** + * Merging a change involving large binary files should short-circuit reads. + * + * @param strategy + * @throws Exception + */ + @Theory + public void checkContentMergeLargeBinaries(MergeStrategy strategy) throws Exception { + Git git = Git.wrap(db); + final int LINELEN = 72; + + // setup a merge that would work correctly if we disconsider the stray '\0' + // that the file contains near the start. + byte[] binary = new byte[LINELEN * 2000]; + for (int i = 0; i < binary.length; i++) { + binary[i] = (byte)((i % LINELEN) == 0 ? '\n' : 'x'); + } + binary[50] = '\0'; + + writeTrashFile("file", new String(binary, StandardCharsets.UTF_8)); + git.add().addFilepattern("file").call(); + RevCommit first = git.commit().setMessage("added file").call(); + + // Generate an edit in a single line. + int idx = LINELEN * 1200 + 1; + byte save = binary[idx]; + binary[idx] = '@'; + writeTrashFile("file", new String(binary, StandardCharsets.UTF_8)); + + binary[idx] = save; + git.add().addFilepattern("file").call(); + RevCommit masterCommit = git.commit().setAll(true) + .setMessage("modified file l 1200").call(); + + git.checkout().setCreateBranch(true).setStartPoint(first).setName("side").call(); + binary[LINELEN * 1500 + 1] = '!'; + writeTrashFile("file", new String(binary, StandardCharsets.UTF_8)); + git.add().addFilepattern("file").call(); + RevCommit sideCommit = git.commit().setAll(true) + .setMessage("modified file l 1500").call(); + + try (ObjectInserter ins = db.newObjectInserter()) { + // Check that we don't read the large blobs. + ObjectInserter forbidInserter = new ObjectInserter.Filter() { + @Override + protected ObjectInserter delegate() { + return ins; + } + + @Override + public ObjectReader newReader() { + return new BigReadForbiddenReader(super.newReader(), 8000); + } + }; + + ResolveMerger merger = + (ResolveMerger) strategy.newMerger(forbidInserter, db.getConfig()); + boolean noProblems = merger.merge(masterCommit, sideCommit); + assertFalse(noProblems); + } + } + + /** + * Throws an exception if reading beyond limit. + */ + class BigReadForbiddenStream extends ObjectStream.Filter { + int limit; + + BigReadForbiddenStream(ObjectStream orig, int limit) { + super(orig.getType(), orig.getSize(), orig); + this.limit = limit; + } + + @Override + public long skip(long n) throws IOException { + limit -= n; + if (limit < 0) { + throw new IllegalStateException(); + } + + return super.skip(n); + } + + @Override + public int read() throws IOException { + int r = super.read(); + limit--; + if (limit < 0) { + throw new IllegalStateException(); + } + return r; + } + + @Override + public int read(byte[] b, int off, int len) throws IOException { + int n = super.read(b, off, len); + limit -= n; + if (limit < 0) { + throw new IllegalStateException(); + } + return n; + } + } + + class BigReadForbiddenReader extends ObjectReader.Filter { + ObjectReader delegate; + int limit; + + @Override + protected ObjectReader delegate() { + return delegate; + } + + BigReadForbiddenReader(ObjectReader delegate, int limit) { + this.delegate = delegate; + this.limit = limit; + } + + @Override + public ObjectLoader open(AnyObjectId objectId, int typeHint) throws IOException { + ObjectLoader orig = super.open(objectId, typeHint); + return new ObjectLoader.Filter() { + @Override + protected ObjectLoader delegate() { + return orig; + } + + @Override + public ObjectStream openStream() throws IOException { + ObjectStream os = orig.openStream(); + return new BigReadForbiddenStream(os, limit); + } + }; + } + } + @Theory public void checkContentMergeConflict(MergeStrategy strategy) throws Exception { diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/merge/ResolveMerger.java b/org.eclipse.jgit/src/org/eclipse/jgit/merge/ResolveMerger.java index 92c2bb35a..fc2fa0b9e 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/merge/ResolveMerger.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/merge/ResolveMerger.java @@ -79,6 +79,7 @@ import org.eclipse.jgit.dircache.DirCacheBuildIterator; import org.eclipse.jgit.dircache.DirCacheBuilder; import org.eclipse.jgit.dircache.DirCacheCheckout; import org.eclipse.jgit.dircache.DirCacheEntry; +import org.eclipse.jgit.errors.BinaryBlobException; import org.eclipse.jgit.errors.CorruptObjectException; import org.eclipse.jgit.errors.IncorrectObjectTypeException; import org.eclipse.jgit.errors.IndexWriteException; @@ -89,9 +90,11 @@ import org.eclipse.jgit.lib.ConfigConstants; import org.eclipse.jgit.lib.FileMode; import org.eclipse.jgit.lib.ObjectId; import org.eclipse.jgit.lib.ObjectInserter; +import org.eclipse.jgit.lib.ObjectLoader; import org.eclipse.jgit.lib.ObjectReader; import org.eclipse.jgit.lib.Repository; import org.eclipse.jgit.revwalk.RevTree; +import org.eclipse.jgit.storage.pack.PackConfig; import org.eclipse.jgit.treewalk.AbstractTreeIterator; import org.eclipse.jgit.treewalk.CanonicalTreeParser; import org.eclipse.jgit.treewalk.NameConflictTreeWalk; @@ -667,6 +670,7 @@ public class ResolveMerger extends ThreeWayMerger { // OURS or THEIRS has been deleted if (((modeO != 0 && !tw.idEqual(T_BASE, T_OURS)) || (modeT != 0 && !tw .idEqual(T_BASE, T_THEIRS)))) { + MergeResult result = contentMerge(base, ours, theirs); add(tw.getRawPath(), base, DirCacheEntry.STAGE_1, 0, 0); add(tw.getRawPath(), ours, DirCacheEntry.STAGE_2, 0, 0); @@ -687,8 +691,7 @@ public class ResolveMerger extends ThreeWayMerger { unmergedPaths.add(tw.getPathString()); // generate a MergeResult for the deleted file - mergeResults.put(tw.getPathString(), - contentMerge(base, ours, theirs)); + mergeResults.put(tw.getPathString(), result); } } return true; @@ -709,12 +712,22 @@ public class ResolveMerger extends ThreeWayMerger { private MergeResult contentMerge(CanonicalTreeParser base, CanonicalTreeParser ours, CanonicalTreeParser theirs) throws IOException { - RawText baseText = base == null ? RawText.EMPTY_TEXT : getRawText( + RawText baseText; + RawText ourText; + RawText theirsText; + + try { + baseText = base == null ? RawText.EMPTY_TEXT : getRawText( base.getEntryObjectId(), reader); - RawText ourText = ours == null ? RawText.EMPTY_TEXT : getRawText( + ourText = ours == null ? RawText.EMPTY_TEXT : getRawText( ours.getEntryObjectId(), reader); - RawText theirsText = theirs == null ? RawText.EMPTY_TEXT : getRawText( + theirsText = theirs == null ? RawText.EMPTY_TEXT : getRawText( theirs.getEntryObjectId(), reader); + } catch (BinaryBlobException e) { + MergeResult r = new MergeResult<>(Collections.emptyList()); + r.setContainsConflicts(true); + return r; + } return (mergeAlgorithm.merge(RawTextComparator.DEFAULT, baseText, ourText, theirsText)); } @@ -891,10 +904,13 @@ public class ResolveMerger extends ThreeWayMerger { } private static RawText getRawText(ObjectId id, ObjectReader reader) - throws IOException { + throws IOException, BinaryBlobException { if (id.equals(ObjectId.zeroId())) return new RawText(new byte[] {}); - return new RawText(reader.open(id, OBJ_BLOB).getCachedBytes()); + + ObjectLoader loader = reader.open(id, OBJ_BLOB); + int threshold = PackConfig.DEFAULT_BIG_FILE_THRESHOLD; + return RawText.load(loader, threshold); } private static boolean nonTree(final int mode) {