ss: Relax but enforce the character limit in HTInterval strings
authorAlexandre Montplaisir <alexmonthy@efficios.com>
Mon, 27 Jun 2016 22:09:36 +0000 (18:09 -0400)
committerAlexandre Montplaisir <alexmonthy@efficios.com>
Wed, 29 Jun 2016 21:05:40 +0000 (17:05 -0400)
The string parsing logic in HTInterval casts the string length to
a byte, which can result in a negative value (and subsequent
NegativeArraySize exception) if the string is actually > 127 bytes
in its encoded form.

A comment mentions that the string length is checked at the
constructor, but this is false! No check is done whatsoever.
The limit should be checked.

While at it, the current limit of 127 bytes is very small for
string values. File system paths for example can easily add up
to more than 127 characters. For this reason, use a short (2 bytes)
instead of one byte to store the string length, which allows for
strings up to 32,767 bytes.

Finally, specify UTF-8 encoding explicitely. This is what was
happening on most platforms already, but it's a better practice
to specify it, and it makes static analysis happy.

Also happens to fix:
Bug: 496864

Change-Id: Ia801782696a1574568d52c52775d475ed2dcf173
Signed-off-by: Alexandre Montplaisir <alexmonthy@efficios.com>
Reviewed-on: https://git.eclipse.org/r/76060
Reviewed-by: Hudson CI
statesystem/org.eclipse.tracecompass.statesystem.core.tests/src/org/eclipse/tracecompass/statesystem/core/tests/backend/historytree/HTIntervalStringReadWriteTest.java [new file with mode: 0644]
statesystem/org.eclipse.tracecompass.statesystem.core/src/org/eclipse/tracecompass/internal/statesystem/core/backend/historytree/HTInterval.java
statesystem/org.eclipse.tracecompass.statesystem.core/src/org/eclipse/tracecompass/internal/statesystem/core/backend/historytree/HistoryTree.java

