8233333 : Incorrect comparison of number version strings in ToolValidator
Submitted-by: asemenyuk
Reviewed-by: aherrick, almatvee
--- a/src/jdk.jpackage/linux/classes/jdk/jpackage/internal/LinuxRpmBundler.java Fri Nov 01 10:22:49 2019 -0400
+++ b/src/jdk.jpackage/linux/classes/jdk/jpackage/internal/LinuxRpmBundler.java Fri Nov 01 10:25:15 2019 -0400
@@ -107,7 +107,8 @@
public final static String TOOL_RPM = "rpm";
public final static String TOOL_RPMBUILD = "rpmbuild";
- public final static String TOOL_RPMBUILD_MIN_VERSION = "4.0";
+ public final static DottedVersion TOOL_RPMBUILD_MIN_VERSION = DottedVersion.lazy(
+ "4.0");
public LinuxRpmBundler() {
super(PACKAGE_NAME);
--- a/src/jdk.jpackage/share/classes/jdk/jpackage/internal/DottedVersion.java Fri Nov 01 10:22:49 2019 -0400
+++ b/src/jdk.jpackage/share/classes/jdk/jpackage/internal/DottedVersion.java Fri Nov 01 10:25:15 2019 -0400
@@ -28,6 +28,7 @@
import java.util.ArrayList;
import java.util.List;
import java.util.Objects;
+import java.util.regex.Pattern;
/**
* Dotted numeric version string.
@@ -36,14 +37,29 @@
class DottedVersion implements Comparable<String> {
public DottedVersion(String version) {
- components = parseVersionString(version);
+ greedy = true;
+ components = parseVersionString(version, greedy);
+ value = version;
+ }
+
+ private DottedVersion(String version, boolean greedy) {
+ this.greedy = greedy;
+ components = parseVersionString(version, greedy);
value = version;
}
+ public static DottedVersion greedy(String version) {
+ return new DottedVersion(version);
+ }
+
+ public static DottedVersion lazy(String version) {
+ return new DottedVersion(version, false);
+ }
+
@Override
public int compareTo(String o) {
int result = 0;
- int[] otherComponents = parseVersionString(o);
+ int[] otherComponents = parseVersionString(o, greedy);
for (int i = 0; i < Math.min(components.length, otherComponents.length)
&& result == 0; ++i) {
result = components[i] - otherComponents[i];
@@ -56,9 +72,12 @@
return result;
}
- private static int[] parseVersionString(String version) {
+ private static int[] parseVersionString(String version, boolean greedy) {
Objects.requireNonNull(version);
if (version.isEmpty()) {
+ if (!greedy) {
+ return new int[] {0};
+ }
throw new IllegalArgumentException("Version may not be empty string");
}
@@ -66,17 +85,36 @@
List<Integer> components = new ArrayList<>();
for (var component : version.split("\\.", -1)) {
if (component.isEmpty()) {
+ if (!greedy) {
+ break;
+ }
+
throw new IllegalArgumentException(String.format(
"Version [%s] contains a zero lenght component", version));
}
- int num = Integer.parseInt(component);
- if (num < 0) {
+ if (!DIGITS.matcher(component).matches()) {
+ // Catch "+N" and "-N" cases.
+ if (!greedy) {
+ break;
+ }
+
throw new IllegalArgumentException(String.format(
"Version [%s] contains invalid component [%s]", version,
component));
}
+ final int num;
+ try {
+ num = Integer.parseInt(component);
+ } catch (NumberFormatException ex) {
+ if (!greedy) {
+ break;
+ }
+
+ throw ex;
+ }
+
if (num != 0) {
lastNotZeroIdx = components.size();
}
@@ -88,6 +126,9 @@
components = components.subList(0, lastNotZeroIdx + 1);
}
+ if (components.isEmpty()) {
+ components.add(0);
+ }
return components.stream().mapToInt(Integer::intValue).toArray();
}
@@ -98,4 +139,7 @@
final private int[] components;
final private String value;
+ final private boolean greedy;
+
+ private static final Pattern DIGITS = Pattern.compile("\\d+");
}
--- a/src/jdk.jpackage/share/classes/jdk/jpackage/internal/ToolValidator.java Fri Nov 01 10:22:49 2019 -0400
+++ b/src/jdk.jpackage/share/classes/jdk/jpackage/internal/ToolValidator.java Fri Nov 01 10:25:15 2019 -0400
@@ -58,7 +58,7 @@
return this;
}
- ToolValidator setMinimalVersion(String v) {
+ ToolValidator setMinimalVersion(Comparable<String> v) {
minimalVersion = v;
return this;
}
@@ -129,7 +129,7 @@
private final Path toolPath;
private List<String> args;
- private String minimalVersion;
+ private Comparable<String> minimalVersion;
private Function<Stream<String>, String> versionParser;
private BiFunction<String, IOException, ConfigException> toolNotFoundErrorHandler;
private BiFunction<String, String, ConfigException> toolOldVersionErrorHandler;
--- a/src/jdk.jpackage/windows/classes/jdk/jpackage/internal/WixTool.java Fri Nov 01 10:22:49 2019 -0400
+++ b/src/jdk.jpackage/windows/classes/jdk/jpackage/internal/WixTool.java Fri Nov 01 10:25:15 2019 -0400
@@ -114,7 +114,7 @@
})::validate;
}
- private final static String MINIMAL_VERSION = "3.0";
+ private final static DottedVersion MINIMAL_VERSION = DottedVersion.lazy("3.0");
static Path getSystemDir(String envVar, String knownDir) {
return Optional
@@ -132,7 +132,6 @@
} catch (InvalidPathException ex) {
Log.error(MessageFormat.format(I18N.getString(
"error.invalid-envvar"), envVar));
- return null;
}
}
return null;
@@ -142,9 +141,9 @@
PathMatcher wixInstallDirMatcher = FileSystems.getDefault().getPathMatcher(
"glob:WiX Toolset v*");
- Path programFiles = getSystemDir("ProgramFiles", "Program Files");
- Path programFilesX86 = getSystemDir("ProgramFiles(X86)",
- "Program Files (x86)");
+ Path programFiles = getSystemDir("ProgramFiles", "\\Program Files");
+ Path programFilesX86 = getSystemDir("ProgramFiles(x86)",
+ "\\Program Files (x86)");
// Returns list of WiX install directories ordered by WiX version number.
// Newer versions go first.
--- a/test/jdk/tools/jpackage/TEST.properties Fri Nov 01 10:22:49 2019 -0400
+++ b/test/jdk/tools/jpackage/TEST.properties Fri Nov 01 10:25:15 2019 -0400
@@ -1,2 +1,3 @@
keys=jpackagePlatformPackage
requires.properties=jpackage.test.SQETest
+maxOutputSize=2000000
--- a/test/jdk/tools/jpackage/junit/jdk/jpackage/internal/CompareDottedVersionTest.java Fri Nov 01 10:22:49 2019 -0400
+++ b/test/jdk/tools/jpackage/junit/jdk/jpackage/internal/CompareDottedVersionTest.java Fri Nov 01 10:25:15 2019 -0400
@@ -22,10 +22,9 @@
*/
package jdk.jpackage.internal;
+import java.util.ArrayList;
import java.util.List;
-import java.util.stream.Collectors;
-import java.util.stream.Stream;
-import junit.framework.*;
+import java.util.function.Function;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.junit.runners.Parameterized;
@@ -35,24 +34,46 @@
@RunWith(Parameterized.class)
public class CompareDottedVersionTest {
- public CompareDottedVersionTest(String version1, String version2, int result) {
+ public CompareDottedVersionTest(boolean greedy, String version1,
+ String version2, int result) {
this.version1 = version1;
this.version2 = version2;
this.expectedResult = result;
+
+ if (greedy) {
+ createTestee = DottedVersion::greedy;
+ } else {
+ createTestee = DottedVersion::lazy;
+ }
}
@Parameters
public static List<Object[]> data() {
- return List.of(new Object[][] {
- { "00.0.0", "0", 0 },
- { "0.035", "0.0035", 0 },
- { "1", "1", 0 },
- { "2", "2.0", 0 },
- { "2.00", "2.0", 0 },
- { "1.2.3.4", "1.2.3.4.5", -1 },
- { "34", "33", 1 },
- { "34.0.78", "34.1.78", -1 }
- });
+ List<Object[]> data = new ArrayList<>();
+ for (var greedy : List.of(true, false)) {
+ data.addAll(List.of(new Object[][] {
+ { greedy, "00.0.0", "0", 0 },
+ { greedy, "0.035", "0.0035", 0 },
+ { greedy, "1", "1", 0 },
+ { greedy, "2", "2.0", 0 },
+ { greedy, "2.00", "2.0", 0 },
+ { greedy, "1.2.3.4", "1.2.3.4.5", -1 },
+ { greedy, "34", "33", 1 },
+ { greedy, "34.0.78", "34.1.78", -1 }
+ }));
+ }
+
+ data.addAll(List.of(new Object[][] {
+ { false, "", "1", -1 },
+ { false, "1.2.4-R4", "1.2.4-R5", 0 },
+ { false, "1.2.4.-R4", "1.2.4.R5", 0 },
+ { false, "7+1", "7+4", 0 },
+ { false, "2+14", "2-14", 0 },
+ { false, "23.4.RC4", "23.3.RC10", 1 },
+ { false, "77.0", "77.99999999999999999999999999999999999999999999999", 0 },
+ }));
+
+ return data;
}
@Test
@@ -64,8 +85,8 @@
assertEquals(actualResult, -1 * actualNegateResult);
}
- private static int compare(String x, String y) {
- int result = new DottedVersion(x).compareTo(y);
+ private int compare(String x, String y) {
+ int result = createTestee.apply(x).compareTo(y);
if (result < 0) {
return -1;
@@ -81,4 +102,5 @@
private final String version1;
private final String version2;
private final int expectedResult;
+ private final Function<String, DottedVersion> createTestee;
}
--- a/test/jdk/tools/jpackage/junit/jdk/jpackage/internal/DottedVersionTest.java Fri Nov 01 10:22:49 2019 -0400
+++ b/test/jdk/tools/jpackage/junit/jdk/jpackage/internal/DottedVersionTest.java Fri Nov 01 10:25:15 2019 -0400
@@ -22,29 +22,73 @@
*/
package jdk.jpackage.internal;
+import java.util.Collections;
+import java.util.List;
+import java.util.function.Function;
import java.util.stream.Stream;
-import org.junit.Assert;
import org.junit.Rule;
import org.junit.Test;
import org.junit.rules.ExpectedException;
import static org.junit.Assert.*;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
+@RunWith(Parameterized.class)
public class DottedVersionTest {
+ public DottedVersionTest(boolean greedy) {
+ this.greedy = greedy;
+ if (greedy) {
+ createTestee = DottedVersion::greedy;
+ } else {
+ createTestee = DottedVersion::lazy;
+ }
+ }
+
+ @Parameterized.Parameters
+ public static List<Object[]> data() {
+ return List.of(new Object[] { true }, new Object[] { false });
+ }
+
@Rule
public ExpectedException exceptionRule = ExpectedException.none();
@Test
public void testValid() {
- Stream.of(
+ final List<String> validStrings = List.of(
"1.0",
"1",
"2.234.045",
"2.234.0",
"0",
"0.1"
- ).forEach(value -> {
- DottedVersion version = new DottedVersion(value);
+ );
+
+ final List<String> validLazyStrings;
+ if (greedy) {
+ validLazyStrings = Collections.emptyList();
+ } else {
+ validLazyStrings = List.of(
+ "1.-1",
+ "5.",
+ "4.2.",
+ "3..2",
+ "2.a",
+ "0a",
+ ".",
+ " ",
+ " 1",
+ "1. 2",
+ "+1",
+ "-1",
+ "-0",
+ "1234567890123456789012345678901234567890"
+ );
+ }
+
+ Stream.concat(validStrings.stream(), validLazyStrings.stream())
+ .forEach(value -> {
+ DottedVersion version = createTestee.apply(value);
assertEquals(version.toString(), value);
});
}
@@ -52,13 +96,21 @@
@Test
public void testNull() {
exceptionRule.expect(NullPointerException.class);
- new DottedVersion(null);
+ createTestee.apply(null);
}
@Test
public void testEmpty() {
- exceptionRule.expect(IllegalArgumentException.class);
- exceptionRule.expectMessage("Version may not be empty string");
- new DottedVersion("");
+ if (greedy) {
+ exceptionRule.expect(IllegalArgumentException.class);
+ exceptionRule.expectMessage("Version may not be empty string");
+ createTestee.apply("");
+ } else {
+ assertTrue(0 == createTestee.apply("").compareTo(""));
+ assertTrue(0 == createTestee.apply("").compareTo("0"));
+ }
}
+
+ private final boolean greedy;
+ private final Function<String, DottedVersion> createTestee;
}
--- a/test/jdk/tools/jpackage/junit/jdk/jpackage/internal/InvalidDottedVersionTest.java Fri Nov 01 10:22:49 2019 -0400
+++ b/test/jdk/tools/jpackage/junit/jdk/jpackage/internal/InvalidDottedVersionTest.java Fri Nov 01 10:25:15 2019 -0400
@@ -51,7 +51,11 @@
".",
" ",
" 1",
- "1. 2"
+ "1. 2",
+ "+1",
+ "-1",
+ "-0",
+ "1234567890123456789012345678901234567890"
).map(version -> new Object[] { version }).collect(Collectors.toList());
}
--- a/test/jdk/tools/jpackage/junit/jdk/jpackage/internal/ToolValidatorTest.java Fri Nov 01 10:22:49 2019 -0400
+++ b/test/jdk/tools/jpackage/junit/jdk/jpackage/internal/ToolValidatorTest.java Fri Nov 01 10:25:15 2019 -0400
@@ -51,13 +51,21 @@
}).validate();
// Minimal version is 1, actual is 10. Should be OK.
- assertNull(
- new ToolValidator(TOOL_JAVA).setMinimalVersion(
- "1").setVersionParser(unused -> "10").validate());
+ assertNull(new ToolValidator(TOOL_JAVA).setMinimalVersion(
+ new DottedVersion("1")).setVersionParser(unused -> "10").validate());
// Minimal version is 5, actual is 4.99.37. Error expected.
assertValidationFailure(new ToolValidator(TOOL_JAVA).setMinimalVersion(
- "5").setVersionParser(unused -> "4.99.37").validate(), false);
+ new DottedVersion("5")).setVersionParser(unused -> "4.99.37").validate(),
+ false);
+
+ // Minimal version is 8, actual is 10, lexicographical comparison is used. Error expected.
+ assertValidationFailure(new ToolValidator(TOOL_JAVA).setMinimalVersion(
+ "8").setVersionParser(unused -> "10").validate(), false);
+
+ // Minimal version is 8, actual is 10, Use DottedVersion class for comparison. Should be OK.
+ assertNull(new ToolValidator(TOOL_JAVA).setMinimalVersion(
+ new DottedVersion("8")).setVersionParser(unused -> "10").validate());
}
private static void assertValidationFailure(ConfigException v,