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

[Regis]

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.