diff --git a/statesystem/org.eclipse.tracecompass.statesystem.core.tests/src/org/eclipse/tracecompass/statesystem/core/tests/backend/historytree/HTIntervalStringReadWriteTest.java b/statesystem/org.eclipse.tracecompass.statesystem.core.tests/src/org/eclipse/tracecompass/statesystem/core/tests/backend/historytree/HTIntervalStringReadWriteTest.java
new file mode 100644 (file)
index 0000000..5f82e2c
--- /dev/null
@@ -0,0 +1,184 @@
+/*******************************************************************************
+ * Copyright (c) 2016 EfficiOS Inc., Alexandre Montplaisir
+ *
+ * All rights reserved. This program and the accompanying materials
+ * are made available under the terms of the Eclipse Public License v1.0
+ * which accompanies this distribution, and is available at
+ * http://www.eclipse.org/legal/epl-v10.html
+ *******************************************************************************/
+
+package org.eclipse.tracecompass.statesystem.core.tests.backend.historytree;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.fail;
+
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.FileOutputStream;
+import java.io.IOException;
+import java.nio.ByteBuffer;
+import java.nio.ByteOrder;
+import java.nio.channels.FileChannel;
+import java.nio.charset.Charset;
+import java.util.List;
+import java.util.Set;
+import java.util.stream.Collectors;
+import java.util.stream.IntStream;
+
+import org.eclipse.jdt.annotation.NonNullByDefault;
+import org.eclipse.tracecompass.internal.statesystem.core.backend.historytree.HTInterval;
+import org.eclipse.tracecompass.statesystem.core.statevalue.TmfStateValue;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
+import org.junit.runners.Parameterized.Parameters;
+
+import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableSet;
+import com.google.common.collect.Sets;
+
+/**
+ * Test the reading/writing logic in {@link HTInterval}, particularly regarding
+ * the string length limitations.
+ *
+ * @author Alexandre Montplaisir
+ */
+@RunWith(Parameterized.class)
+@NonNullByDefault({})
+public class HTIntervalStringReadWriteTest {
+
+    private static final Charset CHARSET = Charset.forName("UTF-8");
+
+    private int fNbChars;
+    private int fCharLength;
+    private char fChar;
+
+    /**
+     * Parameter generator.
+     *
+     * Generates a combination of all possible string lenghts and all possible
+     * character lengths.
+     *
+     * @return The test parameters
+     */
+    @Parameters(name = "nb of chars: {0}, char length: {1}")
+    public static Iterable<Object[]> getTestParams() {
+        Set<List<Integer>> set = Sets.cartesianProduct(ImmutableList.of(
+                ImmutableSet.of(
+                        0,
+                        10,
+                        Byte.MAX_VALUE - 1,
+                        (int) Byte.MAX_VALUE,
+                        Byte.MAX_VALUE + 1,
+
+                        Short.MAX_VALUE / 2 - 1,
+                        Short.MAX_VALUE / 2,
+                        Short.MAX_VALUE / 2 + 1,
+
+                        Short.MAX_VALUE / 3 - 1,
+                        Short.MAX_VALUE / 3,
+                        Short.MAX_VALUE / 3 + 1,
+
+                        Short.MAX_VALUE - 1,
+                        (int) Short.MAX_VALUE,
+                        Short.MAX_VALUE + 1),
+
+                ImmutableSet.of(1, 2, 3)
+                ));
+
+        return set.stream()
+                .map(List::toArray)
+                .collect(Collectors.toList());
+    }
+
+    /**
+     * Test constructor, take the generated parameters as parameters.
+     *
+     * @param nbChars
+     *            The number of characters in the test string.
+     * @param charLength
+     *            The length (in bytes) of the UTF-8-encoded form of the
+     *            character being used.
+     */
+    public HTIntervalStringReadWriteTest(Integer nbChars, Integer charLength) {
+        fNbChars = nbChars.intValue();
+        fCharLength = charLength.intValue();
+        switch (charLength) {
+        case 1:
+            fChar = 'a';
+            break;
+        case 2:
+            fChar = 'é';
+            break;
+        case 3:
+            fChar = '长'; // "chang" means long / length in Chinese!
+            break;
+        default:
+            throw new IllegalArgumentException();
+        }
+    }
+
+    /**
+     * Test method
+     *
+     * @throws IOException
+     *             Fails the test
+     */
+    @Test
+    public void testStringWithChars() throws IOException {
+        StringBuilder sb = new StringBuilder();
+        IntStream.range(0, fNbChars).forEach(i -> sb.append(fChar));
+        String str = sb.toString();
+        assertEquals(fNbChars, str.length());
+        assertEquals(fNbChars * fCharLength, str.getBytes(CHARSET).length);
+
+        TmfStateValue value = TmfStateValue.newValueString(str);
+        if (fNbChars * fCharLength > Short.MAX_VALUE) {
+            /* For sizes that are known to be too long, expect an exception */
+            try {
+                new HTInterval(0, 10, 1, value);
+            } catch (IllegalArgumentException e) {
+                return;
+            }
+            fail();
+        }
+        HTInterval interval = new HTInterval(0, 10, 1, value);
+        writeAndReadInterval(interval);
+    }
+
+    private static void writeAndReadInterval(HTInterval interval) throws IOException {
+        int sizeOnDisk = interval.getSizeOnDisk();
+
+        /* Write the interval to a file */
+        File tempFile = File.createTempFile("test-interval", ".interval");
+        try (FileOutputStream fos = new FileOutputStream(tempFile, false);
+                FileChannel fc = fos.getChannel();) {
+
+            ByteBuffer bb = ByteBuffer.allocate(sizeOnDisk);
+            bb.order(ByteOrder.LITTLE_ENDIAN);
+            bb.clear();
+
+            interval.writeInterval(bb);
+            bb.flip();
+            int written = fc.write(bb);
+            assertEquals(sizeOnDisk, written);
+        }
+
+        /* Read the interval from the file */
+        HTInterval readInterval;
+        try (FileInputStream fis = new FileInputStream(tempFile);
+                FileChannel fc = fis.getChannel();) {
+
+            ByteBuffer bb = ByteBuffer.allocate(sizeOnDisk);
+            bb.order(ByteOrder.LITTLE_ENDIAN);
+            bb.clear();
+
+            int read = fc.read(bb);
+            assertEquals(sizeOnDisk, read);
+            bb.flip();
+            readInterval = HTInterval.readFrom(bb);
+        }
+
+        assertEquals(interval, readInterval);
+    }
+}
index 925a647194e9afc2f246eb28102c153125acd7f4..7af2e4f76e4ee1680ddcbf0da996257065e1656c 100644 (file)
@@ -17,6 +17,7 @@ package org.eclipse.tracecompass.internal.statesystem.core.backend.historytree;
 
 import java.io.IOException;
 import java.nio.ByteBuffer;
