[13] RFR 8228456: Enhance tests after JDK-8217375

[Philipp Kunz]

Hi Max,
jarsignerProc is definitively a spot where significant amount of time
is spent in the test unlike what is before and what is after it. Have
you measured how much your change would make the test faster? My bet is
it would not be significant. Tell me if I'm wrong.As I see it, we are,
at least to some extent, back to a discussion we already started.
> I also found running jarsigner slow which is why I suggested adding
it as a tool like jar and others in a different thread and which was
not accepted then. As for PreserveRawManifestEntryAndDigest, there is
use of sun.security.tools.jarsigner.Main.main or run.> Using
sun.security.tools.jarsigner.Main re-uses loading of the keystore while
avoiding another JVM which I believe to be almost perfect apart from
the API used not looking very public. Also in terms of performance I
think it should be equivalent to JarSigner::sign.> When the output
should be caputered, there is no way without another JVM (without the
jarsigner tool).
https://mail.openjdk.java.net/pipermail/security-dev/2019-June/020398.h
tml
In the current discussion's context, I'd like to precise: If jarsigner
exits with a non-zero exit code, it calls System.exit which is not
suitable at all for a test. It's actually more about the call to
System.exit that requires it to be run in a child process and not so
much the desire to analyze the output.
I remember having measured each check/assertion's impact on performance
individually and only jarsignerProc is significant. All other checks
and jarsigner operations only add comparatively minor performance
impact not worth in my opinion to optimize in a test.
On the other hand, I admit that there might be too much of checking the
jars and its signers in PreserveRawManifestEntryAndDigest from a code-
style, readability and maintainability point of view.
We might consider removing the parallel=true or add a threadPoolSize
parameter to it with a suitable value that fits the test machines.
Regards,Philipp


On Thu, 2019-07-25 at 17:38 +0800, Weijun Wang wrote:
> Hi Philipp,
> 
> Most are fine but PreserveRawManifestEntryAndDigest.java is still
> very slow and fails intermittently.
> The problem is due to jarsignerProc(). Since you have parallel=true
> for the test providers, many jarsigner processes would run at the
> same time and some of our test machines cannot load so many processes
> (they are also running other jobs).
> 
> I read about the place when jarsignerProc() is called and seems you
> want to make sure the first file is signed by A and the second is
> not. Can we do this using JarFile API? Does you have other things to
> check
> in getExpectedJarSignerOutputUpdatedContentNotValidatedBySignerA()?
> 
> How about we remove the `jarsigner -verify` call? Like this:
> 
>          // double-check reading the files with a verifying JarFile  
>       try (JarFile jar = new JarFile(jarFilename4, true)) {          
>   if (firstAddedFilename != null) {                 JarEntry je1 =
> jar.getJarEntry(firstAddedFilename);                
> jar.getInputStream(je1).readAllBytes();-               
> assertTrue(je1.getCodeSigners().length > 0);+               
> CodeSigner[] ss = je1.getCodeSigners();+               
> assertTrue(ss.length == 2);+               
> assertTrue(containsSignerA(ss));             }             if
> (secondAddedFilename != null) {                 JarEntry je2 =
> jar.getJarEntry(secondAddedFilename);                
> jar.getInputStream(je2).readAllBytes();-               
> assertTrue(je2.getCodeSigners().length > 0);+               
> CodeSigner[] ss = je2.getCodeSigners();+               
> assertTrue(ss.length == 1);+               
> assertFalse(containsSignerA(ss));             }         } 
> @@ -392,6 +380,17 @@         }     } 
> +    private static boolean containsSignerA(CodeSigner[] ss) {+     
>   for (CodeSigner s : ss) {+            X509Certificate x =
> (X509Certificate)+                   
> s.getSignerCertPath().getCertificates().get(0);+            if
> (x.getSubjectX500Principal().toString().equals("CN=A")) {+           
>     return true;+            }+        }+        return false;+    }+
> Thanks,Max
> > On Jul 22, 2019, at 10:02 AM, Weijun Wang <weijun.wang at oracle.com>
> > wrote:
> > 
> > Please take a review at
> > 
> >   http://cr.openjdk.java.net/~weijun/8228456/webrev.00/
> > 
> > The change is contributed by Philipp Kunz. Since we are now in RDP
> > 2 and we are not allowed to fix non-test P3 (and lower) bugs. I've
> > removed all src/ changes in Philipp's patch (If I read correctly,
> > is mostly on renaming methods, and brings no behavior change) and
> > made necessary change in test codes to use original method names.
> > 
> > I haven't read the changes yet. I only made sure they pass on my
> > own system. Oracle's test farm is in maintenance during the
> > weekend.
> > 
> > Thanks,
> > Max
> > 
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/security-dev/attachments/20190728/f782d722/attachment.htm>