From 8182f36f96bbe3f33cc4c4838853239d2efe315a Mon Sep 17 00:00:00 2001 From: Alexandre Montplaisir Date: Mon, 27 Jun 2016 18:09:36 -0400 Subject: [PATCH] ss: Relax but enforce the character limit in HTInterval strings 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 Reviewed-on: https://git.eclipse.org/r/76060 Reviewed-by: Hudson CI --- .../HTIntervalStringReadWriteTest.java | 184 ++++++++++++++++++ .../core/backend/historytree/HTInterval.java | 24 ++- .../core/backend/historytree/HistoryTree.java | 2 +- 3 files changed, 202 insertions(+), 8 deletions(-) create mode 100644 statesystem/org.eclipse.tracecompass.statesystem.core.tests/src/org/eclipse/tracecompass/statesystem/core/tests/backend/historytree/HTIntervalStringReadWriteTest.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 index 0000000000..5f82e2c1a4 --- /dev/null +++ b/statesystem/org.eclipse.tracecompass.statesystem.core.tests/src/org/eclipse/tracecompass/statesystem/core/tests/backend/historytree/HTIntervalStringReadWriteTest.java @@ -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 getTestParams() { + Set> 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); + } +} diff --git a/statesystem/org.eclipse.tracecompass.statesystem.core/src/org/eclipse/tracecompass/internal/statesystem/core/backend/historytree/HTInterval.java b/statesystem/org.eclipse.tracecompass.statesystem.core/src/org/eclipse/tracecompass/internal/statesystem/core/backend/historytree/HTInterval.java index 925a647194..7af2e4f76e 100644 --- a/statesystem/org.eclipse.tracecompass.statesystem.core/src/org/eclipse/tracecompass/internal/statesystem/core/backend/historytree/HTInterval.java +++ b/statesystem/org.eclipse.tracecompass.statesystem.core/src/org/eclipse/tracecompass/internal/statesystem/core/backend/historytree/HTInterval.java @@ -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 { + 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 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