[icedtea-web] RFC: check for nulls in JNLPPolicy.isSystemJar

[Omair Majid]

On 03/01/2011 06:12 PM, Deepak Bhole wrote:
> * Omair Majid<omajid at redhat.com>  [2011-03-01 17:58]:
>> On 03/01/2011 05:41 PM, Deepak Bhole wrote:
>>> * Deepak Bhole<dbhole at redhat.com>   [2011-03-01 17:23]:
>>>> * Omair Majid<omajid at redhat.com>   [2011-03-01 17:06]:
>>>>> On 03/01/2011 04:48 PM, Deepak Bhole wrote:
>>>>>> * Omair Majid<omajid at redhat.com>    [2011-03-01 16:41]:
>>>>>>> Hi,
>>>>>>>
>>>>>>> The attached patch adds a null check in JNLPPolicy.isSystemJar. It's
>>>>>>> needed as PluginAppletSecurityContext.getAccessControlContext
>>>>>>> creates CodeSources with null locations.
>>>>>>>
>>>>>>
>>>>>> Why is this needed? The function is only called from getPermissions()
>>>>>> which does a check on source prior to calling:
>>>>>>
>>>>>>          if (source.equals(systemSource) || source.equals(shellSource))
>>>>>>              return getAllPermissions();
>>>>>>
>>>>>
>>>>> Actually, the code is more like this:
>>>>>
>>>>>      public PermissionCollection getPermissions(CodeSource source) {
>>>>>          if (source.equals(systemSource) || source.equals(shellSource))
>>>>>              return getAllPermissions();
>>>>>
>>>>>          if (isSystemJar(source)) {
>>>>>              return getAllPermissions();
>>>>>          }
>>>>>
>>>>
>>>> Yep, I know. Which means source is (in some way) looked at before
>>>> calling isSystemJar.
>>>>
>>>>> If source is null (or, more importantly, if source.location is null)
>>>>> isSystemJar may still be called. Without this patch, isSystemJar
>>>>> will throw a NullPointerException instead of returning false.
>>>>>
>>>>
>>>> Well source can't be null else .equals will throw an NPE. As for
>>>> source.getLocation.. systemSource is derived as:
>>>>
>>>> systemSource = Policy.class.getProtectionDomain().getCodeSource()
>>>>
>>>> which will have location == null
>>>>
>>>> So in theory there should never be a case where isSystemJar is called
>>>> will null source or where source.getLocation is null.
>>>>
>>>
>>> Sorry, scratch that. Just came to mind that location won't be null for
>>> systemSource, source will be null. So what is an example where location
>>> is null?
>>>
>>
>> One reproducer is at http://www.javatester.org/version.html. Wait
>> for the applet to load and then reload the page. A debugger gives me
>> a stack trace like this:
>>
>> Thread [Thread-22] (Suspended (exception NullPointerException))	
>> 	JNLPPolicy.isSystemJar(CodeSource) line: 143	
>> 	JNLPPolicy.getPermissions(CodeSource) line: 78	
>> 	AppletPanel$10.run() line: 1071	
>> 	AccessController.doPrivileged(PrivilegedAction<T>) line: not
>> available [native method] [local variables unavailable]	
>> 	NetxPanel(AppletPanel).getAccessControlContext(URL) line: 1066	
>> 	NetxPanel(AppletPanel).getClassLoader(URL, String) line: 1021	
>> 	NetxPanel(AppletPanel).sendEvent(int) line: 302	
>> 	PluginAppletViewer.appletShutdown(AppletPanel) line: 1628	
>> 	PluginAppletViewer.access$200(PluginAppletViewer, AppletPanel) line: 301	
>> 	PluginAppletViewer$5.run() line: 1651	
>> 	Thread.run() line: 679	
>>
>> The culprit line is:
>> return p.getPermissions(new CodeSource(null,
>> (java.security.cert.Certificate[]) null));
>>
>>
>> I also see something similar in
>> plugin/icedteanp/java/sun/applet/PluginAppletSecurityContext.java
>> CodeSource cs = new CodeSource((URL) null,
>> (java.security.cert.Certificate[]) null);
>> But I am not sure how to trigger it. I just wanted to point out that
>> there are cases where null location is possible. If the policy is
>> ever consulted on this CodeSource, the code blows up.
>>
>> Hope that makes things clear.
>>
>
> Ah I see. Thanks for the clarification. Okay for HEAD.

Thanks for reviewing the patch. Pushed.

Cheers,
Omair