You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@harmony.apache.org by "Wendy Feng (JIRA)" <ji...@apache.org> on 2010/09/28 05:15:33 UTC

[jira] Created: (HARMONY-6661) Synchonrize on mutable field in Permissions.java

Synchonrize on mutable field in Permissions.java
------------------------------------------------

                 Key: HARMONY-6661
                 URL: https://issues.apache.org/jira/browse/HARMONY-6661
             Project: Harmony
          Issue Type: Bug
          Components: Classlib
    Affects Versions: 6.0M1
         Environment: Windows XP
            Reporter: Wendy Feng


I found a unsafe synchronization in modules/security/src/main/java/common/java/security/Permissions.java
public final class Permissions extends PermissionCollection implements Serializable {
...
private void readObject(java.io.ObjectInputStream in) throws IOException,
        ClassNotFoundException {
...
        klasses = new HashMap();
        synchronized (klasses) {
            for (Iterator iter = perms.entrySet().iterator(); iter.hasNext();) {
                Map.Entry entry = (Map.Entry)  iter.next();
                Class key = (Class) entry.getKey();
                PermissionCollection pc = (PermissionCollection) entry.getValue();
                if (key != pc.elements().nextElement().getClass()) {
                    throw new InvalidObjectException(Messages.getString("security.22")); //$NON-NLS-1$
                }
                klasses.put(key, pc);
            }
        }
    ...
  }
...
}

In the above code , a block is synchronized on klasses field. Before the synchronized block, klasses is assigned to a new value.

Consequence: 
Different threads will synchronize on different klasses objects because it has been assigned to a new value. It  breaks the mutual exclusion and update on klasses would be lost.

I suggest to rewrite it as follow:
public final class Permissions extends PermissionCollection implements Serializable {
   private static final Object monitor = new Object();
...
private void readObject(java.io.ObjectInputStream in) throws IOException,
        ClassNotFoundException {
...
        klasses = new HashMap();
        synchronized (monitor ) {
            for (Iterator iter = perms.entrySet().iterator(); iter.hasNext();) {
                Map.Entry entry = (Map.Entry)  iter.next();
                Class key = (Class) entry.getKey();
                PermissionCollection pc = (PermissionCollection) entry.getValue();
                if (key != pc.elements().nextElement().getClass()) {
                    throw new InvalidObjectException(Messages.getString("security.22")); //$NON-NLS-1$
                }
                klasses.put(key, pc);
            }
        }
    ...
  }
...
}


-- 
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.


Re: [jira] Created: (HARMONY-6661) Synchonrize on mutable field in Permissions.java

Posted by Regis <xu...@gmail.com>.
On 2010-09-29 15:04, Alexey Varlamov wrote:
> You are correct, synchronization in readObject is superfluous.
> Serialization is different indeed, yet readObject is irrelevant.
>
> --
> WBR,
> Alexey
>
>>
>> In this case, readObject play a role like constructor, initialize the
>> object, so I guess it could be safe here without "synchornized" block. But
>> serialization may be different, is there any chance that someone could hold
>> a reference to a incompletely deserialized object and then invoke methods on
>> it?
>>
>> --
>> Best Regards,
>> Regis.
>>
>

Thanks for the explanation in JIRA, Alexey, I just removed "synchronized" block 
in Permissions.readObject() at r1002506.

-- 
Best Regards,
Regis.

Re: [jira] Created: (HARMONY-6661) Synchonrize on mutable field in Permissions.java

Posted by Alexey Varlamov <al...@gmail.com>.
You are correct, synchronization in readObject is superfluous.
Serialization is different indeed, yet readObject is irrelevant.

--
WBR,
Alexey

>
> In this case, readObject play a role like constructor, initialize the
> object, so I guess it could be safe here without "synchornized" block. But
> serialization may be different, is there any chance that someone could hold
> a reference to a incompletely deserialized object and then invoke methods on
> it?
>
> --
> Best Regards,
> Regis.
>

Re: [jira] Created: (HARMONY-6661) Synchonrize on mutable field in Permissions.java

