You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@harmony.apache.org by Regis <xu...@gmail.com> on 2010/09/29 07:08:24 UTC
Re: [jira] Created: (HARMONY-6661) Synchonrize on mutable field in
Permissions.java
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.
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.
>