Check that DfsBlockCache#blockSize is a power of 2

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 <smigfu@googlemail.com>

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 (file)
index 0000000..e612061
--- /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 <philippmarx@gmx.de> 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 399436dfaaab8e61c0b9d3b4d9d8530f1a8687f3..aa73b448480b37b4eb45716a8db105e89442ff1c 100644 (file)
--- 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 00d019184d7b8c25844d19763f200d2ad444da2a..29b319fe65c10a1e4bd82d0190124bb7647d6251 100644 (file)
--- 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 764ae128496968ff59360236cb060ef79b162c2b..05627ed86baad404e1cd23219f8a354c773c5706 100644 (file)
--- 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 {
         * <p>
         * If a pack file has a native size, a whole multiple of the native size
         * will be used until it matches this size.
+        * <p>
+        * 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 7e32554c697668e8e4a803f7a55ba57d52fcf47a..089bfa471dd602e87517f9c9458dcefdfb4474b0 100644 (file)
--- 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;
        }