gnodet-bot commented on code in PR #13652:
URL: https://github.com/apache/maven/pull/13652#discussion_r4335669782
##########
apache-maven/src/assembly/maven/bin/JvmConfigParser.java:
##########
@@ -128,14 +129,40 @@ private static String processLine(String line, String
mavenProjectBasedir) {
}
/**
- * Append parsed arguments as quoted strings to the result builder.
+ * Append parsed arguments in Java @argfile format: one argument per line.
+ * Arguments are quoted when they contain whitespace, double-quotes,
backslashes,
+ * or any shell metacharacter that {@code eval} would re-interpret on Unix
+ * (pipe, ampersand, semicolon, redirection, backtick).
+ * Quoting is always valid in {@code @argfile} format, so this is safe for
+ * both the Windows {@code @argfile} consumer and the Unix {@code eval}
consumer.
Review Comment:
The Javadoc says "the Unix `{@code eval}` consumer" — but Unix no longer
uses `eval` to process the argfile content. The `@argfile` is now passed
directly to the JVM as `@"$JVM_CONFIG_ARGFILE"`; the JVM's own @argfile parser
reads it. Shell metacharacters (`|`, `&`, `;`, `<`, `>`, `` ` ``) have no
special meaning in Java's @argfile format, so the shell-metachar quoting
rationale is technically no longer needed (though harmless).
```suggestion
* Append parsed arguments in Java @argfile format: one argument per
line.
* Arguments are quoted when they contain whitespace, double-quotes, or
backslashes.
* Quoting is always valid in {@code @argfile} format and ensures safe
parsing
* by both the Windows and Unix JVM @argfile consumers.
```
##########
apache-maven/src/assembly/maven/bin/mvn:
##########
@@ -473,17 +474,16 @@ if $cygwin || $mingw ; then
MAVEN_PROJECTBASEDIR_NATIVE=`cygpath --windows "$MAVEN_PROJECTBASEDIR"`
fi
-# Read JVM config and append to MAVEN_OPTS, preserving special characters
-_jvm_config="`concat_lines "$MAVEN_PROJECTBASEDIR/.mvn/jvm.config"`"
-if [ -n "$_jvm_config" ]; then
- if [ -n "$MAVEN_OPTS" ]; then
- MAVEN_OPTS="$MAVEN_OPTS $_jvm_config"
- else
- MAVEN_OPTS="$_jvm_config"
- fi
-fi
+# Parse jvm.config into a temp argfile (avoids newline-in-MAVEN_OPTS
regression).
+# The temp file path is stored in JVM_CONFIG_ARGFILE; if no jvm.config exists,
+# JvmConfigParser creates an empty file and the @argfile is silently ignored.
+JVM_CONFIG_ARGFILE=$(mktemp "${TMPDIR:-/tmp}/mvn-jvm-config.XXXXXX"
2>/dev/null || echo "${TMPDIR:-/tmp}/mvn-jvm-config.$$")
+prepare_jvm_config_argfile "$MAVEN_PROJECTBASEDIR/.mvn/jvm.config"
"$JVM_CONFIG_ARGFILE"
Review Comment:
`mktemp` runs unconditionally here — even when `.mvn/jvm.config` does not
exist. Since the script ends with `eval exec` (replacing the shell process), no
EXIT trap fires and the file is never cleaned up. Every `mvn` invocation leaks
a temp file.
The comment on the line above is also incorrect: it says "JvmConfigParser
creates an empty file", but JvmConfigParser is not called at all when no
jvm.config exists — `mktemp` is what creates the file.
```suggestion
JVM_CONFIG_ARGFILE=
if [ -f "$MAVEN_PROJECTBASEDIR/.mvn/jvm.config" ]; then
JVM_CONFIG_ARGFILE=$(mktemp "${TMPDIR:-/tmp}/mvn-jvm-config.XXXXXX"
2>/dev/null || echo "${TMPDIR:-/tmp}/mvn-jvm-config.$$")
prepare_jvm_config_argfile "$MAVEN_PROJECTBASEDIR/.mvn/jvm.config"
"$JVM_CONFIG_ARGFILE"
fi
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
