diff --git a/launcher-package/citest/build.sbt b/launcher-package/citest/build.sbt index 9afef5b42..12f5f108b 100644 --- a/launcher-package/citest/build.sbt +++ b/launcher-package/citest/build.sbt @@ -1,5 +1,8 @@ lazy val check = taskKey[Unit]("") lazy val check2 = taskKey[Unit]("") +lazy val checkEvalArgHandling = taskKey[Unit]("") +lazy val checkDArgHandling = taskKey[Unit]("") +lazy val checkXXArgHandling = taskKey[Unit]("") lazy val root = (project in file(".")) .settings( @@ -18,5 +21,47 @@ lazy val root = (project in file(".")) val ys = IO.readLines(file("err.txt")).toVector.distinct assert(ys.isEmpty, s"there's an stderr: $ys") + }, + // Regression check for https://github.com/sbt/sbt/issues/9660, run as a + // real .bat command by test.bat rather than via a JVM-constructed command + // line, so cmd.exe parses the argument the same way it would if a person + // had typed it at a prompt. + checkEvalArgHandling := { + val evalOut = IO.readLines(file("evalOutput.txt")).toVector + println(evalOut) + assert( + evalOut.exists(_.contains("barqux")), + s"""eval ("bar") ++ ("qux") should print barqux, but got: $evalOut""" + ) + assert( + !evalOut.exists(l => + l.contains("was unexpected at this time") || l.contains("is not recognized") + ), + s"eval with quoted parens should not trigger a cmd.exe parse error: $evalOut" + ) + + assert( + !file("injected.txt").exists, + "the & in the eval argument must not escape sbt.bat's quoting and run as a separate command" + ) + }, + // Regression check for https://github.com/sbt/sbt/issues/9660, matching a + // reporter-confirmed case: "sbt" "-Dfoo=()&calc" pops calc when typed at a + // real cmd.exe prompt, even though the whole value sits inside one clean, + // matched pair of quotes (no embedded quote characters to lose parity). + checkDArgHandling := { + assert( + !file("injected2.txt").exists, + "the & in a -D argument value must not escape sbt.bat's quoting and run as a separate command" + ) + }, + // Regression check for https://github.com/sbt/sbt/issues/9660: the -XX + // handling in args_loop has the same shape as -D, so the same reporter- + // confirmed bypass applies to it. + checkXXArgHandling := { + assert( + !file("injected3.txt").exists, + "the & in a -XX argument value must not escape sbt.bat's quoting and run as a separate command" + ) } ) diff --git a/launcher-package/citest/test.bat b/launcher-package/citest/test.bat index c35ddae49..608abff66 100755 --- a/launcher-package/citest/test.bat +++ b/launcher-package/citest/test.bat @@ -28,4 +28,24 @@ SET SBT_OPTS=-Xmx4g -Dfile.encoding=UTF8 "freshly-baked\sbt\bin\sbt" -Dsbt.no.format=true --version > version.txt "freshly-baked\sbt\bin\sbt" -Dsbt.no.format=true checkVersion +rem Regression test for https://github.com/sbt/sbt/issues/9660, run as a real +rem .bat command (parsed by cmd.exe the same way as if typed at a prompt), +rem rather than via a JVM-constructed command line. +"freshly-baked\sbt\bin\sbt" -Dsbt.no.format=true "eval (\"bar\") ++ (\"qux\")" 1> evalOutput.txt 2> evalErr.txt + +"freshly-baked\sbt\bin\sbt" -Dsbt.no.format=true "eval (\"foo\") & echo INJECTED>injected.txt & rem (" 1> evalInjOutput.txt 2> evalInjErr.txt + +"freshly-baked\sbt\bin\sbt" -Dsbt.no.format=true checkEvalArgHandling + +rem "about" comes before the risky argument so sbt.bat always receives a real +rem command and cannot fall into an interactive shell (which would hang this +rem script) even if the & below does escape its quoting. +"freshly-baked\sbt\bin\sbt" -Dsbt.no.format=true about "-Dfoo=()© nul injected2.txt" + +"freshly-baked\sbt\bin\sbt" -Dsbt.no.format=true checkDArgHandling + +"freshly-baked\sbt\bin\sbt" -Dsbt.no.format=true about "-XXbar=()© nul injected3.txt" + +"freshly-baked\sbt\bin\sbt" -Dsbt.no.format=true checkXXArgHandling + ENDLOCAL diff --git a/launcher-package/integration-test/src/test/scala/RunnerMemoryScriptTest.scala b/launcher-package/integration-test/src/test/scala/RunnerMemoryScriptTest.scala index 7f2c3ffd0..1a68cae65 100644 --- a/launcher-package/integration-test/src/test/scala/RunnerMemoryScriptTest.scala +++ b/launcher-package/integration-test/src/test/scala/RunnerMemoryScriptTest.scala @@ -75,12 +75,14 @@ object RunnerMemoryScriptTest extends verify.BasicTestSuite with ShellScriptUtil assert(out.contains[String]("-Xss12m")) // Test for issue #5742: -X options passed directly on command line + // Note: the -v preview quotes these (see #9660), so match by substring rather + // than exact line equality. testOutput("sbt -Xmx1G directly on command line")("-Xmx1G", "-v"): (out: List[String]) => - assert(out.contains[String]("-Xmx1G")) + assert(out.exists(_.contains("-Xmx1G"))) testOutput("sbt -Xms512M -Xmx1G directly on command line")("-Xms512M", "-Xmx1G", "-v"): (out: List[String]) => - assert(out.contains[String]("-Xms512M")) - assert(out.contains[String]("-Xmx1G")) + assert(out.exists(_.contains("-Xms512M"))) + assert(out.exists(_.contains("-Xmx1G"))) end RunnerMemoryScriptTest diff --git a/launcher-package/integration-test/src/test/scala/RunnerScriptTest.scala b/launcher-package/integration-test/src/test/scala/RunnerScriptTest.scala index ad5774e77..ebf8af331 100644 --- a/launcher-package/integration-test/src/test/scala/RunnerScriptTest.scala +++ b/launcher-package/integration-test/src/test/scala/RunnerScriptTest.scala @@ -24,7 +24,8 @@ object RunnerScriptTest extends verify.BasicTestSuite with ShellScriptUtil: assert(out.contains[String]("-Dsbt.log.noformat=true")) testOutput("sbt --color=false")("compile", "--color=false", "-v"): (out: List[String]) => - assert(out.contains[String]("-Dsbt.color=false")) + // Note: the -v preview quotes this (see #9660), so match by substring. + assert(out.exists(_.contains("-Dsbt.color=false"))) testOutput("sbt --no-colors in SBT_OPTS", sbtOpts = "--no-colors")("compile", "-v"): (out: List[String]) => @@ -38,16 +39,21 @@ object RunnerScriptTest extends verify.BasicTestSuite with ShellScriptUtil: assert(out.contains[String]("-Dxsbt.inc.debug=true")) testOutput("sbt --supershell=never")("compile", "--supershell=never", "-v"): - (out: List[String]) => assert(out.contains[String]("-Dsbt.supershell=never")) + (out: List[String]) => + // Note: the -v preview quotes this (see #9660), so match by substring. + assert(out.exists(_.contains("-Dsbt.supershell=never"))) testOutput("sbt --timings")("compile", "--timings", "-v"): (out: List[String]) => assert(out.contains[String]("-Dsbt.task.timings=true")) testOutput("sbt -D arguments")("-Dsbt.supershell=false", "compile", "-v"): (out: List[String]) => - assert(out.contains[String]("-Dsbt.supershell=false")) + // Note: the -v preview quotes CLI -D arguments (see #9660), so match by + // substring rather than exact line equality. + assert(out.exists(_.contains("-Dsbt.supershell=false"))) testOutput("sbt --sbt-version")("--sbt-version", "1.3.13", "-v"): (out: List[String]) => - assert(out.contains[String]("-Dsbt.version=1.3.13")) + // Note: the -v preview quotes this (see #9660), so match by substring. + assert(out.exists(_.contains("-Dsbt.version=1.3.13"))) testOutput( name = "sbt with -Dhttp.proxyHost=proxy -Dhttp.proxyPort=8080 in SBT_OPTS", @@ -365,6 +371,12 @@ object RunnerScriptTest extends verify.BasicTestSuite with ShellScriptUtil: s"Should not have shell expansion errors, but found: ${errorMessages.mkString(", ")}" ) + // NOTE on https://github.com/sbt/sbt/issues/9660: an argument with an + // unquoted &, (, or ) can be split apart by cmd.exe's parse of the command + // line used to *invoke* sbt.bat, before any line of sbt.bat runs -- no code + // inside the script can intervene there. Closing that would require a real + // executable entry point instead of a .bat file (see the sbtw project). + // Test for issue #8755: Inline comments should be supported in .jvmopts testOutput( "sbt with inline comments in .jvmopts", @@ -411,6 +423,8 @@ object RunnerScriptTest extends verify.BasicTestSuite with ShellScriptUtil: ) testOutput("sbt --experimental_execution_log=true")("--experimental_execution_log=true", "-v"): - (out: List[String]) => assert(out.contains[String]("-Dsbt.experimental_execution_log=true")) + (out: List[String]) => + // Note: the -v preview quotes this (see #9660), so match by substring. + assert(out.exists(_.contains("-Dsbt.experimental_execution_log=true"))) end RunnerScriptTest diff --git a/launcher-package/src/universal/bin/sbt.bat b/launcher-package/src/universal/bin/sbt.bat index 9c5fe2252..68f8d26cb 100755 --- a/launcher-package/src/universal/bin/sbt.bat +++ b/launcher-package/src/universal/bin/sbt.bat @@ -537,71 +537,75 @@ if "%~0" == "init" ( ) ) -if "%g:~0,2%" == "-D" ( - rem special handling for -D since '=' gets parsed away - for /F "tokens=1 delims==" %%a in ("%g%") do ( - rem make sure it doesn't have the '=' already - if "%g%" == "%%a" ( - if not "%~1" == "" ( - call :dlog [args_loop] -D argument %~0=%~1 - set "SBT_ARGS=!SBT_ARGS! %~0=%~1" - shift - goto args_loop - ) else ( - echo %g% is missing a value - goto error - ) - ) else ( - call :dlog [args_loop] -D argument %~0 - set "SBT_ARGS=!SBT_ARGS! %~0" - goto args_loop - ) - ) -) +rem NOTE: -D handling below is intentionally flat (goto-based, single-line +rem `for` bodies) rather than nested if/for blocks. cmd.exe finds a multi-line +rem block's closing paren by counting every literal ( and ) across the lines +rem it spans, including ones arriving via variable substitution -- splicing an +rem argument value (which may itself contain unbalanced-looking parens, see +rem #9660) into a deeply nested multi-line construct risks miscounting that. +rem A flat, single-line-per-command shape does not put cmd.exe in a position +rem where it needs to hunt across multiple lines for a matching paren. +if not "%g:~0,2%" == "-D" goto args_loop_after_D +rem special handling for -D since '=' gets parsed away +set "g_key=" +for /F "tokens=1 delims==" %%a in ("%g%") do set "g_key=%%a" +if not "%g%" == "%g_key%" goto args_loop_D_has_value +if "%~1" == "" goto args_loop_D_missing_value +call :dlog [args_loop] -D argument %~0=%~1 +set SBT_ARGS=!SBT_ARGS! "%~0=%~1" +shift +goto args_loop +:args_loop_D_missing_value +echo %g% is missing a value +goto error +:args_loop_D_has_value +call :dlog [args_loop] -D argument %~0 +set SBT_ARGS=!SBT_ARGS! "%~0" +goto args_loop +:args_loop_after_D -if not "%g:~0,5%" == "-XX:+" if not "%g:~0,5%" == "-XX:-" if "%g:~0,3%" == "-XX" ( - rem special handling for -XX since '=' gets parsed away - for /F "tokens=1 delims==" %%a in ("%g%") do ( - rem make sure it doesn't have the '=' already - if "%g%" == "%%a" ( - if not "%~1" == "" ( - call :dlog [args_loop] -XX argument %~0=%~1 - set "SBT_ARGS=!SBT_ARGS! %~0=%~1" - shift - goto args_loop - ) else ( - echo %g% is missing a value - goto error - ) - ) else ( - call :dlog [args_loop] -XX argument %~0 - set "SBT_ARGS=!SBT_ARGS! %~0" - goto args_loop - ) - ) -) +rem See the -D handling above for why this is flat rather than nested blocks. +if "%g:~0,5%" == "-XX:+" goto args_loop_after_XX +if "%g:~0,5%" == "-XX:-" goto args_loop_after_XX +if not "%g:~0,3%" == "-XX" goto args_loop_after_XX +rem special handling for -XX since '=' gets parsed away +set "g_key=" +for /F "tokens=1 delims==" %%a in ("%g%") do set "g_key=%%a" +if not "%g%" == "%g_key%" goto args_loop_XX_has_value +if "%~1" == "" goto args_loop_XX_missing_value +call :dlog [args_loop] -XX argument %~0=%~1 +set SBT_ARGS=!SBT_ARGS! "%~0=%~1" +shift +goto args_loop +:args_loop_XX_missing_value +echo %g% is missing a value +goto error +:args_loop_XX_has_value +call :dlog [args_loop] -XX argument %~0 +set SBT_ARGS=!SBT_ARGS! "%~0" +goto args_loop +:args_loop_after_XX rem handle -X JVM options (e.g., -Xmx1G, -Xms512M, -Xss4M) - fixes #5742 if "%g:~0,2%" == "-X" ( call :dlog [args_loop] -X JVM argument %~0 - call :addJava %~0 + call :addJava "%~0" goto args_loop ) -if defined sbt_new if "%g:~0,2%" == "--" ( - rem special handling for -- template arguments since '=' gets parsed away on Windows - for /F "tokens=1 delims==" %%a in ("%g%") do ( - rem make sure it doesn't have the '=' already - if "%g%" == "%%a" ( - if not "%~1" == "" ( - call :dlog [args_loop] -- argument %~0=%~1 - set "SBT_ARGS=!SBT_ARGS! %~0=%~1" - shift - goto args_loop - ) - ) - ) -) +rem See the -D handling above for why this is flat rather than nested blocks. +if not defined sbt_new goto args_loop_after_dashdash +if not "%g:~0,2%" == "--" goto args_loop_after_dashdash +rem special handling for -- template arguments since '=' gets parsed away on Windows +set "g_key=" +for /F "tokens=1 delims==" %%a in ("%g%") do set "g_key=%%a" +if not "%g%" == "%g_key%" goto args_loop_after_dashdash +if "%~1" == "" goto args_loop_after_dashdash +call :dlog [args_loop] -- argument %~0=%~1 +set SBT_ARGS=!SBT_ARGS! "%~0=%~1" +shift +goto args_loop +:args_loop_after_dashdash rem the %0 (instead of %~0) preserves original argument quoting set sbt_args_seen_command=1 @@ -655,7 +659,11 @@ if !sbt_args_print_sbt_script_version! equ 1 ( call :checkjava if !run_native_client! equ 1 if not defined sbt_args_print_version ( - goto :runnative !SBT_ARGS! + rem Do not append !SBT_ARGS! here: `goto` ignores it, it is not a real + rem argument-passing mechanism, but the text is still spliced into this + rem line and re-scanned by cmd.exe, so it is live to shell operators ^(#9660^). + rem :runnative reads SBT_ARGS via delayed expansion instead. + goto :runnative goto :eof ) @@ -675,7 +683,11 @@ if defined JVM_DEBUG_PORT ( call :sync_preloaded -call :run !SBT_ARGS! +rem Do not append !SBT_ARGS! to this call: splicing it onto a `call` command +rem line makes cmd.exe re-tokenize it, letting shell operators inside a +rem quoted user argument escape their quoting and run ^(#9660^). +rem :run reads SBT_ARGS via delayed expansion instead. +call :run if ERRORLEVEL 1 goto error goto end @@ -700,34 +712,38 @@ if defined sbt_args_no_share ( set _SBT_OPTS=-Dsbt.global.base=project/.sbtboot -Dsbt.boot.directory=project/.boot -Dsbt.ivy.home=project/.ivy !_SBT_OPTS! ) +rem NOTE: each user-supplied value below is wrapped in its own quotes before +rem being appended to _SBT_OPTS. _SBT_OPTS is later spliced unquoted into +rem exec lines ^(:copyrt, the final java invocation^), so an unquoted shell +rem operator here would be live to cmd.exe at that point ^(#9660^). if defined sbt_args_supershell ( - set _SBT_OPTS=-Dsbt.supershell=!sbt_args_supershell! !_SBT_OPTS! + set _SBT_OPTS="-Dsbt.supershell=!sbt_args_supershell!" !_SBT_OPTS! ) if defined sbt_args_sbt_version ( - set _SBT_OPTS=-Dsbt.version=!sbt_args_sbt_version! !_SBT_OPTS! + set _SBT_OPTS="-Dsbt.version=!sbt_args_sbt_version!" !_SBT_OPTS! ) if defined sbt_args_sbt_dir ( - set _SBT_OPTS=-Dsbt.global.base=!sbt_args_sbt_dir! !_SBT_OPTS! + set _SBT_OPTS="-Dsbt.global.base=!sbt_args_sbt_dir!" !_SBT_OPTS! ) else if defined LOCALAPPDATA ( - set _SBT_OPTS=-Dsbt.global.base=!LOCALAPPDATA!\sbt !_SBT_OPTS! + set _SBT_OPTS="-Dsbt.global.base=!LOCALAPPDATA!\sbt" !_SBT_OPTS! ) if defined sbt_args_sbt_boot ( - set _SBT_OPTS=-Dsbt.boot.directory=!sbt_args_sbt_boot! !_SBT_OPTS! + set _SBT_OPTS="-Dsbt.boot.directory=!sbt_args_sbt_boot!" !_SBT_OPTS! ) if defined sbt_args_sbt_cache ( - set _SBT_OPTS=-Dsbt.global.localcache=!sbt_args_sbt_cache! !_SBT_OPTS! + set _SBT_OPTS="-Dsbt.global.localcache=!sbt_args_sbt_cache!" !_SBT_OPTS! ) if defined sbt_args_ivy ( - set _SBT_OPTS=-Dsbt.ivy.home=!sbt_args_ivy! !_SBT_OPTS! + set _SBT_OPTS="-Dsbt.ivy.home=!sbt_args_ivy!" !_SBT_OPTS! ) if defined sbt_args_color ( - set _SBT_OPTS=-Dsbt.color=!sbt_args_color! !_SBT_OPTS! + set _SBT_OPTS="-Dsbt.color=!sbt_args_color!" !_SBT_OPTS! ) if defined sbt_args_mem ( @@ -755,7 +771,7 @@ if not defined sbt_args_no_hide_jdk_warnings ( ) if defined sbt_args_experimental_execution_log ( - set _SBT_OPTS=-Dsbt.experimental_execution_log=!sbt_args_experimental_execution_log! !_SBT_OPTS! + set _SBT_OPTS="-Dsbt.experimental_execution_log=!sbt_args_experimental_execution_log!" !_SBT_OPTS! ) rem TODO: _SBT_OPTS needs to be processed as args and diffed against SBT_ARGS @@ -792,11 +808,14 @@ if defined sbt_args_verbose ( echo -cp echo "!sbt_jar!" echo xsbt.boot.Boot - if not "%~1" == "" ( call :echolist %* ) + if defined SBT_ARGS ( call :echolist !SBT_ARGS! ) echo. ) -"!_JAVACMD!" !_JAVA_OPTS! !_SBT_OPTS! %JAVA_TOOL_OPTIONS% %JDK_JAVA_OPTIONS% -cp "!sbt_jar!" xsbt.boot.Boot %* +rem one-hop marker: drop it before the server JVM +set "SBT_EXPLICIT_JAVA_HOME=" + +"!_JAVACMD!" !_JAVA_OPTS! !_SBT_OPTS! %JAVA_TOOL_OPTIONS% %JDK_JAVA_OPTIONS% -cp "!sbt_jar!" xsbt.boot.Boot !SBT_ARGS! goto :eof @@ -804,14 +823,19 @@ goto :eof set "_SBTNCMD=!SBT_BIN_DIR!sbtn-x86_64-pc-win32.exe" +rem NOTE: SBT_ARGS entries may carry their own embedded quotes, see the +rem args_loop -D/-XX/catch-all handling above, to keep shell operators safe +rem across the re-scan below. Do not wrap these set lines in an outer pair +rem of quotes: that would pair up with an embedded quote instead of its +rem match, breaking the quoting it is meant to preserve ^(#9660^). if defined sbt_args_verbose ( echo # running native client if not "%~1" == "" ( call :echolist %* ) - set "SBT_ARGS=-v !SBT_ARGS!" + set SBT_ARGS=-v !SBT_ARGS! ) for %%I in ("!SBT_BIN_DIR!sbt.bat") do set "SBT_SCRIPT=%%~sI" -set "SBT_ARGS=--sbt-script=!SBT_SCRIPT! %SBT_ARGS%" +set SBT_ARGS=--sbt-script=!SBT_SCRIPT! %SBT_ARGS% rem Microsoft Visual C++ 2010 SP1 Redistributable Package (x64) is required rem https://www.microsoft.com/en-us/download/details.aspx?id=13523 @@ -885,7 +909,7 @@ exit /B 0 :addJava call :dlog [addJava] arg = '%*' - set "_JAVA_OPTS=!_JAVA_OPTS! %*" + set _JAVA_OPTS=!_JAVA_OPTS! %* exit /B 0 :addMemory