From 8adbfe4da6723768febbb86c22ac118e233c2154 Mon Sep 17 00:00:00 2001 From: Philipp Marx Date: Fri, 11 Nov 2016 10:43:09 +0100 Subject: [PATCH] Check that DfsBlockCache#blockSize is a power of 2 MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit In case a value is used which isn’t a power of 2 there will be a high chance of java.lang.ArrayIndexOutBoundsException and org.eclipse.jgit.errors.CorruptObjectException due to a mismatching assumption for the DfsBlockCache#blockSizeShift parameter. Change-Id: Ib348b3704edf10b5f93a3ffab4fa6f09cbbae231 Signed-off-by: Philipp Marx --- .../storage/dfs/DfsBlockCacheConfigTest.java | 88 +++++++++++++++++++ .../eclipse/jgit/internal/JGitText.properties | 1 + .../org/eclipse/jgit/internal/JGitText.java | 1 + .../internal/storage/dfs/DfsBlockCache.java | 2 + .../storage/dfs/DfsBlockCacheConfig.java | 8 +- 5 files changed, 99 insertions(+), 1 deletion(-) create mode 100644 org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/dfs/DfsBlockCacheConfigTest.java diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/dfs/DfsBlockCacheConfigTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/dfs/DfsBlockCacheConfigTest.java new file mode 100644 index 0000000000..e612061abf --- /dev/null +++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/dfs/DfsBlockCacheConfigTest.java @@ -0,0 +1,88 @@ +/* + * Copyright (C) 2016, Philipp Marx and + * other copyright owners as documented in the project's IP log. + * + * This program and the accompanying materials are made available under the + * terms of the Eclipse Distribution License v1.0 which accompanies this + * distribution, is reproduced below, and is available at + * http://www.eclipse.org/org/documents/edl-v10.php + * + * All rights reserved. + * + * Redistribution and use in source and binary forms, with or without + * modification, are permitted provided that the following conditions are met: + * + * - Redistributions of source code must retain the above copyright notice, this + * list of conditions and the following disclaimer. + * + * - Redistributions in binary form must reproduce the above copyright notice, + * this list of conditions and the following disclaimer in the documentation + * and/or other materials provided with the distribution. + * + * - Neither the name of the Eclipse Foundation, Inc. nor the names of its + * contributors may be used to endorse or promote products derived from this + * software without specific prior written permission. + * + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" + * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE + * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE + * ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR CONTRIBUTORS BE + * LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR + * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF + * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS + * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN + * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) + * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE + * POSSIBILITY OF SUCH DAMAGE. + */ + +package org.eclipse.jgit.internal.storage.dfs; + +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.is; + +import org.eclipse.jgit.internal.JGitText; +import org.junit.Rule; +import org.junit.Test; +import org.junit.rules.ExpectedException; + +public class DfsBlockCacheConfigTest { + + @Rule + public ExpectedException thrown = ExpectedException.none(); + + @Test + public void blockSizeNotPowerOfTwoExpectsException() { + thrown.expect(IllegalArgumentException.class); + thrown.expectMessage(is(JGitText.get().blockSizeNotPowerOf2)); + + new DfsBlockCacheConfig().setBlockSize(1000); + } + + @Test + @SuppressWarnings("boxing") + public void negativeBlockSizeIsConvertedToDefault() { + DfsBlockCacheConfig config = new DfsBlockCacheConfig(); + config.setBlockSize(-1); + + assertThat(config.getBlockSize(), is(512)); + } + + @Test + @SuppressWarnings("boxing") + public void tooSmallBlockSizeIsConvertedToDefault() { + DfsBlockCacheConfig config = new DfsBlockCacheConfig(); + config.setBlockSize(10); + + assertThat(config.getBlockSize(), is(512)); + } + + @Test + @SuppressWarnings("boxing") + public void validBlockSize() { + DfsBlockCacheConfig config = new DfsBlockCacheConfig(); + config.setBlockSize(65536); + + assertThat(config.getBlockSize(), is(65536)); + } +} 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 399436dfaa..aa73b44848 100644 --- a/org.eclipse.jgit/resources/org/eclipse/jgit/internal/JGitText.properties +++ b/org.eclipse.jgit/resources/org/eclipse/jgit/internal/JGitText.properties @@ -39,6 +39,7 @@ bitmapsMustBePrepared=Bitmaps must be prepared before they may be written. blameNotCommittedYet=Not Committed Yet blobNotFound=Blob not found: {0} blobNotFoundForPath=Blob not found: {0} for path: {1} +blockSizeNotPowerOf2=blockSize must be a power of 2 branchNameInvalid=Branch name {0} is not allowed buildingBitmaps=Building bitmaps cachedPacksPreventsIndexCreation=Using cached packs prevents index creation 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 00d019184d..29b319fe65 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/JGitText.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/JGitText.java @@ -98,6 +98,7 @@ public class JGitText extends TranslationBundle { /***/ public String blameNotCommittedYet; /***/ public String blobNotFound; /***/ public String blobNotFoundForPath; + /***/ public String blockSizeNotPowerOf2; /***/ public String branchNameInvalid; /***/ public String buildingBitmaps; /***/ public String cachedPacksPreventsIndexCreation; diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/dfs/DfsBlockCache.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/dfs/DfsBlockCache.java index 764ae12849..05627ed86b 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/dfs/DfsBlockCache.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/dfs/DfsBlockCache.java @@ -145,6 +145,8 @@ public final class DfsBlockCache { *

* If a pack file has a native size, a whole multiple of the native size * will be used until it matches this size. + *

+ * The value for blockSize must be a power of 2. */ private final int blockSize; diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/dfs/DfsBlockCacheConfig.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/dfs/DfsBlockCacheConfig.java index 7e32554c69..089bfa471d 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/dfs/DfsBlockCacheConfig.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/dfs/DfsBlockCacheConfig.java @@ -106,10 +106,16 @@ public class DfsBlockCacheConfig { /** * @param newSize * size in bytes of a single window read in from the pack file. + * The value must be a power of 2. * @return {@code this} */ public DfsBlockCacheConfig setBlockSize(final int newSize) { - blockSize = Math.max(512, newSize); + int size = Math.max(512, newSize); + if ((size & (size - 1)) != 0) { + throw new IllegalArgumentException( + JGitText.get().blockSizeNotPowerOf2); + } + blockSize = size; return this; } -- 2.39.5