diff --git a/src/main/java/org/openrewrite/staticanalysis/ReplaceDeprecatedRuntimeExecMethods.java b/src/main/java/org/openrewrite/staticanalysis/ReplaceDeprecatedRuntimeExecMethods.java index d56a1a14f..1a1fb7101 100644 --- a/src/main/java/org/openrewrite/staticanalysis/ReplaceDeprecatedRuntimeExecMethods.java +++ b/src/main/java/org/openrewrite/staticanalysis/ReplaceDeprecatedRuntimeExecMethods.java @@ -30,6 +30,7 @@ import java.time.Duration; import java.util.ArrayList; import java.util.List; +import java.util.StringTokenizer; public class ReplaceDeprecatedRuntimeExecMethods extends Recipe { private static final MethodMatcher RUNTIME_EXEC_CMD = new MethodMatcher("java.lang.Runtime exec(String)"); @@ -41,7 +42,10 @@ public class ReplaceDeprecatedRuntimeExecMethods extends Recipe { @Getter final String description = "Replace `Runtime#exec(String)` methods to use `exec(String[])` instead because the former is deprecated " + - "after Java 18 and is no longer recommended for use by the Java documentation."; + "after Java 18 and is no longer recommended for use by the Java documentation. Only commands made up entirely of " + + "string literals are replaced, because only then is it known at compile time which arguments " + + "`Runtime#exec(String)` would build; any other command is left unchanged rather than launched with " + + "different arguments."; @Getter final Duration estimatedEffortPerOccurrence = Duration.ofMinutes(3); @@ -71,7 +75,8 @@ public J.MethodInvocation visitMethodInvocation(J.MethodInvocation method, Execu StringBuilder sb = new StringBuilder(); if (flattenAble) { for (Expression e : commands) { - if (e instanceof J.Literal && ((J.Literal) e).getType() == JavaType.Primitive.String) { + if (e instanceof J.Literal && ((J.Literal) e).getType() == JavaType.Primitive.String && + isDecoded((J.Literal) e)) { sb.append(((J.Literal) e).getValue()); } else { flattenAble = false; @@ -80,48 +85,36 @@ public J.MethodInvocation visitMethodInvocation(J.MethodInvocation method, Execu } } - updateCursor(m); - if (flattenAble) { - String[] cmds = sb.toString().split(" "); - String templateCode = String.format("new String[] {%s}", toStringArguments(cmds)); - JavaTemplate template = JavaTemplate.builder(templateCode).build(); - - List args = m.getArguments(); - Cursor cursor = new Cursor(getCursor(), args.get(0)); - args.set(0, template.apply(cursor, args.get(0).getCoordinates().replace())); - - if (m.getMethodType() != null) { - List parameterTypes = m.getMethodType().getParameterTypes(); - parameterTypes.set(0, JavaType.ShallowClass.build("java.lang.String[]")); - - return m.withArguments(args) - .withMethodType(m.getMethodType().withParameterTypes(parameterTypes)); - } - } else { - // replace argument to 'command.split(" ")' - List args = m.getArguments(); - boolean needWrap = false; - Expression arg0 = args.get(0); - if (!(arg0 instanceof J.Identifier) && - !(arg0 instanceof J.Literal) && - !(arg0 instanceof J.MethodInvocation)) { - needWrap = true; - } - - String code = needWrap ? "(#{any()}).split(\" \")" : "#{any()}.split(\" \")"; - JavaTemplate template = JavaTemplate.builder(code).contextSensitive().build(); - Cursor cursor = new Cursor(getCursor(), args.get(0)); - arg0 = template.apply(cursor, args.get(0).getCoordinates().replace(), args.get(0)); - args.set(0, arg0); - - if (m.getMethodType() != null) { - List parameterTypes = m.getMethodType().getParameterTypes(); - parameterTypes.set(0, JavaType.ShallowClass.build("java.lang.String[]")); + // Only an all-literal command can be tokenized the way `Runtime#exec(String)` does, on any of + // ' ', '\t', '\n', '\r' and '\f', collapsing runs; anything else would launch a different process + if (!flattenAble) { + return m; + } - return m.withArguments(args).withMethodType(m.getMethodType().withParameterTypes(parameterTypes)); - } + List cmds = new ArrayList<>(); + for (StringTokenizer tokenizer = new StringTokenizer(sb.toString()); tokenizer.hasMoreTokens(); ) { + cmds.add(tokenizer.nextToken()); + } + // `exec("")` throws `IllegalArgumentException` where `exec(new String[]{})` throws + // `IndexOutOfBoundsException`, so a command tokenizing to nothing is left alone + JavaType.Method methodType = m.getMethodType(); + if (cmds.isEmpty() || methodType == null) { return m; } + + updateCursor(m); + JavaTemplate template = JavaTemplate.builder(String.format("new String[] {%s}", toStringArguments(cmds))).build(); + + List args = m.getArguments(); + Cursor cursor = new Cursor(getCursor(), args.get(0)); + args.set(0, template.apply(cursor, args.get(0).getCoordinates().replace())); + + // `getParameterTypes()` writes through to the interned `JavaType.Method` shared by every call + // of this overload, so copy before replacing + List parameterTypes = new ArrayList<>(methodType.getParameterTypes()); + parameterTypes.set(0, new JavaType.Array(null, JavaType.ShallowClass.build("java.lang.String"), null)); + return m.withArguments(args) + .withMethodType(methodType.withParameterTypes(parameterTypes)); } return m; @@ -129,17 +122,42 @@ public J.MethodInvocation visitMethodInvocation(J.MethodInvocation method, Execu }); } - private static String toStringArguments(String[] cmds) { + // getValue() is not fully decoded when the literal contains Unicode escapes. + private static boolean isDecoded(J.Literal literal) { + List unicodeEscapes = literal.getUnicodeEscapes(); + return literal.getValue() != null && (unicodeEscapes == null || unicodeEscapes.isEmpty()); + } + + private static final String TEMPLATE_INTERPOLATION_START = "#{"; + + private static String toStringArguments(List cmds) { StringBuilder sb = new StringBuilder(); - for (int i = 0; i < cmds.length; i++) { - String token = cmds[i]; - if (i != 0) { + for (String token : cmds) { + if (sb.length() != 0) { sb.append(", "); } - sb.append("\"") - .append(token) - .append("\""); + sb.append('"'); + for (int i = 0; i < token.length(); i++) { + char character = token.charAt(i); + if (character == '"' || character == '\\') { + // Prefix a backslash so the character survives inside the generated string literal + sb.append('\\').append(character); + } else if (Character.isISOControl(character) || + token.startsWith(TEMPLATE_INTERPOLATION_START, i)) { + // Control characters stay legible rather than becoming invisible bytes, and `#{` needs escaping + // because this text is also `JavaTemplate` source + sb.append(unicodeEscape(character)); + } else { + sb.append(character); + } + } + sb.append('"'); } return sb.toString(); } + + // Formats the character as a Java Unicode escape, `\u0007` for the bell character + private static String unicodeEscape(char character) { + return String.format("\\u%04x", (int) character); + } } diff --git a/src/main/resources/META-INF/rewrite/recipes.csv b/src/main/resources/META-INF/rewrite/recipes.csv index bdfbd9e95..5fbb0111b 100644 --- a/src/main/resources/META-INF/rewrite/recipes.csv +++ b/src/main/resources/META-INF/rewrite/recipes.csv @@ -143,7 +143,7 @@ According to the `Collection#toArray(T[])` documentation: However, although it's not intuitive, allocating a right-sized array ahead of time to pass to the API appears to be [generally worse for performance](https://shipilev.net/blog/2016/arrays-wisdom-ancients/#_conclusion) according to benchmarking and JVM developers due to a number of implementation details in both Java and the virtual machine. H2 achieved significant performance gains by [switching to empty arrays instead pre-sized ones](https://github.com/h2database/h2database/issues/311).",1,,Static analysis and remediation,,Remediations for issues identified by SAST tools.,, -maven,org.openrewrite.recipe:rewrite-static-analysis,org.openrewrite.staticanalysis.ReplaceDeprecatedRuntimeExecMethods,Replace deprecated `Runtime#exec()` methods,Replace `Runtime#exec(String)` methods to use `exec(String[])` instead because the former is deprecated after Java 18 and is no longer recommended for use by the Java documentation.,1,,Static analysis and remediation,,Remediations for issues identified by SAST tools.,, +maven,org.openrewrite.recipe:rewrite-static-analysis,org.openrewrite.staticanalysis.ReplaceDeprecatedRuntimeExecMethods,Replace deprecated `Runtime#exec()` methods,"Replace `Runtime#exec(String)` methods to use `exec(String[])` instead because the former is deprecated after Java 18 and is no longer recommended for use by the Java documentation. Only commands made up entirely of string literals are replaced, because only then is it known at compile time which arguments `Runtime#exec(String)` would build; any other command is left unchanged rather than launched with different arguments.",1,,Static analysis and remediation,,Remediations for issues identified by SAST tools.,, maven,org.openrewrite.recipe:rewrite-static-analysis,org.openrewrite.staticanalysis.ReplaceDuplicateStringLiterals,Replace duplicate `String` literals,"Replaces `String` literals with a length of 5 or greater repeated a minimum of 3 times. Qualified `String` literals include final Strings, method invocations, and new class invocations. Adds a new `private static final String` or uses an existing equivalent class field. A new variable name will be generated based on the literal value if an existing field does not exist. The generated name will append a numeric value to the variable name if a name already exists in the compilation unit. Centralizing repeated string values into constants makes refactoring safer and reduces the risk of inconsistent updates.",1,,Static analysis and remediation,,Remediations for issues identified by SAST tools.,"[{""name"":""includeTestSources"",""type"":""Boolean"",""displayName"":""Apply recipe to test source set"",""description"":""Changes only apply to main by default. `includeTestSources` will apply the recipe to `test` source files.""}]", maven,org.openrewrite.recipe:rewrite-static-analysis,org.openrewrite.staticanalysis.ReplaceHashtableWithHashMap,Replace `java.util.Hashtable` with `java.util.HashMap`,"`Hashtable` synchronizes every operation, which adds overhead in the common single-threaded case. This recipe replaces a local `Hashtable` with a `HashMap` when data flow analysis can prove the `Hashtable` never escapes its method (it is not returned, assigned to a field, or passed as an argument), so no other thread can observe it and the synchronization is redundant. Fields, escaping variables, and `Hashtable`-specific method usages (`contains`, `elements`, `keys`) are left untouched. `HashMap` permits `null` keys and values, so it accepts every input `Hashtable` did.",1,,Static analysis and remediation,,Remediations for issues identified by SAST tools.,,"[{""name"":""org.openrewrite.staticanalysis.table.LegacySynchronizedTypesNotMigrated"",""displayName"":""Legacy synchronized types not migrated"",""instanceName"":""Legacy synchronized types not migrated"",""description"":""Instances of a legacy synchronized type (`Hashtable`, `Vector`, `Stack`, `StringBuffer`) that were found but left unchanged because they could not be proven safe to modernize."",""columns"":[{""name"":""sourcePath"",""type"":""String"",""displayName"":""Source path"",""description"":""The path to the source file containing the unmigrated reference.""},{""name"":""enclosingClass"",""type"":""String"",""displayName"":""Class"",""description"":""The fully qualified name of the class containing the reference.""},{""name"":""unmigratedType"",""type"":""String"",""displayName"":""Unmigrated type"",""description"":""The fully qualified name of the legacy synchronized type that was found but not migrated.""},{""name"":""reason"",""type"":""String"",""displayName"":""Reason"",""description"":""Why the instance was left unchanged.""}]}]" maven,org.openrewrite.recipe:rewrite-static-analysis,org.openrewrite.staticanalysis.ReplaceLambdaWithMethodReference,Use method references in lambda,"Replaces the single statement lambdas `o -> o instanceOf X`, `o -> (A) o`, `o -> System.out.println(o)`, `o -> o != null`, `o -> o == null` with the equivalent method reference. Method references are often more concise and readable than their lambda equivalents, making the code's intent clearer at a glance.",1,,Static analysis and remediation,,Remediations for issues identified by SAST tools.,, diff --git a/src/test/java/org/openrewrite/staticanalysis/ReplaceDeprecatedRuntimeExecMethodsTest.java b/src/test/java/org/openrewrite/staticanalysis/ReplaceDeprecatedRuntimeExecMethodsTest.java index 18de6d4ef..8c0ad6e46 100644 --- a/src/test/java/org/openrewrite/staticanalysis/ReplaceDeprecatedRuntimeExecMethodsTest.java +++ b/src/test/java/org/openrewrite/staticanalysis/ReplaceDeprecatedRuntimeExecMethodsTest.java @@ -73,7 +73,7 @@ void method() throws IOException { } @Test - void stringVariableAsInput() { + void repeatedDelimitersInRawString() { rewriteRun( version( //language=java @@ -82,31 +82,173 @@ void stringVariableAsInput() { import java.io.File; import java.io.IOException; + class A { + void method(Runtime runtime, String[] envp, File dir) throws IOException { + runtime.exec("printf '%s' value"); + runtime.exec("printf '%s' value", envp); + runtime.exec("printf '%s' value", envp, dir); + } + } + """, + """ + import java.io.File; + import java.io.IOException; + + class A { + void method(Runtime runtime, String[] envp, File dir) throws IOException { + runtime.exec(new String[]{"printf", "'%s'", "value"}); + runtime.exec(new String[]{"printf", "'%s'", "value"}, envp); + runtime.exec(new String[]{"printf", "'%s'", "value"}, envp, dir); + } + } + """ + ), 18) + ); + } + + @Test + void tokenizeRawStrings() { + rewriteRun( + version( + //language=java + java( + """ + import java.io.IOException; + + class A { + void method(Runtime runtime) throws IOException { + runtime.exec(" \\tls\\n-a \\r\\r -l\\f-h "); + runtime.exec("echo \\"a b\\" C:\\\\dir"); + runtime.exec("echo a\\u000bb \\u007f"); + runtime.exec("sed -e s/#{a}/b/ f.txt"); + } + } + """, + """ + import java.io.IOException; + + class A { + void method(Runtime runtime) throws IOException { + runtime.exec(new String[]{"ls", "-a", "-l", "-h"}); + runtime.exec(new String[]{"echo", "\\"a", "b\\"", "C:\\\\dir"}); + runtime.exec(new String[]{"echo", "a\\u000bb", "\\u007f"}); + runtime.exec(new String[]{"sed", "-e", "s/\\u0023{a}/b/", "f.txt"}); + } + } + """ + ), 18) + ); + } + + @Test + void everyCallOfTheSameOverloadIsReplaced() { + rewriteRun( + version( + //language=java + java( + """ + import java.io.IOException; + + class A { + void method(Runtime runtime) throws IOException { + runtime.exec("ls -a"); + runtime.exec("ps -e"); + } + } + """, + """ + import java.io.IOException; + + class A { + void method(Runtime runtime) throws IOException { + runtime.exec(new String[]{"ls", "-a"}); + runtime.exec(new String[]{"ps", "-e"}); + } + } + """ + ), 18), + version( + //language=java + java( + """ + import java.io.IOException; + class B { - void method() throws IOException { - Runtime runtime = Runtime.getRuntime(); - String command = "ls -al"; - String[] envp = { "E1=1", "E2=2"}; - File dir = new File("/tmp"); - Process process1 = runtime.exec(command); - Process process2 = runtime.exec(command, envp); - Process process3 = runtime.exec(command, envp, dir); + void method(Runtime runtime) throws IOException { + runtime.exec("df -h"); } } """, + """ + import java.io.IOException; + + class B { + void method(Runtime runtime) throws IOException { + runtime.exec(new String[]{"df", "-h"}); + } + } + """ + ), 18) + ); + } + + @Test + void rawStringWithSideEffectingEnvironmentAndDirectory() { + rewriteRun( + version( + //language=java + java( + """ + import java.io.File; + import java.io.IOException; + + class A { + String[] envp() { + return new String[]{"E1=1"}; + } + File dir() { + return new File("/tmp"); + } + void method(Runtime runtime) throws IOException { + runtime.exec("ls -a", envp(), dir()); + } + } + """, + """ + import java.io.File; + import java.io.IOException; + + class A { + String[] envp() { + return new String[]{"E1=1"}; + } + File dir() { + return new File("/tmp"); + } + void method(Runtime runtime) throws IOException { + runtime.exec(new String[]{"ls", "-a"}, envp(), dir()); + } + } + """ + ), 18) + ); + } + + @Test + void doNotChangeStringVariableAsInput() { + rewriteRun( + version( + //language=java + java( """ import java.io.File; import java.io.IOException; class B { - void method() throws IOException { - Runtime runtime = Runtime.getRuntime(); - String command = "ls -al"; - String[] envp = { "E1=1", "E2=2"}; - File dir = new File("/tmp"); - Process process1 = runtime.exec(command.split(" ")); - Process process2 = runtime.exec(command.split(" "), envp); - Process process3 = runtime.exec(command.split(" "), envp, dir); + void method(Runtime runtime, String command, String[] envp, File dir) throws IOException { + Process process1 = runtime.exec(command); + Process process2 = runtime.exec(command, envp); + Process process3 = runtime.exec(command, envp, dir); } } """ @@ -115,7 +257,7 @@ void method() throws IOException { } @Test - void methodInvocationAsInput() { + void doNotChangeMethodInvocationAsInput() { rewriteRun( version( //language=java @@ -127,22 +269,49 @@ class B { String command() { return "ls -al"; } - void method() throws IOException { - Runtime runtime = Runtime.getRuntime(); + void method(Runtime runtime) throws IOException { Process process = runtime.exec(command()); } } - """, + """ + ), 18) + ); + } + + @Test + void doNotChangeCommandsThatFailAtRuntime() { + rewriteRun( + version( + //language=java + java( """ import java.io.IOException; - class B { - String command() { - return "ls -al"; + class A { + void method(Runtime runtime) throws IOException { + runtime.exec(""); + runtime.exec(" "); + runtime.exec("\\t\\n\\r\\f"); + runtime.exec((String) null); } - void method() throws IOException { - Runtime runtime = Runtime.getRuntime(); - Process process = runtime.exec(command().split(" ")); + } + """ + ), 18) + ); + } + + @Test + void doNotChangeCommandWithSupplementaryCharacterEscape() { + rewriteRun( + version( + //language=java + java( + """ + import java.io.IOException; + + class A { + void method(Runtime runtime) throws IOException { + runtime.exec("echo \\ud83d\\ude00x"); } } """ @@ -181,7 +350,7 @@ void method() throws IOException { } @Test - void concatenatedObjectsAsInput() { + void doNotChangeConcatenatedObjectsAsInput() { rewriteRun( version( //language=java @@ -191,22 +360,10 @@ void concatenatedObjectsAsInput() { class B { String options = "-a -l"; - void method() throws IOException { - Runtime runtime = Runtime.getRuntime(); + void method(Runtime runtime) throws IOException { Process process = runtime.exec("ls" + " " + options); } } - """, - """ - import java.io.IOException; - - class B { - String options = "-a -l"; - void method() throws IOException { - Runtime runtime = Runtime.getRuntime(); - Process process = runtime.exec(("ls" + " " + options).split(" ")); - } - } """ ), 18) );