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

[Jiri Vanek]

On 11/05/2013 04:54 PM, Andrew Azores wrote:
> 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.
>

ok then :)