+import java.nio.charset.Charset;
 import java.util.Objects;
 
 import org.eclipse.jdt.annotation.NonNull;
@@ -37,6 +38,8 @@ import org.eclipse.tracecompass.statesystem.core.statevalue.TmfStateValue;
  */
 public final class HTInterval implements ITmfStateInterval, Comparable<HTInterval> {
 
+    private static final Charset CHARSET = Charset.forName("UTF-8"); //$NON-NLS-1$
+
     private static final String errMsg = "Invalid interval data. Maybe your file is corrupt?"; //$NON-NLS-1$
 
     /* 'Byte' equivalent for state values types */
@@ -104,10 +107,17 @@ public final class HTInterval implements ITmfStateInterval, Comparable<HTInterva
         case DOUBLE:
             return (minSize + Double.BYTES);
         case STRING:
+            String str = sv.unboxStr();
+            int strLength = str.getBytes(CHARSET).length;
+
+            if (strLength > Short.MAX_VALUE) {
+                throw new IllegalArgumentException("String is too long to be stored in state system: " + str); //$NON-NLS-1$
+            }
+
             /*
-             * String's length + 2 (1 byte for size, 1 byte for \0 at the end
+             * String's length + 3 (2 bytes for size, 1 byte for \0 at the end)
              */
-            return (minSize + sv.unboxStr().getBytes().length + 2);
+            return (minSize + strLength + 3);
         case CUSTOM:
             /* Length of serialized value (short) + state value */
             return (minSize + Short.BYTES + ((CustomStateValue) sv).getSerializedSize());
@@ -182,12 +192,12 @@ public final class HTInterval implements ITmfStateInterval, Comparable<HTInterva
             break;
 
         case TYPE_STRING: {
-            /* the first byte = the size to read */
-            int valueSize = buffer.get();
+            /* the first short = the size to read */
+            int valueSize = buffer.getShort();
 
             byte[] array = new byte[valueSize];
             buffer.get(array);
-            value = TmfStateValue.newValueString(new String(array));
+            value = TmfStateValue.newValueString(new String(array, CHARSET));
 
             /* Confirm the 0'ed byte at the end */
             byte res = buffer.get();
@@ -261,13 +271,13 @@ public final class HTInterval implements ITmfStateInterval, Comparable<HTInterva
 
         case TYPE_STRING: {
             String string = sv.unboxStr();
-            byte[] strArray = string.getBytes();
+            byte[] strArray = string.getBytes(CHARSET);
 
             /*
              * Write the Strings entry (1st byte = size, then the bytes, then
              * the 0). We have checked the string length at the constructor.
              */
-            buffer.put((byte) strArray.length);
+            buffer.putShort((short) strArray.length);
             buffer.put(strArray);
             buffer.put((byte) 0);
             break;
index e4154496ac0bd10aca3261fe1d2240bd2964f81d..33d33d120e144ed5e02e5ba9e826c7d2cdedb084 100644 (file)
@@ -51,7 +51,7 @@ public class HistoryTree {
     private static final int HISTORY_FILE_MAGIC_NUMBER = 0x05FFA900;
 
     /** File format version. Increment when breaking compatibility. */
-    private static final int FILE_VERSION = 6;
+    private static final int FILE_VERSION = 7;
 
     // ------------------------------------------------------------------------
     // Tree-specific configuration
This page took 0.028141 seconds and 5 git commands to generate.