ReplaceDeprecatedRuntimeExecMethods: preserve the argument vector - #976
Draft
martinfrancois wants to merge 7 commits into
Draft
ReplaceDeprecatedRuntimeExecMethods: preserve the argument vector#976martinfrancois wants to merge 7 commits into
martinfrancois wants to merge 7 commits into
Conversation
`Runtime#exec(String)` tokenizes its command with `StringTokenizer`,
splitting on ' ', '\t', '\n', '\r' and '\f' and collapsing runs of
them. The recipe built the replacement array with `split(" ")`
instead, so `exec("printf '%s' value")` became `new
String[]{"printf", "", "'%s'", "", "value"}` and no other whitespace
was split at all. The rewritten call could launch a process with
different arguments than the original.
Tokenize literal commands with `StringTokenizer` itself, and leave
every other command unchanged, since `command.split(" ")` cannot
reproduce the tokenizer at runtime. A command that tokenizes to
nothing is also left alone, because `exec("")` throws
`IllegalArgumentException` where `exec(new String[]{})` throws
`IndexOutOfBoundsException`. Generated tokens are escaped so that
quotes, backslashes, control characters and a literal `#{` survive the
`JavaTemplate` round trip, and a literal the parser did not decode is
declined.
Worth weighing: the recipe can no longer modernize non-literal
commands, which the description and the `recipes.csv` row now state,
and three existing tests that asserted `.split(" ")` output for a
variable, a method invocation and a concatenation with a non-constant
operand now assert no change. The replacement also copies the
parameter type list before overwriting it, because `JavaType.Method`
is interned and that list is a write through view shared with every
other call of the same overload.
martinfrancois
force-pushed
the
fix/runtime-exec-whitespace-tokenization
branch
from
August 16, 2026 20:17
93eabae to
8e69acc
Compare
martinfrancois
marked this pull request as draft
August 17, 2026 08:08
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Suggested review order: 17 of 52 (Score: 6.5)
Review first: #966
What's changed?
ReplaceDeprecatedRuntimeExecMethodsnow writes the same argument array thatRuntime#exec(String)builds internally before it passes the arguments on toexec(String[], String[], File).Before
Actual after the recipe
Using current
main.Expected after the recipe
The corrected result is shown below.
The recipe now tokenizes the command itself, while the recipe runs, with
new StringTokenizer(command), the classRuntime#exec(String)itself uses, and writes the resulting tokens into the source as a fixednew String[]{...}. NoStringTokenizercall is written into anyone's source. Each token is escaped before it goes into theJavaTemplatesource, so a double quote, a backslash, a control character or a#{sequence survives the round trip as itself.The recipe also copies the parameter type list held by the
JavaType.Methodof the call it is converting before replacing that list's first entry, instead of writing into the shared list.The set of call shapes the recipe converts is narrower than on
main. A call is converted only when every operand of the command expression is aStringliteral, soexec("ls -a")andexec("ls" + " " + "-a")are still converted, whileexec(command)for aStringvariable,exec(command())for a method call, andexec(LS + " -a")for astatic final String LSare all left exactly as written. Two literal-only cases are also left as written: a command that tokenizes to no tokens at all, such asexec("")orexec(" "), and a literal the parser did not decode, which happens when a supplementary character is written as a pair of unicode escapes, as in the source textruntime.exec("echo \ud83d\ude00x").The recipe description now states which calls are converted, and this recipe's row in
src/main/resources/META-INF/rewrite/recipes.csv, the third file in the diff, was regenerated to match. No other row changed.What's your motivation?
Recipe:
org.openrewrite.staticanalysis.ReplaceDeprecatedRuntimeExecMethods.The code that
mainproduces runs a different command. All three deprecatedStringoverloads end up inexec(String, String[], File), which tokenizes on any of" \t\n\r\f", collapses runs of them, and ignores leading and trailing ones.String#split(" "), which the recipe uses onmain, does none of that: in the example above the argument array has five elements instead of three, two of them empty.The implementation on
mainhas three further problems, all separate from that tokenization:runtime.exec("echo \\u0041 b")the backslash is itself escaped, so at run time the command value holds the six characters\,u,0,0,4,1betweenechoandb. Onmainthe recipe writes that token back with a single backslash, so the output source text isruntime.exec(new String[]{"echo", "\u0041", "b"}). There\u0041is a unicode escape, so the compiler reads the argument as the single letterA, and the compiled program passes a different argument than before with no warning. This change writes"\\u0041"instead, and the argument keeps its six characters.mainthe inputruntime.exec("echo \"a b\" C:\\dir")produces that failure.JavaType.Methodis interned, so every call of the same overload shares one instance, and onmainthe recipe writes into that shared instance's parameter type list. Once the first call is converted,MethodMatcher("java.lang.Runtime exec(String)")matches no call of that overload any more, so no later call is converted.I reproduced all four problems on 2.40.0, the newest release, and on 2.41.0-SNAPSHOT built from
mainat 5785534, which produce the same output on these inputs, so every statement aboutmainbehaviour here holds for 2.40.0 as well.Confirmed real-world executions
All six executions used
org.openrewrite.recipe:rewrite-static-analysis:2.41.0.ExecEmptyString.javaatb8207347ExecEmptyString.javaatadf16cc8ExecEmptyString.javaat65345899ExecEmptyString.javaat995ac3e1OldRuntimeTest.javaatef091902AdminTask.javaat49ae824dThe five runtime-library projects contain regression tests for
exec(""). The replacement changes the exception contract. Payara independently builds commands with adjacent spaces; splitting on one literal space adds an empty argument and changes the launched command.Anything in particular you'd like reviewers to focus on?
Three tests that exist on
mainasserted output in which the command argument had been rewritten to a.split(" ")call. This change alters those expectations: each now asserts that the call is left exactly as written, and each name gained adoNotChangeprefix.stringVariableAsInputis nowdoNotChangeStringVariableAsInput. It dropped three expected calls,runtime.exec(command.split(" "))and itsenvpandenvp, dirvariants. Its input changed too:runtime,command,envpanddirwere locals, includingString command = "ls -al";, and are now parameters, socommandis aStringof unknown value instead of one initialised from a literal. The threeruntime.exec(command)calls in the input are themselves unchanged.methodInvocationAsInputis nowdoNotChangeMethodInvocationAsInput. It dropped the expectedruntime.exec(command().split(" ")). The only input change is thatRuntime runtime = Runtime.getRuntime();became aRuntime runtimeparameter.concatenatedObjectsAsInputis nowdoNotChangeConcatenatedObjectsAsInput. It dropped the expectedruntime.exec(("ls" + " " + options).split(" ")). The only input change is that sameRuntime runtimeparameter.When reading the test diff, note that the first of those renames is not shown as one: the diff replaces
stringVariableAsInputin place with a new test,repeatedDelimitersInRawString, and its scenario reappears further down the file asdoNotChangeStringVariableAsInput.This change adds 6 net test methods to
ReplaceDeprecatedRuntimeExecMethodsTest, taking the focused class from 9 to 15 executions. Counting 3 renamed tests, 9 new or renamed methods cover 12 scenarios. Without the code change in this pull request, these 8 methods fail and represent 11 failing scenarios:tokenizeRawStrings, covering delimiters, quotes, backslashes, control characters, and template placeholdersdoNotChangeCommandWithSupplementaryCharacterEscapedoNotChangeCommandsThatFailAtRuntimedoNotChangeConcatenatedObjectsAsInputdoNotChangeMethodInvocationAsInputdoNotChangeStringVariableAsInputeveryCallOfTheSameOverloadIsReplacedrepeatedDelimitersInRawStringrawStringWithSideEffectingEnvironmentAndDirectorypasses either way.Three limitations are worth knowing:
runtime.exec(LS + " -a")whereLSis astatic final String, is no longer converted at all, becauseLSreaches the recipe as an identifier rather than as a string literal.mainconverts it intoruntime.exec((LS + " -a").split(" ")), and forLS = "ls"that call builds at run time exactly the two argumentsRuntime#exec(String)would have built, somainis not wrong on that value. It is wrong as soon as the concatenated command holds a tab, a repeated separator or a leading separator:runtime.exec(LS + " -a")becomesruntime.exec((LS + " -a").split(" "))onmain, which passes three arguments, the middle one empty. This branch leaves both calls as written.new String[]{...}. Unchanged frommain.main.Have you considered any alternatives or workarounds?
One alternative is to keep converting commands that are not built only from literals by writing
command.split("[ \\t\\n\\r\\f]+")into the source instead of leaving those calls alone. That is a small change, but thesplitcall it would write still differs fromRuntime#exec(String)in two ways: it leaves an empty first element for a command that starts with a separator, such as" ls -a", and it does not reproduce theIllegalArgumentException("Empty command")thatexec("")throws. That is why I skip such calls instead. If you would rather have the recipe keep converting them with that regular expression, say so in review and I will add it.Any additional context
This change was prepared with AI assistance (Claude Code). I reviewed the code, the tests and this description.
I ran the formatter with the repository's
.editorconfig. It also wanted to re-indent lines that this change does not touch, so I left those alone and kept the diff limited to this change.Checklist
./gradlew buildlocally, and committed any resulting changes torecipes.csv