Posted by Regis <xu...@gmail.com>.
On 2010-09-28 11:15, Wendy Feng (JIRA) wrote:
> Synchonrize on mutable field in Permissions.java
> ------------------------------------------------
>
>                   Key: HARMONY-6661
>                   URL: https://issues.apache.org/jira/browse/HARMONY-6661
>               Project: Harmony
>            Issue Type: Bug
>            Components: Classlib
>      Affects Versions: 6.0M1
>           Environment: Windows XP
>              Reporter: Wendy Feng
>
>
> I found a unsafe synchronization in modules/security/src/main/java/common/java/security/Permissions.java
> public final class Permissions extends PermissionCollection implements Serializable {
> ...
> private void readObject(java.io.ObjectInputStream in) throws IOException,
>          ClassNotFoundException {
> ...
>          klasses = new HashMap();
>          synchronized (klasses) {
>              for (Iterator iter = perms.entrySet().iterator(); iter.hasNext();) {
>                  Map.Entry entry = (Map.Entry)  iter.next();
>                  Class key = (Class) entry.getKey();
>                  PermissionCollection pc = (PermissionCollection) entry.getValue();
>                  if (key != pc.elements().nextElement().getClass()) {
>                      throw new InvalidObjectException(Messages.getString("security.22")); //$NON-NLS-1$
>                  }
>                  klasses.put(key, pc);
>              }
>          }
>      ...
>    }
> ...
> }
>
> In the above code , a block is synchronized on klasses field. Before the synchronized block, klasses is assigned to a new value.
>
> Consequence:
> Different threads will synchronize on different klasses objects because it has been assigned to a new value. It  breaks the mutual exclusion and update on klasses would be lost.
>
> I suggest to rewrite it as follow:
> public final class Permissions extends PermissionCollection implements Serializable {
>     private static final Object monitor = new Object();
> ...
> private void readObject(java.io.ObjectInputStream in) throws IOException,
>          ClassNotFoundException {
> ...
>          klasses = new HashMap();
>          synchronized (monitor ) {
>              for (Iterator iter = perms.entrySet().iterator(); iter.hasNext();) {
>                  Map.Entry entry = (Map.Entry)  iter.next();
>                  Class key = (Class) entry.getKey();
>                  PermissionCollection pc = (PermissionCollection) entry.getValue();
>                  if (key != pc.elements().nextElement().getClass()) {
>                      throw new InvalidObjectException(Messages.getString("security.22")); //$NON-NLS-1$
>                  }
>                  klasses.put(key, pc);
>              }
>          }
>      ...
>    }
> ...
> }
>
>

In this case, readObject play a role like constructor, initialize the object, so 
I guess it could be safe here without "synchornized" block. But serialization 
may be different, is there any chance that someone could hold a reference to a 
incompletely deserialized object and then invoke methods on it?

-- 
Best Regards,
Regis.

[jira] Commented: (HARMONY-6661) Synchonrize on mutable field in Permissions.java

