Regression in itw from Tue Mar 26

Adam Domurad adomurad at redhat.com
Mon Apr 22 11:58:38 PDT 2013


Discussion fell off the list by accident, putting back.

On 04/22/2013 01:49 PM, Jiri Vanek wrote:
> On 04/18/2013 06:45 PM, Adam Domurad wrote:
>> The test seems wrong to me, which needs to be clarified before we can
>> properly address if this is a problem. I don't see any case where
>> SecurityDesc could have a null file, and this is the code causing the
>> difference:
>
> the test is doing what it is doing - testing SecurityDesc with null
> file .

See this mail thread:
http://permalink.gmane.org/gmane.comp.java.openjdk.distro-packaging.devel/19751

These tests were added without any use-case

It seems the correct solution to the original problem (investigated by 
checking out commit 507, right before it) would have been to just get 
rid of the uses of null here.

I am strongly in favour of reverting 508 or reworking the test to not 
check for the null jnlp files. NullJnlpFileException should be removed.

> To be honest I'm not sure whether it is possible "in real live" or
> not... (Applets?... just first thought - anyway see the difference
> between jnlp and applet evaluation of test).

No, applets do not have a null JNLPFile. See eg line 186 of PluginBridge 
(which extends JNLPFile).

> And was passing and is failing and due :
> -            boolean allSigned = true;
> +            boolean allSigned = (loaders.length > 1) /* has
> extensions */;
>
> But you may be (probably are) correct that it could hide more hidden
> issue.
>>
>> } else if (signing == true) {
>> this.security = file.getSecurity();
>> } else {
>> this.security = new SecurityDesc(file,
>> SecurityDesc.SANDBOX_PERMISSIONS,
>> codebase.getHost());
>>
>> If it's signing, it carries on the null file, if not, it overwrites
>> with its own SecurityDesc. Am I missing something ? What does a
>> null-file signify ? Looking at the code-paths that create
>> SecurityDesc's it seems like it cannot happen.
>
> to cite you for remembering"
> > Without this change, applets that do not have an associated jar will
> always be considered signed.
>
> Sounds quite serious!
>
> > Thus they will always run regardless of unsigned applet trust setting.
> > What does the broken test indicate ? "
>
> Maybe the behaviour of test is now correct... But who can say ....
>>
>> There may still be an issue, however it would not have been
>> introduced by my code.
> bad luck, your code made it vissible.

Surely not bad luck :-)
Anyway, I don't think there is an issue anymore. It seems like a bad 
test purely.

>>
>> Thanks,
>
>
> If you are right, and null file is not possible, then I'm for throw
> launcherror from every place where it can be set. And adapt unittest
> to it....

A null file is not possible in any location it is setting -- mostly 
stemming from JNLPFile methods themselves where it is obviously not 
null. It appeared purely from mocks.

I agree though: SecurityDesc should throw a NPE / argument exception if 
JNLPFile is ever null to prevent this in the future. We should eradicate 
it from the unit tests.

>
> J.
>

Happy hacking,
-Adam



More information about the distro-pkg-dev mailing list