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

[Andrew Azores]

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,

-- 
Andrew A