Posted by "Hudson (JIRA)" <ji...@apache.org>.
    [ https://issues.apache.org/jira/browse/HARMONY-6661?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12916703#action_12916703 ] 

Hudson commented on HARMONY-6661:
---------------------------------

Integrated in Harmony-1.5-head-linux-x86_64 #973 (See [https://hudson.apache.org/hudson/job/Harmony-1.5-head-linux-x86_64/973/])
    Apply fix for HARMONY-6661: Synchonrize on mutable field in Permissions.java

readObject() is only reachable by one thread, so remove unnecessary
"synchronized" block.


> Synchonrize on mutable field in Permissions.java
> ------------------------------------------------
>
>                 Key: HARMONY-6661
>                 URL: https://issues.apache.org/jira/browse/HARMONY-6661
>             Project: Harmony
>          Issue Type: Bug
>          Components: Classlib
>    Affects Versions: 6.0M1
>         Environment: Windows XP
>            Reporter: Wendy Feng
>            Assignee: Regis Xu
>             Fix For: 5.0M16
>
>   Original Estimate: 5h
>  Remaining Estimate: 5h
>
> I found a unsafe synchronization in modules/security/src/main/java/common/java/security/Permissions.java
> public final class Permissions extends PermissionCollection implements Serializable {
> ...
> private void readObject(java.io.ObjectInputStream in) throws IOException,
>         ClassNotFoundException {
> ...
>         klasses = new HashMap();
>         synchronized (klasses) {
>             for (Iterator iter = perms.entrySet().iterator(); iter.hasNext();) {
>                 Map.Entry entry = (Map.Entry)  iter.next();
>                 Class key = (Class) entry.getKey();
>                 PermissionCollection pc = (PermissionCollection) entry.getValue();
>                 if (key != pc.elements().nextElement().getClass()) {
>                     throw new InvalidObjectException(Messages.getString("security.22")); //$NON-NLS-1$
>                 }
>                 klasses.put(key, pc);
>             }
>         }
>     ...
>   }
> ...
> }
> In the above code , a block is synchronized on klasses field. Before the synchronized block, klasses is assigned to a new value.
> Consequence: 
> Different threads will synchronize on different klasses objects because it has been assigned to a new value. It  breaks the mutual exclusion and update on klasses would be lost.
> I suggest to rewrite it as follow:
> public final class Permissions extends PermissionCollection implements Serializable {
>    private static final Object monitor = new Object();
> ...
> private void readObject(java.io.ObjectInputStream in) throws IOException,
>         ClassNotFoundException {
> ...
>         klasses = new HashMap();
>         synchronized (monitor ) {
>             for (Iterator iter = perms.entrySet().iterator(); iter.hasNext();) {
>                 Map.Entry entry = (Map.Entry)  iter.next();
>                 Class key = (Class) entry.getKey();
>                 PermissionCollection pc = (PermissionCollection) entry.getValue();
>                 if (key != pc.elements().nextElement().getClass()) {
>                     throw new InvalidObjectException(Messages.getString("security.22")); //$NON-NLS-1$
>                 }
>                 klasses.put(key, pc);
>             }
>         }
>     ...
>   }
> ...
> }

-- 
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.


[jira] Reopened: (HARMONY-6661) Synchonrize on mutable field in Permissions.java

Posted by "Mark Hindess (JIRA)" <ji...@apache.org>.
     [ https://issues.apache.org/jira/browse/HARMONY-6661?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]

Mark Hindess reopened HARMONY-6661:
-----------------------------------


There are some other readObject methods with sync blocks perhaps we should fix them before closing this?  See:

  bash$ find classlib -name '*.java' -print0|xargs -0 perl -ne 'if (/^(\s+)private void readObject\(/) { $end = "^$1}\s*\$"; } elsif (/$end/) { undef $end } if (defined $end && /synchronized/) { print "$ARGV $.: $_\n";} } continue { close ARGV if eof' 
classlib/modules/beans/src/main/java/java/beans/beancontext/BeanContextServicesSupport.java line 1073:         synchronized (bcsListeners) {

classlib/modules/beans/src/main/java/java/beans/beancontext/BeanContextSupport.java line 1298:         synchronized (bcmListeners) {

classlib/modules/security/src/main/java/common/java/security/UnresolvedPermissionCollection.java line 177:         synchronized (klasses) {

classlib/modules/security/src/main/java/common/java/security/BasicPermissionCollection.java line 187:         synchronized (this) {

classlib/modules/security/src/main/java/common/java/security/Permissions.java line 217:         synchronized (klasses) {

(In case anyone is wondering about the '} continue {' in the perl line, it is to get $. reset on eof so the line numbers are correct.)

> Synchonrize on mutable field in Permissions.java
> ------------------------------------------------
>
>                 Key: HARMONY-6661
>                 URL: https://issues.apache.org/jira/browse/HARMONY-6661
>             Project: Harmony
>          Issue Type: Bug
>          Components: Classlib
>    Affects Versions: 6.0M1
>         Environment: Windows XP
>            Reporter: Wendy Feng
>            Assignee: Regis Xu
>             Fix For: 5.0M16
>
>   Original Estimate: 5h
>  Remaining Estimate: 5h
>
> I found a unsafe synchronization in modules/security/src/main/java/common/java/security/Permissions.java
> public final class Permissions extends PermissionCollection implements Serializable {
> ...
> private void readObject(java.io.ObjectInputStream in) throws IOException,
>         ClassNotFoundException {
> ...
>         klasses = new HashMap();
>         synchronized (klasses) {
>             for (Iterator iter = perms.entrySet().iterator(); iter.hasNext();) {
>                 Map.Entry entry = (Map.Entry)  iter.next();
>                 Class key = (Class) entry.getKey();
>                 PermissionCollection pc = (PermissionCollection) entry.getValue();
>                 if (key != pc.elements().nextElement().getClass()) {
>                     throw new InvalidObjectException(Messages.getString("security.22")); //$NON-NLS-1$
>                 }
>                 klasses.put(key, pc);
>             }
>         }
>     ...
>   }
> ...
> }
> In the above code , a block is synchronized on klasses field. Before the synchronized block, klasses is assigned to a new value.
> Consequence: 
> Different threads will synchronize on different klasses objects because it has been assigned to a new value. It  breaks the mutual exclusion and update on klasses would be lost.
> I suggest to rewrite it as follow:
> public final class Permissions extends PermissionCollection implements Serializable {
>    private static final Object monitor = new Object();
> ...
> private void readObject(java.io.ObjectInputStream in) throws IOException,
>         ClassNotFoundException {
> ...
>         klasses = new HashMap();
>         synchronized (monitor ) {
>             for (Iterator iter = perms.entrySet().iterator(); iter.hasNext();) {
>                 Map.Entry entry = (Map.Entry)  iter.next();
>                 Class key = (Class) entry.getKey();
>                 PermissionCollection pc = (PermissionCollection) entry.getValue();
>                 if (key != pc.elements().nextElement().getClass()) {
>                     throw new InvalidObjectException(Messages.getString("security.22")); //$NON-NLS-1$
>                 }
>                 klasses.put(key, pc);
>             }
>         }
>     ...
>   }
> ...
> }

-- 
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.


[jira] Commented: (HARMONY-6661) Synchonrize on mutable field in Permissions.java

Posted by "Hudson (JIRA)" <ji...@apache.org>.
    [ https://issues.apache.org/jira/browse/HARMONY-6661?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12916684#action_12916684 ] 

Hudson commented on HARMONY-6661:
---------------------------------

Integrated in Harmony-select-1.5-head-linux-x86_64 #126 (See [https://hudson.apache.org/hudson/job/Harmony-select-1.5-head-linux-x86_64/126/])
    Apply fix for HARMONY-6661: Synchonrize on mutable field in Permissions.java

readObject() is only reachable by one thread, so remove unnecessary
"synchronized" block.


> Synchonrize on mutable field in Permissions.java
> ------------------------------------------------
>
>                 Key: HARMONY-6661
>                 URL: https://issues.apache.org/jira/browse/HARMONY-6661
>             Project: Harmony
>          Issue Type: Bug
>          Components: Classlib
>    Affects Versions: 6.0M1
>         Environment: Windows XP
>            Reporter: Wendy Feng
>            Assignee: Regis Xu
>             Fix For: 5.0M16
>
>   Original Estimate: 5h
>  Remaining Estimate: 5h
>
> I found a unsafe synchronization in modules/security/src/main/java/common/java/security/Permissions.java
> public final class Permissions extends PermissionCollection implements Serializable {
> ...
> private void readObject(java.io.ObjectInputStream in) throws IOException,
>         ClassNotFoundException {
> ...
>         klasses = new HashMap();
>         synchronized (klasses) {
>             for (Iterator iter = perms.entrySet().iterator(); iter.hasNext();) {
>                 Map.Entry entry = (Map.Entry)  iter.next();
>                 Class key = (Class) entry.getKey();
>                 PermissionCollection pc = (PermissionCollection) entry.getValue();
>                 if (key != pc.elements().nextElement().getClass()) {
>                     throw new InvalidObjectException(Messages.getString("security.22")); //$NON-NLS-1$
>                 }
>                 klasses.put(key, pc);
>             }
>         }
>     ...
>   }
> ...
> }
> In the above code , a block is synchronized on klasses field. Before the synchronized block, klasses is assigned to a new value.
> Consequence: 
> Different threads will synchronize on different klasses objects because it has been assigned to a new value. It  breaks the mutual exclusion and update on klasses would be lost.
> I suggest to rewrite it as follow:
> public final class Permissions extends PermissionCollection implements Serializable {
>    private static final Object monitor = new Object();
> ...
> private void readObject(java.io.ObjectInputStream in) throws IOException,
>         ClassNotFoundException {
> ...
>         klasses = new HashMap();
>         synchronized (monitor ) {
>             for (Iterator iter = perms.entrySet().iterator(); iter.hasNext();) {
>                 Map.Entry entry = (Map.Entry)  iter.next();
>                 Class key = (Class) entry.getKey();
>                 PermissionCollection pc = (PermissionCollection) entry.getValue();
>                 if (key != pc.elements().nextElement().getClass()) {
>                     throw new InvalidObjectException(Messages.getString("security.22")); //$NON-NLS-1$
>                 }
>                 klasses.put(key, pc);
>             }
>         }
>     ...
>   }
> ...
> }

-- 
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.


[jira] Assigned: (HARMONY-6661) Synchonrize on mutable field in Permissions.java

Posted by "Regis Xu (JIRA)" <ji...@apache.org>.
     [ https://issues.apache.org/jira/browse/HARMONY-6661?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]

Regis Xu reassigned HARMONY-6661:
---------------------------------

    Assignee: Regis Xu

> Synchonrize on mutable field in Permissions.java
> ------------------------------------------------
>
>                 Key: HARMONY-6661
>                 URL: https://issues.apache.org/jira/browse/HARMONY-6661
>             Project: Harmony
>          Issue Type: Bug
>          Components: Classlib
>    Affects Versions: 6.0M1
>         Environment: Windows XP
>            Reporter: Wendy Feng
>            Assignee: Regis Xu
>   Original Estimate: 5h
>  Remaining Estimate: 5h
>
> I found a unsafe synchronization in modules/security/src/main/java/common/java/security/Permissions.java
> public final class Permissions extends PermissionCollection implements Serializable {
> ...
> private void readObject(java.io.ObjectInputStream in) throws IOException,
>         ClassNotFoundException {
> ...
>         klasses = new HashMap();
>         synchronized (klasses) {
>             for (Iterator iter = perms.entrySet().iterator(); iter.hasNext();) {
>                 Map.Entry entry = (Map.Entry)  iter.next();
>                 Class key = (Class) entry.getKey();
>                 PermissionCollection pc = (PermissionCollection) entry.getValue();
>                 if (key != pc.elements().nextElement().getClass()) {
>                     throw new InvalidObjectException(Messages.getString("security.22")); //$NON-NLS-1$
>                 }
>                 klasses.put(key, pc);
>             }
>         }
>     ...
>   }
> ...
> }
> In the above code , a block is synchronized on klasses field. Before the synchronized block, klasses is assigned to a new value.
> Consequence: 
> Different threads will synchronize on different klasses objects because it has been assigned to a new value. It  breaks the mutual exclusion and update on klasses would be lost.
> I suggest to rewrite it as follow:
> public final class Permissions extends PermissionCollection implements Serializable {
>    private static final Object monitor = new Object();
> ...
> private void readObject(java.io.ObjectInputStream in) throws IOException,
>         ClassNotFoundException {
> ...
>         klasses = new HashMap();
>         synchronized (monitor ) {
>             for (Iterator iter = perms.entrySet().iterator(); iter.hasNext();) {
>                 Map.Entry entry = (Map.Entry)  iter.next();
>                 Class key = (Class) entry.getKey();
>                 PermissionCollection pc = (PermissionCollection) entry.getValue();
>                 if (key != pc.elements().nextElement().getClass()) {
>                     throw new InvalidObjectException(Messages.getString("security.22")); //$NON-NLS-1$
>                 }
>                 klasses.put(key, pc);
>             }
>         }
>     ...
>   }
> ...
> }

-- 
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.


[jira] Commented: (HARMONY-6661) Synchonrize on mutable field in Permissions.java

Posted by "Alexey Varlamov (JIRA)" <ji...@apache.org>.
    [ https://issues.apache.org/jira/browse/HARMONY-6661?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12916054#action_12916054 ] 

Alexey Varlamov commented on HARMONY-6661:
------------------------------------------

Synchronization on a static field is certainly a bad idea. 
In fact synchronization should be just omitted here, see [1]: 
"By definition, an object created by deserialization is only reachable by one thread, and thus there is no need for readObject() to be synchronized."
[1] http://findbugs.sourceforge.net/bugDescriptions.html#RS_READOBJECT_SYNC

> Synchonrize on mutable field in Permissions.java
> ------------------------------------------------
>
>                 Key: HARMONY-6661
>                 URL: https://issues.apache.org/jira/browse/HARMONY-6661
>             Project: Harmony
>          Issue Type: Bug
>          Components: Classlib
>    Affects Versions: 6.0M1
>         Environment: Windows XP
>            Reporter: Wendy Feng
>   Original Estimate: 5h
>  Remaining Estimate: 5h
>
> I found a unsafe synchronization in modules/security/src/main/java/common/java/security/Permissions.java
> public final class Permissions extends PermissionCollection implements Serializable {
> ...
> private void readObject(java.io.ObjectInputStream in) throws IOException,
>         ClassNotFoundException {
> ...
>         klasses = new HashMap();
>         synchronized (klasses) {
>             for (Iterator iter = perms.entrySet().iterator(); iter.hasNext();) {
>                 Map.Entry entry = (Map.Entry)  iter.next();
>                 Class key = (Class) entry.getKey();
>                 PermissionCollection pc = (PermissionCollection) entry.getValue();
>                 if (key != pc.elements().nextElement().getClass()) {
>                     throw new InvalidObjectException(Messages.getString("security.22")); //$NON-NLS-1$
>                 }
>                 klasses.put(key, pc);
>             }
>         }
>     ...
>   }
> ...
> }
> In the above code , a block is synchronized on klasses field. Before the synchronized block, klasses is assigned to a new value.
> Consequence: 
> Different threads will synchronize on different klasses objects because it has been assigned to a new value. It  breaks the mutual exclusion and update on klasses would be lost.
> I suggest to rewrite it as follow:
> public final class Permissions extends PermissionCollection implements Serializable {
>    private static final Object monitor = new Object();
> ...
> private void readObject(java.io.ObjectInputStream in) throws IOException,
>         ClassNotFoundException {
> ...
>         klasses = new HashMap();
>         synchronized (monitor ) {
>             for (Iterator iter = perms.entrySet().iterator(); iter.hasNext();) {
>                 Map.Entry entry = (Map.Entry)  iter.next();
>                 Class key = (Class) entry.getKey();
>                 PermissionCollection pc = (PermissionCollection) entry.getValue();
>                 if (key != pc.elements().nextElement().getClass()) {
>                     throw new InvalidObjectException(Messages.getString("security.22")); //$NON-NLS-1$
>                 }
>                 klasses.put(key, pc);
>             }
>         }
>     ...
>   }
> ...
> }

-- 
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.


[jira] Resolved: (HARMONY-6661) Synchonrize on mutable field in Permissions.java

Posted by "Regis Xu (JIRA)" <ji...@apache.org>.
     [ https://issues.apache.org/jira/browse/HARMONY-6661?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]

Regis Xu resolved HARMONY-6661.
-------------------------------

    Fix Version/s: 5.0M16
       Resolution: Fixed

I just removed "synchronized" block in Permissions.readObject() at r1002506, please verify if it resolved your issue. Thanks.

> Synchonrize on mutable field in Permissions.java
> ------------------------------------------------
>
>                 Key: HARMONY-6661
>                 URL: https://issues.apache.org/jira/browse/HARMONY-6661
>             Project: Harmony
>          Issue Type: Bug
>          Components: Classlib
>    Affects Versions: 6.0M1
>         Environment: Windows XP
>            Reporter: Wendy Feng
>            Assignee: Regis Xu
>             Fix For: 5.0M16
>
>   Original Estimate: 5h
>  Remaining Estimate: 5h
>
> I found a unsafe synchronization in modules/security/src/main/java/common/java/security/Permissions.java
> public final class Permissions extends PermissionCollection implements Serializable {
> ...
> private void readObject(java.io.ObjectInputStream in) throws IOException,
>         ClassNotFoundException {
> ...
>         klasses = new HashMap();
>         synchronized (klasses) {
>             for (Iterator iter = perms.entrySet().iterator(); iter.hasNext();) {
>                 Map.Entry entry = (Map.Entry)  iter.next();
>                 Class key = (Class) entry.getKey();
>                 PermissionCollection pc = (PermissionCollection) entry.getValue();
>                 if (key != pc.elements().nextElement().getClass()) {
>                     throw new InvalidObjectException(Messages.getString("security.22")); //$NON-NLS-1$
>                 }
>                 klasses.put(key, pc);
>             }
>         }
>     ...
>   }
> ...
> }
> In the above code , a block is synchronized on klasses field. Before the synchronized block, klasses is assigned to a new value.
> Consequence: 
> Different threads will synchronize on different klasses objects because it has been assigned to a new value. It  breaks the mutual exclusion and update on klasses would be lost.
> I suggest to rewrite it as follow:
> public final class Permissions extends PermissionCollection implements Serializable {
>    private static final Object monitor = new Object();
> ...
> private void readObject(java.io.ObjectInputStream in) throws IOException,
>         ClassNotFoundException {
> ...
>         klasses = new HashMap();
>         synchronized (monitor ) {
>             for (Iterator iter = perms.entrySet().iterator(); iter.hasNext();) {
>                 Map.Entry entry = (Map.Entry)  iter.next();
>                 Class key = (Class) entry.getKey();
>                 PermissionCollection pc = (PermissionCollection) entry.getValue();
>                 if (key != pc.elements().nextElement().getClass()) {
>                     throw new InvalidObjectException(Messages.getString("security.22")); //$NON-NLS-1$
>                 }
>                 klasses.put(key, pc);
>             }
>         }
>     ...
>   }
> ...
> }

-- 
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.