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.