Regression in itw from Tue Mar 26

[Jiri Vanek]

On 04/22/2013 08:58 PM, Adam Domurad wrote:
> 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

oook so something like this?

-------------- next part --------------
A non-text attachment was scrubbed...
Name: fixedRegression.patch
Type: text/x-patch
Size: 4925 bytes
Desc: not available
Url : http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20130423/705bae6f/fixedRegression.patch