[rfc][icedtea-web] Remove bad debug logging statements from JNLPSecurityManager

[Andrew Azores]

On 11/05/2013 10:33 AM, Andrew Azores wrote:
> On 11/05/2013 03:44 AM, Jiri Vanek wrote:
>> So I would say, that instead of removing the messages, somethinkg like:
>>
>> >            if (JNLPRuntime.isDebug()) {
>> if (!cl.isSecurityNull()}{
>> OutputController.getLogger().log(OutputController.Level.ERROR_ALL, 
>> "Trying to get permission  for null  source to environment with 
>> security, its wrong ");
>> }else { //conntinue as before
>> >                if (cl.getPermissions(null).implies(perm)){
>> }
>
> Ehh, if these debug messages are important I guess. The method now 
> contains significantly more logic for dealing with if/when/how to 
> produce debug output than actually doing its job :) and what if 
> "security" appears in between checking if it's null and calling 
> getPermissions(null)? Then we can still end up with an NPE being 
> thrown that would never have occurred if we weren't in debug mode. I 
> don't know if that situation would ever arise right now, where we 
> would have a thread calling addPermission on the security manager 
> while the classloader is still in the process of being initialized by 
> another thread. But there's a small chance of that happening anyway.
>
>> Also I have suspicions that this can hide more serious bug:
>>   - why passing null to getPemrmission
>
> I have no idea, I'm guessing this was written expecting getPermissions 
> to just skip that block of code rather than throwing an NPE. If it 
> just skipped that block of code rather than throwing an NPE then the 
> debug statement actually makes some sense, since calling addPermission 
> here adds it to the classloader's runtimePermissions field. 
> getPermissions' result would include permissions from that field if 
> the NPE was not thrown.
>
>>   - why if (cs == null) {                    throw new 
>> NullPointerException("Code source was null");                } if 
>> security is
>
> Well the rest of the code in that block is pretty dependent on having 
> that CodeSource...
>
>>   - and esepcially  why  // Class from host X should be allowed to 
>> connect to host X
>>             if (cs.getLocation() != null && 
>> cs.getLocation().getHost().length() > 0)
>>                 result.add(new 
>> SocketPermission(cs.getLocation().getHost(),
>>                         "connect, accept"));
>>   when is not?
>
> I don't know, I'm assuming this is just here to ensure that that 
> permission is included in the result whether or not "security" exists. 
> If security exists I'd imagine this permission would be included in 
> the result collection by this point, but if security is null then it 
> probably won't be (unless somehow it was a user-granted permission). 
> Allowing permission to accept local socket connections seems fine to me.
>
> Thanks,
>

Oops, forgot the attachment.

Thanks,

-- 
Andrew A

-------------- next part --------------
A non-text attachment was scrubbed...
Name: JNLPSecurityManager-bad-debug-2.patch
Type: text/x-patch
Size: 1482 bytes
Desc: not available
Url : http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20131105/748546cc/JNLPSecurityManager-bad-debug-2-0001.patch