gnodet-bot commented on code in PR #13652:
URL: https://github.com/apache/maven/pull/13652#discussion_r4347456660
##########
apache-maven/src/assembly/maven/bin/mvn.cmd:
##########
@@ -336,17 +336,34 @@ if exist "%JVM_CONFIG_TEMP%" (
echo [DEBUG] Temp file contents:
type "%JVM_CONFIG_TEMP%"
)
+ setlocal EnableDelayedExpansion
set /p JVM_CONFIG_MAVEN_OPTS=<"%JVM_CONFIG_TEMP%" 2>nul
rem Retry once after a brief delay if the read returned empty (Windows
Defender file lock).
rem Note: use string comparison instead of 'if not defined' because 'set
JVM_CONFIG_MAVEN_OPTS='
rem (empty assignment) counts as defined, so 'if not defined' would never
trigger the retry.
- if "%JVM_CONFIG_MAVEN_OPTS%"=="" (
+ rem Note: use delayed expansion (!VAR! instead of %VAR%) because cmd.exe
expands %VAR% at
+ rem parse time for the entire compound block, before 'set /p' above has run
at runtime.
+ if "!JVM_CONFIG_MAVEN_OPTS!"=="" (
if defined MAVEN_DEBUG_SCRIPT (
echo [DEBUG] First read returned empty, retrying after delay...
)
ping -n 2 127.0.0.1 >nul 2>nul
set /p JVM_CONFIG_MAVEN_OPTS=<"%JVM_CONFIG_TEMP%" 2>nul
)
+ rem Export JVM_CONFIG_MAVEN_OPTS from the setlocal scope using FOR /F.
+ rem The 'endlocal & set' idiom cannot use !VAR! after endlocal because
delayed
+ rem expansion is disabled in the outer scope (@setlocal at line 53 has no
+ rem EnableDelayedExpansion). FOR /F captures the value while still inside the
+ rem delayed-expansion scope, then endlocal inside the loop body restores the
+ rem outer scope — at which point %%V is already bound to the correct value.
+ rem When the value is empty (both reads failed), FOR /F skips the empty
token,
+ rem so we use a sentinel prefix trick: prefix with '#', strip it after
export.
+ for /f "delims=" %%V in ("#!JVM_CONFIG_MAVEN_OPTS!") do (
+ endlocal
+ set "JVM_CONFIG_MAVEN_OPTS=%%V"
+ )
+ rem Strip the '#' sentinel prefix added above (works in both empty and
non-empty cases)
+ set "JVM_CONFIG_MAVEN_OPTS=%JVM_CONFIG_MAVEN_OPTS:~1%"
Review Comment:
⚠️ **`FOR /F` literal-string quoting breaks when the value contains `"` —
which it always does.**
`JvmConfigParser.appendQuotedArguments` wraps every argument in
double-quotes before writing to the temp file. For `-Xmx512m`, the file
contains `"-Xmx512m"` (with literal `"` characters). After `set /p`,
`JVM_CONFIG_MAVEN_OPTS` = `"-Xmx512m"`.
After delayed expansion, the `FOR /F` line becomes:
```
for /f "delims=" %%V in ("#"-Xmx512m"") do (
```
The `in ("...")` literal-string form uses the outermost `"` as delimiters.
With embedded `"` in the expanded value, cmd.exe sees `"#"` as the complete
string and treats `-Xmx512m""` as garbage outside the `in (...)` clause. Only
`#` is captured as `%%V` — after the sentinel strip, `JVM_CONFIG_MAVEN_OPTS` is
empty. JVM opts are silently dropped on every non-empty jvm.config.
The `mng-4559-spaces-jvm-opts` test uses `-Dprop.jvm-opts="foo bar"` in
jvm.config; `JvmConfigParser` produces `"-Dprop.jvm-opts=foo bar"` in the temp
file — this test will fail on Windows.
**Fix:** use the dbenham double-double-quote idiom with `%%~V` to strip the
outer quotes — this safely handles values containing embedded `"` and also
correctly exports an empty value (no sentinel needed):
```suggestion
rem so we use the dbenham double-double-quote export idiom: wrap in
""..."" and
rem use %%~V to strip the outer quotes. This handles values with embedded
" safely.
for /f "delims=" %%V in (""!JVM_CONFIG_MAVEN_OPTS!"") do (
endlocal
set "JVM_CONFIG_MAVEN_OPTS=%%~V"
)
```
--
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]
