Hi,
Sorry for this late reply!
I only focus on the changes on the tests in
sun/security/tools/jarsigner/compatibility.
Compatibility.java has been modified significantly, I just had a quick
look at it.
sun/security/tools/jarsigner/compatibility/SignTwice.java
--------------------
With my testing, this test usually got timeout in our CI testing system.
The timeout is 480 seconds.
sun/security/tools/jarsigner/compatibility/Compatibility.java
--------------------
38 import java.io.ByteArrayOutputStream;
44 import java.io.FilterOutputStream;
65 import java.util.jar.Attributes;
The above import statements are unnecessary.
82 private static JdkInfo TEST_JDK_INFO;
100 private static String[] KEY_ALGORITHMS;
111 private static String[] DIGEST_ALGORITHMS;
Now that these variables are not constant anymore, the names would be
testJdkInfo, keyAlgorithms and digestAlgorithms.
161 System.out.println("using sigfile " + sigfileName + " for
alias "
162 + alias + " signing " + u + ".jar to " + s +
".jar");
Could it be better to use System.out.printf()?
409 private static String outfile() {
410 return System.getProperty("o");
411 }
Would it be better to define a constant for outfile?
416 String[] jdkList = list("jdkList");
417 if (jdkList.length == 0) {
418 jdkList = new String[] { "TEST_JDK" };
419 }
420
421 List<JdkInfo> jdkInfoList = new ArrayList<>();
422 for (String jdkPath : jdkList) {
423 JdkInfo jdkInfo = "TEST_JDK".equalsIgnoreCase(jdkPath) ?
424 TEST_JDK_INFO : new JdkInfo(jdkPath);
Not sure understand this update.
Now that the constant TEST_JDK is the current testing JDK path, then new
JdkInfo(TEST_JDK) should be equivalent to TEST_JDK_INFO in this update.
435 private static List<String> keyAlgs() throws IOException {
436 if (KEY_ALGORITHMS == null) KEY_ALGORITHMS = list("keyAlgs");
437 if (KEY_ALGORITHMS.length == 0)
438 return Arrays.asList(DEFAULT_KEY_ALGORITHMS);
439 return Arrays.stream(KEY_ALGORITHMS).map(a -> a.split(";")[0])
440 .collect(Collectors.toList());
441 }
...
467 private static List<String> digestAlgs() throws IOException {
468 if (DIGEST_ALGORITHMS == null) DIGEST_ALGORITHMS =
list("digestAlgs");
469 if (DIGEST_ALGORITHMS.length == 0)
470 return Arrays.asList(DEFAULT_DIGEST_ALGORITHMS);
471 return Arrays.asList(DIGEST_ALGORITHMS);
472 }
It looks better that the above methods return String array rather than
list. Then, the callings on Arrays.asList() are unnecessary.
793 System.out.println("verifyingStatus: error: exit code !=
" + expectedExitCode + ": " + outputAnalyzer.getExitValue() + " != " +
expectedExitCode);
...
799 System.out.println("verifyingStatus: error:
expectedSuccessMessage not found: " + expectedSuccessMessage);
...
810 System.out.println("verifyingStatus: error:
line.matches(" + Test.ERROR + "\" ?\"): " + line);
The above lines are too long.
sun/security/tools/jarsigner/compatibility/JdkUtils.java
--------------------
The year should be updated in the copyright notes.
Best regards,
John Jiang
On 2019/6/21 11:34, Weijun Wang wrote:
BTW, I haven't looked at the changes in
test/jdk/sun/security/tools/jarsigner/compatibility yet.
John, do you have any comment in this area?
Thanks,
Max
On Jun 21, 2019, at 11:32 AM, Weijun Wang <weijun.w...@oracle.com> wrote:
The tests.
- For all:
We are thinking about removing default value for -keyalg for "keytool -genkeypair" some day, so it
will be better to add an explicit "-keyalg RSA" or "-keyalg EC" now.
There are several tests printing out the manifest in a sequence of "[[0]=83='S', ...". While it is precise, I
find it not very easy to check. How about we just print out the string format but for each "\r" we just print
"\r" literally and keep printing "\n" with a new line, like this:
Signature-Version: 1.0\r
Created-By: 13-internal (Oracle Corporation)\r
SHA-256-Digest-Manifest: Pxklci7ELW1zzSh2jZ+oz7TPR0WK2uTsAIiHar1m6Eo=\r
SHA-256-Digest-Manifest-Main-Attributes: ETfvdTNx3yN/ClSZz20wR0SM9ta8H7O\r
E7U0F4uZ9JV8=\r
\r
Name: abc\r
SHA-256-Digest: uTZ8UM3iJLPtl8W7+NEC/AC2auI7LP2Gu53ajj6jLgg=\r
\r
If you find running jarsigner slow, is the JarSigner::sign API any better? The
verify side can also usually be replaced with open the file and read everything
inside unless you want to grab certain outputs, but those can also be done with
JarEntry::getSigners.
- DigestDontIgnoreCase.java:
getManifestBytes(). This could reformat the manifest (I understand it's
irrelevant in this test), you can just return
jf.getInputStream(jf.getJarEntry(JarFile.MANIFEST_NAME)).readAllBytes()
- EmptyIndividualSectionName.java:
Are you going to do something around testNameEmptyTrusted()?
- FindHeaderEndVsManifestDigesterFindFirstSection.java:
Several TODO and FIXME.
52: typo: backwars-
76: No need to maintain FIXED_8217375 anymore
229: Typo: ManifestSigester
- InsufficientSectionDelimiter.java:
We usually abbreviate "attribute" to "attr" instead of "att".
We usually use Path.of() instead of Paths.get() these days.
- MainAttributesConfused.java:
Have you tried adding a file named "Manifest-Main-Attributes" into a jar and
see if it would mess up anything?
- PreserveRawManifestEntryAndDigest.java:
This test is running for a long time. The output is also quite big and you can see
"Output overflow" inside which means if there is a failure it might not show.
Is it possible to simplify it?
getExpectedJarSignerOutputUpdatedContentNotValidatedBySignerA(). The output
might change often. If you only focus on several lines, it's better to check
them with OutputAnalyzer::shouldMatch***. We can always manually construct an
OutputAnalyzer object.
- DigestInput.java and FindSection.java:
static final boolean FIXED_8217375 = true;
Is this still needed?
- OutputAnalyzer.java:
This is a shared utility class and you changed the behavior. It might be
correct (start matching *after* the from line) but I cannot be sure if it would
break anything. I'll ask people about it.
Thanks,
Max
On Jun 11, 2019, at 3:08 PM, Weijun Wang <weijun.w...@oracle.com> wrote:
Hi Philipp,
I'll start reviewing this code change. Since this is a P3 bug fix, we still
have a month's time (before RDP 2 starts on 7/18) to work on it.
Also, I've included John as a reviewer. He is the author of the
Compatibility.java test.
Thanks,
Max
On May 23, 2019, at 9:49 AM, Weijun Wang <weijun.w...@oracle.com> wrote:
Hi Philipp,
I've just uploaded your patch to
http://cr.openjdk.java.net/~weijun/8217375/webrev.01/
