Thanks for the fix. I had looked into it, it’s a nice catch, glad you find it.
The patch looks good, and test is sufficient, I have tested it with jdk.jdk repo. I will help you to push the fix. The only minor improvement I like to see is to verify content after the escape character, which I took the liberty to do it. Webrev coming soon. Cheers, Henry > On Sep 26, 2018, at 4:03 PM, Bo Zhang <zhangbo...@gmail.com> wrote: > > Can anyone please take a look? > > Thank you. > >> On 25 Sep 2018, at 14:27, Bo Zhang <zhangbo...@gmail.com> wrote: >> >> Hi all, >> >> This is my first time to try to submit a patch. Please forgive me if I did >> anything inappropriate. I’ve signed the OCA with the name “Bo Zhang”. >> >> Bug description: >> >> 8210810 <https://bugs.openjdk.java.net/browse/JDK-8210810>: Incorrect >> argument file parser behavior in certain cases >> >> Previously, if a quoted backslash character appears at argument file >> parser buffer boundary (i.e. 4096th/8192nd/... character of an argument >> file is backslash '\'), the segment before the backslash character will >> be added into argument twice due to a missing anchor position reset >> operation. >> >> For example, you have an argument file: >> >> -cp "...a\\b" >> >> and the 4096th and 4097th characters happen to be both '\', the result >> generated by the argument file parser would be: >> >> -cp "...aa\\b” >> >> This patch fixes the issue by resetting anchor position correctly >> in this case. >> >> I’m a little surprised that this issue has existed for several years >> unnoticed. >> >> >> Patch: >> >> # HG changeset patch >> # User Bo Zhang <zhangbo...@gmail.com <mailto:zhangbo...@gmail.com>> >> # Date 1537835887 -28800 >> # Tue Sep 25 08:38:07 2018 +0800 >> # Node ID 34f2803ad62cde8c82e3dfcf0b66e80276bd7352 >> # Parent 16f0deae8fa6ef85f7230308e1fe7556bd007c39 >> 8210810: Incorrect argument file parser behavior in certain cases >> >> Previously, if a quoted backslash character appears at argument file >> parser buffer boundary (i.e. 4096th/8192nd/... character of an argument >> file is backslash '\'), the segment before the backslash character will >> be added into argument twice due to a missing anchor position reset >> operation. >> >> For example, you have an argument file: >> >> -cp "...a\\b" >> >> and the 4096th and 4097th characters happen to be both '\', the result >> generated by the argument file parser would be: >> >> -cp "...aa\\b" >> >> This patch fixes the issue by resetting anchor position correctly >> in this case. >> >> diff --git a/src/java.base/share/native/libjli/args.c >> b/src/java.base/share/native/libjli/args.c >> --- a/src/java.base/share/native/libjli/args.c >> +++ b/src/java.base/share/native/libjli/args.c >> @@ -262,6 +262,8 @@ >> continue; >> } >> JLI_List_addSubstring(pctx->parts, anchor, nextc - anchor); >> + // anchor after backslash character >> + anchor = nextc + 1; >> pctx->state = IN_ESCAPE; >> break; >> case '\'': >> diff --git a/test/jdk/tools/launcher/Test8210810.java >> b/test/jdk/tools/launcher/Test8210810.java >> new file mode 100644 >> --- /dev/null >> +++ b/test/jdk/tools/launcher/Test8210810.java >> @@ -0,0 +1,107 @@ >> +/* >> + * Copyright (c) 2018, Oracle and/or its affiliates. All rights reserved. >> + * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. >> + * >> + * This code is free software; you can redistribute it and/or modify it >> + * under the terms of the GNU General Public License version 2 only, as >> + * published by the Free Software Foundation. >> + * >> + * This code is distributed in the hope that it will be useful, but WITHOUT >> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or >> + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License >> + * version 2 for more details (a copy is included in the LICENSE file that >> + * accompanied this code). >> + * >> + * You should have received a copy of the GNU General Public License version >> + * 2 along with this work; if not, write to the Free Software Foundation, >> + * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA. >> + * >> + * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA >> + * or visit www.oracle.com <http://www.oracle.com/> if you need additional >> information or have any >> + * questions. >> + */ >> + >> +/* >> + * @test >> + * @bug 8210810 >> + * @summary Incorrect argument file parser behavior when backslash appears >> at buffer boundary >> + * @modules java.base >> + * @author Bo Zhang >> + * @build TestHelper >> + * @run main Test8210810 >> + */ >> + >> +/* >> + * 8210810: Previously, if a quoted backslash character appears at argument >> file >> + * parser buffer boundary (i.e. 4096th/8192nd/... character of an argument >> + * file is backslash '\'), the segment before the backslash character will >> + * be added into argument twice due to a missing anchor position reset >> + * operation. This test creates a special argument file which can reproduce >> this issue. >> + */ >> +import java.io.File; >> +import java.nio.file.Files; >> +import java.io.IOException; >> +import java.util.Arrays; >> + >> +public class Test8210810 extends TestHelper { >> + // Buffer size in args.c readArgFile() method >> + private static final int ARG_FILE_PARSER_BUF_SIZE = 4096; >> + public static void main(String... args) throws Exception { >> + new Test8210810().canParseArgFileCorrectly(); >> + } >> + >> + private void canParseArgFileCorrectly() throws IOException { >> + File testJar = createTestClassWhichPrintsClasspath(); >> + File argFile = createArgFileWithSpecificClasspath(); >> + TestResult result = doExec(javaCmd, "@argFile", "Foo"); >> + >> + long zCountInArgFile = countZ(new >> String(Files.readAllBytes(argFile.toPath()))); >> + long zCountReadByParser = countZ(result.testOutput.get(0)); >> + if(zCountInArgFile != zCountReadByParser) { >> + throw new RuntimeException("Classpath parsed by parser is >> different from argument file!"); >> + } >> + >> + testJar.delete(); >> + argFile.delete(); >> + } >> + >> + private long countZ(String str) { >> + return str.chars().filter(ch -> ch == 'z').count(); >> + } >> + >> + private File createTestClassWhichPrintsClasspath() throws IOException { >> + File testJar = new File("test.jar"); >> + testJar.delete(); >> + >> + StringBuilder tsrc = new StringBuilder(); >> + tsrc.append("public static void main(String... args) {\n"); >> + tsrc.append(" System.out.println(\"Classpath:\" + >> System.getProperty(\"java.class.path\"));\n"); >> + tsrc.append("}\n"); >> + createJar(testJar, new File("Foo"), tsrc.toString()); >> + return testJar; >> + } >> + >> + // Create a file whose 4096th and 4097th characters are both '\' >> (backslash) >> + // File content: >> + // |-cp "test.jar:zzzzz...zzz\\"| >> + private File createArgFileWithSpecificClasspath() throws IOException { >> + File argFile = new File("argFile"); >> + argFile.delete(); >> + >> + StringBuilder arg = new StringBuilder("-cp \"test.jar"); >> + arg.append(File.pathSeparator); >> + >> + int fillingCharactersCount = ARG_FILE_PARSER_BUF_SIZE - 1 - >> arg.length(); >> + >> + for(int i = 0; i < fillingCharactersCount; ++i) { >> + arg.append('z'); >> + } >> + >> + arg.append('\\'); // 4096th character in the arg file >> + arg.append('\\'); // 4097th character in the arg file >> + arg.append('"'); >> + >> + createAFile(argFile, Arrays.asList(arg.toString())); >> + return argFile; >> + } >> +} >> >> Unit test: >> >> Included in the patch. It creates a special argument file whose 4096th and >> 4097th characters are both ‘\’. Then it compares the classpath read in the >> VM and the classpath in the argument file to verify they’re identical. >> >> <8210810.patch> >> >
