[rfc][icedtea-web] extended reflection tests

[Jiri Vanek]

On 03/06/2012 05:59 PM, Danesh Dadachanji wrote:
> On 06/03/12 05:41 AM, Jiri Vanek wrote:
>> On 03/05/2012 11:23 PM, Danesh Dadachanji wrote:
>>> Hi Jiri,
>>>
>>> Thanks for the tests!
>>>
>>> On 05/03/12 07:55 AM, Jiri Vanek wrote:
>>>> Hi! Few more test to reflection
>>>>
>>>> What I'm surprised is, that unsigned application can use reflection at
>>>> all. I tough they can not....
>>>
>>> I thought so too..
>> Pavel have thoughts taht it should be correct..adding him to cc for case
>> I'm lying. Also Omair cced as he had something to do with reflection too..
>>>
>>>> This text is expecting them unsigned ones using reflection as correct as
>>>> it is current behaviour. But feel free to correct me if unsigned
>>>> applications should have reflection forbidden.
>>>
>>> I'll let someone who knows more comment on this. Until then, I have
>>> some comments on the patch below.
>> see above
>>>
>>>>
>>>> 2012-03-05 Jiri Vanek <jvanek at redhat.com>
>>>>
>>>> Improved reflection test:
>>>> *
>>>> /home/jvanek/hg/icedtea-web/tests/jnlp_tests/simple/AccessClassInPackage/testcases/AccessClassInPackageTest.java:
>>>>
>>>>
>>>> This testcase was extended for three more unsigned reflection tries
>>>> and four signed
>>>> */home/jvanek/hg/icedtea-web/tests/jnlp_tests/simple/AccessClassInPackage/srcs/AccessClassInPackage.java:
>>>>
>>>> now accepting class to be findByName as argument. Four new jnlp
>>>> files in
>>>> signed a four in simple are then passing those argument
>>>> */home/jvanek/hg/icedtea-web/tests/jnlp_tests/simple/AccessClassInPackage/resources/AccessClassInPackageSUNSEC.jnlp:
>>>>
>>>> */home/jvanek/hg/icedtea-web/tests/jnlp_tests/simple/AccessClassInPackage/resources/AccessClassInPackageNETSF.jnlp:
>>>>
>>>> */home/jvanek/hg/icedtea-web/tests/jnlp_tests/simple/AccessClassInPackage/resources/AccessClassInPackageJAVAXJNLP.jnlp:
>>>>
>>>> */home/jvanek/hg/icedtea-web/tests/jnlp_tests/simple/AccessClassInPackage/resources/AccessClassInPackageSELF.jnlp:
>>>>
>>>> */home/jvanek/hg/icedtea-web/tests/jnlp_tests/simple/AccessClassInPackage/resources/AccessClassInPackage.jnlp:
>>>>
>>>> removed
>>>
>>> Don't think we need your full path of "/home/jvanek/hg/icedtea-web/" ;)
>> Damn, thanx for catch.
>>>
>>> Please can you add <vendor> to all the jnlp files. It'd work right now
>>> but once the changeset I have in review is accepted, it'll start
>>> failing to run javaws altogether.
>>
>> Done for this case, But I'm afraid you will break more tests. Before
>> commit of your changeset you should ensure (fix missing vendors in
>> current tests) that no new failures appears.
>> I will also add vendors in all current and future tests.
>
> Yeah I suspected that was the case. I'll update my patch to include fixes for all the current tests.
>
>>>
>>> I believe you also need to add the copyright header for many of the
>>> files.
>> done. I always forgotr about jnlp files :-/
>>>
>>>> + private String[] badExceptions = {
>>>> + "accessClassInPackage.javax.jnlp.ServiceManager)",
>>>> + "accessClassInPackage.AccessClassInPackage)",
>>>> + "accessClassInPackage.net.sourceforge.jnlp)",
>>>> + "accessClassInPackage.sun.security.internal.spec)"
>>>
>>> [snip]
>>>
>>>> + private void testShouldFail(ServerAccess.ProcessResult pr, String s) {
>>>> + String c = "java.security.AccessControlException: access denied
>>>> (java.lang.RuntimePermission " + s;
>>>> + Assert.assertTrue("stderr should contains `" + c + "`, but didn't
>>>> ", pr.stderr.contains(c));
>>>> + }
>>>> + private void testShouldNOTFail(ServerAccess.ProcessResult pr,
>>>> String s) {
>>>> + String c = "java.security.AccessControlException: access denied
>>>> (java.lang.RuntimePermission " + s;
>>>> + Assert.assertFalse("stderr should NOT contains `" + c + "`, but did
>>>> ", pr.stderr.contains(c));
>>>> + }
>>>
>>>
>>> I noticed testShouldFail asserts were always failing when the methods
>>> were being called. I believe you need to add double quotes around
>>> java.lang.RuntimePermission and s in order for contains() to pass.
>>>
>>> stderr.log outputted:
>>> [...] access denied ("java.lang.RuntimePermission"
>>> "accessClassInPackage.sun.security.internal.spec")
>>>
>>> Change the String c's to (\"java.lang.RuntimePermission\" and wrap
>>> each element of badExceptions similarly, that should take care of the
>>> problem.
>> hmm.. this scares me a lot. I have changed catching from contains to
>> match, to have more vague regex inside. But what scares me is that I
>> have never seen this kind of exception in format you have described...
>> Attached is my exception format (same on f14 and f16) and new patch.
>
> Interesting, my F16 has "" around it. I built it with stock 6 (did not use --with-jdk-home).
>
> I don't think using matches with .* is the best idea. In this case, pr.stderr is a String, right? That means that if let's say stderr contains "java.security.AccessControlException" and then somewhere later on contains the other part we're searching for, it will return true.
>

True. I tought you will complain like this :)
What do you think about this?

     private void testShouldFail(ServerAccess.ProcessResult pr, String s) {
         String c = "(?s).*java.security.AccessControlException.{0,5}access denied.{0,5}java.lang.RuntimePermission.*" + s + ".*";
         Assert.assertTrue("stderr should match `" + c + "`, but didn't ", pr.stderr.matches(c));
     }

     private void testShouldNOTFail(ServerAccess.ProcessResult pr, String s) {
         String c = "(?s).*java.security.AccessControlException.{0,5}access denied.{0,5}java.lang.RuntimePermission.*" + s + ".*";
         Assert.assertFalse("stderr should NOT match `" + c + "`, but did ", pr.stderr.matches(c));
     }


> Perhaps an alternative solution for now is to match 0 or 1 '"' chars around the exceptions, instead of .* being used. Maybe leave a warning in comments saying stderr could be formatted differently. Then, if someone runs into this later on down the line, we can adjust to their output too.
>
> Attached is the exception I'm seeing. Hope this helps!
>
>>>
>>>
>>> Once the above's sorted out, this looks good to me for pushing.
>>>
>>> Regards,
>>> Danesh
>>
>>
>> 2012-03-06 Jiri Vanek <jvanek at redhat.com>
>>
>> Improved reflection test:
>> *
>> tests/jnlp_tests/simple/AccessClassInPackage/testcases/AccessClassInPackageTest.java:
>>
>> This testcase was extended for three more unsigned reflection tries and
>> four signed
>> *tests/jnlp_tests/simple/AccessClassInPackage/srcs/AccessClassInPackage.java:
>>
>> now accepting class to be findByName as argument. Four new jnlp files in
>> signed a four in simple are then passing those argument
>> *tests/jnlp_tests/simple/AccessClassInPackage/resources/AccessClassInPackageSUNSEC.jnlp:
>>
>> *tests/jnlp_tests/simple/AccessClassInPackage/resources/AccessClassInPackageNETSF.jnlp:
>>
>> *tests/jnlp_tests/simple/AccessClassInPackage/resources/AccessClassInPackageJAVAXJNLP.jnlp:
>>
>> *tests/jnlp_tests/simple/AccessClassInPackage/resources/AccessClassInPackageSELF.jnlp:
>>
>> *tests/jnlp_tests/simple/AccessClassInPackage/resources/AccessClassInPackage.jnlp:
>>
>> removed
>> *
>> tests/jnlp_tests/signed/AccessClassInPackageSigned/srcs/AccessClassInPackageSigned.java
>>
>> signed variation of AccessClassInPackage, tescase is also in
>> AccessClassInPackage
>> *
>> tests/jnlp_tests/signed/AccessClassInPackageSigned/resources/AccessClassInPackageSignedSELF.jnlp
>>
>> *
>> tests/jnlp_tests/signed/AccessClassInPackageSigned/resources/AccessClassInPackageSignedNETSF.jnlp
>>
>> *
>> tests/jnlp_tests/signed/AccessClassInPackageSigned/resources/AccessClassInPackageSignedSUNSEC.jnlp
>>
>> *
>> tests/jnlp_tests/signed/AccessClassInPackageSigned/resources/AccessClassInPackageSignedJAVAXJNLP.jnlp
>>
>>