river-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Peter Firmstone <j...@zeus.net.au>
Subject Re: svn commit: r999115 - in /incubator/river/jtsk/trunk/src: com/sun/jini/tool/classdepend/ net/jini/jeri/ net/jini/security/
Date Tue, 21 Sep 2010 09:47:16 GMT
Jonathan Costers wrote:
> Hi Peter
>
> You are talking about this change, correct?
>   

Yes.
>
>  /**
>  * Permission required to dynamically grant permissions by security policy
> @@ -554,7 +555,7 @@ public final class GrantPermission exten
>      * of permissions.
>      */
>     private static String constructName(Permission[] pa) {
> -       StringBuffer sb = new StringBuffer(60);
> +       StringBuffer sb = new StringBuffer();
>   

16 characters wasn't enough to avoid resizing.

>
> -> I'm curious, why is the above?
>
>
>        for (int i = 0; i < pa.length; i++) {
>            Permission p = pa[i];
>            if (p instanceof UnresolvedPermission) {
> @@ -675,7 +676,7 @@ public final class GrantPermission exten
>     private static class Implier {
>
>        private final PermissionCollection perms = new Permissions();
> -       private final List unresolved =new ArrayList();
> +       private final ArrayList unresolved = new ArrayList();
>
>
> -> I'm curious, why is the above?
>   

Can't remember, there shouldn't be too many unresolved permissions, so 
I've haven't set a default size greater than 16, I've probably changed 
the ArrayList to List, simply because List is a standard interface and 
no ArrayList specific methods were used.  Minor Refactoring.

>
>        void add(GrantPermission gp) {
>            for (int i = 0; i < gp.grants.length; i++) {
> @@ -752,8 +753,7 @@ public final class GrantPermission exten
>      * @serial include
>      */
>     static class GrantPermissionCollection extends PermissionCollection {
> -        // All access is synchronized through GrantPermissionCollection
> -        // Nothing within should use synchronization
> +
>        private static final long serialVersionUID = 8227621799817733985L;
>
>        /**
> @@ -763,7 +763,7 @@ public final class GrantPermission exten
>            new ObjectStreamField("perms", List.class, true)
>        };
>
> -       private List perms = new ArrayList(40);
> +       private List perms = new ArrayList();
>
>
> -> I'm curious, why is the above?
>   
>        private Implier implier = new Implier();
>
>        public synchronized void add(Permission p) {
> @@ -774,10 +774,8 @@ public final class GrantPermission exten
>                throw new SecurityException(
>                    "can't add to read-only PermissionCollection");
>            }
> -           if (!perms.contains(p)){
> -               perms.add(p);
> -               implier.add((GrantPermission) p);
> -           }
> +           perms.add(p);
> +           implier.add((GrantPermission) p);
>        }
>
>
> -> I'm curious, why is the above?
>   

This prevents duplicates being added to perms, under some circumstances 
a Memory exception occurred while expanding UmbrellaGrantPermission 
dynamic grants, in the DynamicPolicy (ConcurrentDynamicPolicyProvider), 
DynamicPolicyProvider doesn't expand UmbrellaGrantPermission's.  Feature 
was requested on Jira.

I'd have to go over the code again to remember exactly how it was 
caused, but this was part of the fix, it was something to do with 
expanding UmbrellaGrant's getting caught in an ever expanding loop.

I added a comment about synchronization also.

Peter.

>
> Sorry again, I was quite tired last night ... This slipped my attention
> while doing all these tiny changes to fix javadoc.
>
> Best
> Jonathan
>
> 2010/9/21 Jonathan Costers <jonathan.costers@googlemail.com>
>
>   
>> I will restore your improvements, apologies.
>> What I wanted to do is back out some of your import removals, since they
>> broke javadoc generation for those classes.
>> Good to see people are awake :-)
>>
>> 2010/9/21 Peter Firmstone <jini@zeus.net.au>
>>
>> I'm curious about these changes, in your commit, what concerns me here is
>>     
>>> not the removal of some performance improvements, but a bug that I found
>>> while expanding UmbrellaGrant's.
>>>
>>> I guess I should have created a regression test and documented it.
>>>
>>> It relates to a memory out of bounds exception that is thrown if a dynamic
>>> policy supports the use of UmbrellaGrantPermission, as requested on JIRA.
>>>
>>> Not everything I did should be considered experimental.  I've removed all
>>> experimental changes.
>>>
>>> Rather than remove my hard work, wouldn't it be better to just find the
>>> build that passes all tests in svn, like Patricia was doing?
>>>
>>> I'm assuming you removed it so a test passes?  If this change caused a
>>> test failure, then the bug is not this piece of code, we should be
>>> investigating why this change causes that test to fail, so we can solve the
>>> bug, rather than obscure it.
>>>
>>> Peter.
>>>
>>>
>>> jcosters@apache.org wrote:
>>>
>>>       
>>>> Modified:
>>>> incubator/river/jtsk/trunk/src/net/jini/security/GrantPermission.java
>>>> URL:
>>>> http://svn.apache.org/viewvc/incubator/river/jtsk/trunk/src/net/jini/security/GrantPermission.java?rev=999115&r1=999114&r2=999115&view=diff
>>>>
>>>> ==============================================================================
>>>> --- incubator/river/jtsk/trunk/src/net/jini/security/GrantPermission.java
>>>> (original)
>>>> +++ incubator/river/jtsk/trunk/src/net/jini/security/GrantPermission.java
>>>> Mon Sep 20 20:56:19 2010
>>>> @@ -41,6 +41,7 @@ import java.util.Enumeration;
>>>>  import java.util.HashSet;
>>>>  import java.util.Iterator;
>>>>  import java.util.List;
>>>> +import net.jini.security.policy.DynamicPolicy;
>>>>  /**
>>>>  * Permission required to dynamically grant permissions by security
>>>> policy
>>>> @@ -554,7 +555,7 @@ public final class GrantPermission exten
>>>>      * of permissions.
>>>>      */
>>>>     private static String constructName(Permission[] pa) {
>>>> -       StringBuffer sb = new StringBuffer(60);
>>>> +       StringBuffer sb = new StringBuffer();
>>>>        for (int i = 0; i < pa.length; i++) {
>>>>            Permission p = pa[i];
>>>>            if (p instanceof UnresolvedPermission) {
>>>> @@ -675,7 +676,7 @@ public final class GrantPermission exten
>>>>     private static class Implier {
>>>>
>>>>        private final PermissionCollection perms = new Permissions();
>>>> -       private final List unresolved =new ArrayList();
>>>> +       private final ArrayList unresolved = new ArrayList();
>>>>        void add(GrantPermission gp) {
>>>>            for (int i = 0; i < gp.grants.length; i++) {
>>>> @@ -752,8 +753,7 @@ public final class GrantPermission exten
>>>>      * @serial include
>>>>      */
>>>>     static class GrantPermissionCollection extends PermissionCollection {
>>>> -        // All access is synchronized through GrantPermissionCollection
>>>> -        // Nothing within should use synchronization +
>>>>        private static final long serialVersionUID = 8227621799817733985L;
>>>>        /**
>>>> @@ -763,7 +763,7 @@ public final class GrantPermission exten
>>>>            new ObjectStreamField("perms", List.class, true)
>>>>        };
>>>>  -      private List perms = new ArrayList(40);
>>>> +       private List perms = new ArrayList();
>>>>        private Implier implier = new Implier();
>>>>        public synchronized void add(Permission p) {
>>>> @@ -774,10 +774,8 @@ public final class GrantPermission exten
>>>>                throw new SecurityException(
>>>>                    "can't add to read-only PermissionCollection");
>>>>            }
>>>> -           if (!perms.contains(p)){
>>>> -               perms.add(p);
>>>> -               implier.add((GrantPermission) p);
>>>> -           }
>>>> +           perms.add(p);
>>>> +           implier.add((GrantPermission) p);
>>>>        }
>>>>
>>>>        public synchronized Enumeration elements() {
>>>>
>>>>
>>>>
>>>>         
>>>       
>
>   


Mime
View raw message