Properly quote arguments passed via argument files #3775
Conversation
|
@lazar-mitrovic Please review! |
|
Hi @jerboaa, thanks for reporting and looking into this bug! It would look something like this: diff --git a/substratevm/src/com.oracle.svm.driver/src/com/oracle/svm/driver/NativeImage.java b/substratevm/src/com.oracle.svm.driver/src/com/oracle/svm/driver/NativeImage.java
index b73b7045d57..f9d00d12f87 100644
--- a/substratevm/src/com.oracle.svm.driver/src/com/oracle/svm/driver/NativeImage.java
+++ b/substratevm/src/com.oracle.svm.driver/src/com/oracle/svm/driver/NativeImage.java
@@ -1424,12 +1424,17 @@ public class NativeImage {
*/
arguments.addAll(Arrays.asList(SubstrateOptions.WATCHPID_PREFIX, "" + ProcessProperties.getProcessID()));
}
+
List<String> finalImageBuilderArgs = createImageBuilderArgs(imageArgs, imagecp, imagemp);
/* Construct ProcessBuilder command from final arguments */
List<String> command = new ArrayList<>();
command.add(canonicalize(config.getJavaExecutable()).toString());
+
List<String> completeCommandList = new ArrayList<>(command);
+ completeCommandList.addAll(Stream.concat(arguments.stream(), finalImageBuilderArgs.stream()).collect(Collectors.toList()));
+
+ arguments = arguments.stream().map(SubstrateUtil::quoteShellArg).collect(Collectors.toList());
if (config.useJavaModules()) { // Only in JDK9+ 'java' executable supports @argFiles.
command.add(createVMInvocationArgumentFile(arguments));
} else {
@@ -1437,7 +1442,6 @@ public class NativeImage {
}
command.add(createImageBuilderArgumentFile(finalImageBuilderArgs));
- completeCommandList.addAll(Stream.concat(arguments.stream(), finalImageBuilderArgs.stream()).collect(Collectors.toList()));
final String commandLine = SubstrateUtil.getShellCommandString(completeCommandList, true);
if (isDiagnostics()) {
// write to the diagnostics dir |
I don't think it's needed. JDK 8 uses the commandline for passing arguments. A |
|
@jerboaa This patch is not enough to fix the JDK 17 Mandrel build on Windows (last known successful build Graal 36cfbfa, JDK17 ff6a9382f5ed. Previous failure ✔️Fixed in this PR. With the patch in this PR ❌Still problematic arguments: |
Oh, my bad! In that case this LGTM! |
|
@lazar-mitrovic Thanks for the review! Could you please help getting this integrated? |
|
I'm in the process of integrating this, but it seems that there are some issues with certain internal tests on Windows. |
Thanks, @lazar-mitrovic. Much appreciated. Please let me know if there is something which needs changing. |
|
@lazar-mitrovic @pejovica Any update on this? Is this good to go or are there updates needed? |
|
Could somebody please help integrate this? It would be a fix needed for the 21.3 release too. Thanks! |
FTR, this failure was unrelated and got fixed with #3793 |
|
Hey @jerboaa, thank you for your contribution! :) It seems like this fix doesn't work on Windows, attempting to build any native images on Windows yields: I suspect the issue here may be because of the argfile rules: backslashes don't need to be escaped if the option they are used in isn't quoted, but if it is, then they need to be escaped. However, it appears other options would cause problems further down the road, like: Would you have time to look into this? |
|
@gradinac OK, good to know. I'll look into it. |
|
I can reproduce this in Mandrel build too, left Windows ❌ , right Linux ✔️
We will amend the PR. |
|
|
jerboaa
force-pushed
the
arguments_files_fix_jdk
branch
2 times, most recently
from
1a92a4f to
f81d522
Compare
|
@gradinac The updated patch should now work on Windows. It's was a bit tricky since |
I suspect a similar issue could happen when using |
|
This is a good point, though I suspect we may still hit issues: backslashes in other native-image options. A user could, for example, specify |
More candidates seem to be: Shall I fix them all, or do you suggest to go a different route? As it stands if any one of these options contained a space it would likely break. It's not really inherent to this patch, but to the argument file change (7c4a040) as a whole. |
|
Fixing all of these options manually, along with the ones we've already changed would work but it would be very brittle in the long run. I'm not aware of any options which would use backslashes, so perhaps another approach we could take is to always escape backslashes in quoted options - this would greatly reduce the overall maintenance in the future. What do you think? |
Sure, lets go with that. I'll point out that it'll not work for certain corner cases. Like user-provided input that contains |
NativeImage passes some arguments via the @file feature of the JDK launcher. When some arguments, separated by new lines, contain whitespace *and* aren't properly quoted, the JDK launcher code fails to parse the argument file and the native image generator class is never launched. Shell-quoting arguments before writing them to an @argument file avoids this issue. Due to a Windows argument file quirk, where '\' don't need to be escaped if NOT quoted, but DO need escaping when they are quoted we unconditionally escape '\'. Closes oracle#3769
jerboaa
force-pushed
the
arguments_files_fix_jdk
branch
from
f81d522 to
d11728e
Compare
|
@gradinac Updated patch. Take 3 :) Thoughts? I can make the |
|
@jerboaa Thank you very much! :) It looks great, I'll start integrating this and also try to backport it to 21.3. For now, I would leave it uniform across all platforms - however, we can revisit it if it ends up being needed. |
Thank you for taking care of integrating it! |
This is now part of 21.3 as well 9579cc9 Thank you @gradinac ! |
7 participants
NativeImage passes some arguments via the @file feature of the
JDK launcher. When some arguments, separated by new lines, contain
whitespace and aren't properly quoted, the JDK launcher code fails
to parse the argument file and the native image generator class is
never launched.
Shell-quoting arguments before writing them to an @argument file
avoids this issue.
Closes #3769