United StatesChange Country, Oracle Worldwide Web Sites Communities I am a... I want to...
JDK-6218682 : SubjectDomainCombiner pdCache (WeakHashMap) values strongly reference keys

Details
Type:
Bug
Submit Date:
2005-01-18
Status:
Resolved
Updated Date:
2010-12-08
Project Name:
JDK
Resolved Date:
2005-02-18
Component:
security-libs
OS:
solaris_9,generic
Sub-Component:
java.security
CPU:
sparc,generic
Priority:
P3
Resolution:
Fixed
Affected Versions:
1.0,1.1,1.4.2_07,5.0,6
Fixed Versions:

Related Reports
Backport:
Backport:
Backport:
Backport:
Duplicate:
Duplicate:

Sub Tasks

Description
SubjectDomainCombiner creates an instance of WeakHashMap.  Key objects in this map are the "current" ProtectionDomain instances received via the combine method.  Each "current" PD is mapped to a new PD instance that holds both the contents of the "current" PD, as well as the principals from the Subject associated with this combiner.

At the moment, the WeakHashMap values are regular strong references to the newly created PD "principal-based" instances.  However, this is a problem because these values contain strong references to the corresponding key object (the "current" non-principal-based PD).  Specifically, a "principal-based" PD contains strong references to the CodeSource, signer certs, PermissionCollection and ClassLoader objects in the "current PD".  This prevents the key from being garbage-collected, resulting in an ever growing Map over time (assuming the SubjectDomainCombiner instance is shared across different ClassLoader instances).

To correct this problem, the newly created "principal-based" PD values must be stored as WeakReferences.  This will allow the keys to become weakly referencable and therefore let Map entries become garbage collected as ClassLoader instances become weakly referencable.  

Note that if the SubjectDomainCombiner instance is NOT shared across different ClassLoader instances, this problem is less severe.  The code effectively behaves as it were using a regular HashMap as opposed to a WeakHashMap - entries never leave the Map via garbage collection.  The size of the Map is roughly limited by the number of ClassLoader classpath elements.

JMX does share SubjectDomainCombiner instances across ClassLoaders, so this problem eventually results in an OutOfMemoryError (since the Map grows infinitely over time).

###@###.### 2005-1-18 22:49:03 GMT

                                    
                                
SubjectDomainCombiner creates an instance of WeakHashMap.  Key objects in this map are the "current" ProtectionDomain instances received via the combine method.  Each "current" PD is mapped to a new PD instance that holds both the contents of the "current" PD, as well as the principals from the Subject associated with this combiner.

At the moment, the WeakHashMap values are regular strong references to the newly created PD "principal-based" instances.  However, this is a problem because these values contain strong references to the corresponding key object (the "current" non-principal-based PD).  Specifically, a "principal-based" PD contains strong references to the CodeSource, signer certs, PermissionCollection and ClassLoader objects in the "current PD".  This prevents the key from being garbage-collected, resulting in an ever growing Map over time (assuming the SubjectDomainCombiner instance is shared across different ClassLoader instances).

To correct this problem, the newly created "principal-based" PD values must be stored as WeakReferences.  This will allow the keys to become weakly referencable and therefore let Map entries become garbage collected as ClassLoader instances become weakly referencable.  

Note that if the SubjectDomainCombiner instance is NOT shared across different ClassLoader instances, this problem is less severe.  The code effectively behaves as it were using a regular HashMap as opposed to a WeakHashMap - entries never leave the Map via garbage collection.  The size of the Map is roughly limited by the number of ClassLoader classpath elements.

JMX does share SubjectDomainCombiner instances across ClassLoaders, so this problem eventually results in an OutOfMemoryError (since the Map grows infinitely over time).

###@###.### 2005-1-18 22:49:03 GMT

                                    

Comments
EVALUATION

No regression test.  Unable to rely on deterministic garbage collection behavior

###@###.### 2005-1-22 01:29:45 GMT

It is not dependent to the garbage collector behavior, but the class WeakHashMap implementation, look at the Javadoc of WeakHashMap:
Implementation note: The value objects in a WeakHashMap are held by ordinary strong references. Thus care should be taken to ensure that value objects do not strongly refer to their own keys, either directly or indirectly, since that will prevent the keys from being discarded. Note that a value object may refer indirectly to its key via the WeakHashMap itself; that is, a value object may strongly refer to some other key object whose associated value object, in turn, strongly refers to the key of the first value object. One way to deal with this is to wrap values themselves within WeakReferences before inserting, as in: m.put(key, new WeakReference(value)), and then unwrapping upon each get.
###@###.### 2005-1-26 10:12:42 GMT


Clarified with Shanliang that we agree on the problem in the code.  Shanliang also suggested the following test could be used (below).  We agreed the test is not guaranteed to always work since it depends on unspecified GC behavior.  However if run by hand it should work in most cases currently.  It can be used to sanity check the fix.



You can write a test like this:
1) create 10 different keys and values, the classloader of a value keeps 
a reference to the value's key;

2) call 10 times SubjectDomainCombiner.combine() to make those 10 
key-value paires saved to SubjectDomainCombiner's internal map;

3) save 10 values or keys into another WeakHashMap, like
	myMap.put(value, "");

4) do check as
	for (int i=0; i<300; i++) {
		Thread.sleep(100);
		System.gc();
		if (myMap.size() < 10) {
			return true // successed
		}
	}

	return false; // failed

Another way is to specify a small heap like:
	java -Xmx1M Test
we get OutOfmemeoryException in about 5 minutes with out test, you can 
reduce the heap or make bigger objects in question to speed up.
###@###.### 2005-1-28 18:27:40 GMT


the escalation engineer for 1.4.2 asked me to write up the above test.  i believe the following approximates what is described above:

import java.lang.ref.WeakReference;

import java.util.WeakHashMap;

import java.security.cert.Certificate;

import javax.security.auth.Subject;
import javax.security.auth.SubjectDomainCombiner;
import javax.security.auth.x500.X500Principal;

import java.security.Principal;
import java.security.BasicPermission;
import java.security.CodeSource;
import java.security.Permission;
import java.security.Permissions;
import java.security.ProtectionDomain;

public class Test {

    public static void main(String[] args) throws Exception {

        // construct a PD key for the WeakHashMap inside SubjectDomainCombiner
        //
        // add an instance of MyPerm to the PD key's Permissions object.
        // MyPerm has a strong reference back to the PD key.

        Permissions perms = new Permissions();
        MyPerm perm = new MyPerm("foo");
        perms.add(perm);

        // create a strong reference to the PD key

        ProtectionDomain key = new ProtectionDomain
                                (new CodeSource(null, (Certificate[])null),
                                perms,
                                null,
                                null);
        perm.key = key;

        // clear out all our other references to the PD key

        perms = null;
        perm = null;

        // get a strong reference to MapMaker.
        //
        // MapMaker references
        // SubjectDomainCombiner references
        // WeakKeyValueMap references
        // PD key, new PD value
        //      - new PD value has MyPerm that references PD key

        MapMaker mapMaker = new MapMaker();

        // get a WeakHashMap with the PD key stored as a WeakReference.
        // use this map to determine whether the PD key gets GC'd properly
        // inside the SubjectDomainCombiner cache.

        WeakHashMap myMap = mapMaker.makeMap(key);

        boolean status = false;
        for (int i=0; i<300; i++) {
            Thread.sleep(100);

            // clear our sole strong reference to the PD key
            key = null;

            // now when we run GC, the only possible reason PD key
            // would not be reclaimed is because the SubjectDomainCombiner's
            // cache has a new Subject-based PD value that strongly
            // references the PD key.

            System.gc();

            if (myMap.size() < 1) {
                status = true; // success
                break;
            }
        }

        if (status == true) {
            System.out.println("test passed");
        } else {
            throw new SecurityException("test failed");
        }
    }

    private static class MapMaker {

        // strong reference to SubjectDomainCombiner
        private SubjectDomainCombiner sdc;

        public WeakHashMap makeMap(ProtectionDomain key) {

            Subject s = new Subject();
            s.getPrincipals().add(new X500Principal("cn=Test"));
            sdc = new SubjectDomainCombiner(s);
            ProtectionDomain[] pds = new ProtectionDomain[] { key };

            // store PD key in SDC cache, with new Subject-based PD as value.
            // new Subject-based PD contains MyPerm, which refers back to PD key

            sdc.combine(pds, null);

            WeakHashMap returnMe = new WeakHashMap();
            returnMe.put(key, "");
            return returnMe;
        }
    }


    private static class MyPerm extends BasicPermission {

        // strong reference to PD key
        public ProtectionDomain key;

        public MyPerm(String name) {
            super(name);
        }

        public MyPerm(String name, String action) {
            super(name, action);
        }
    }
}

###@###.### 2005-04-08 00:08:48 GMT
                                     
2005-01-19
EVALUATION

No regression test.  Unable to rely on deterministic garbage collection behavior

###@###.### 2005-1-22 01:29:45 GMT

It is not dependent to the garbage collector behavior, but the class WeakHashMap implementation, look at the Javadoc of WeakHashMap:
Implementation note: The value objects in a WeakHashMap are held by ordinary strong references. Thus care should be taken to ensure that value objects do not strongly refer to their own keys, either directly or indirectly, since that will prevent the keys from being discarded. Note that a value object may refer indirectly to its key via the WeakHashMap itself; that is, a value object may strongly refer to some other key object whose associated value object, in turn, strongly refers to the key of the first value object. One way to deal with this is to wrap values themselves within WeakReferences before inserting, as in: m.put(key, new WeakReference(value)), and then unwrapping upon each get.
###@###.### 2005-1-26 10:12:42 GMT


Clarified with Shanliang that we agree on the problem in the code.  Shanliang also suggested the following test could be used (below).  We agreed the test is not guaranteed to always work since it depends on unspecified GC behavior.  However if run by hand it should work in most cases currently.  It can be used to sanity check the fix.



You can write a test like this:
1) create 10 different keys and values, the classloader of a value keeps 
a reference to the value's key;

2) call 10 times SubjectDomainCombiner.combine() to make those 10 
key-value paires saved to SubjectDomainCombiner's internal map;

3) save 10 values or keys into another WeakHashMap, like
	myMap.put(value, "");

4) do check as
	for (int i=0; i<300; i++) {
		Thread.sleep(100);
		System.gc();
		if (myMap.size() < 10) {
			return true // successed
		}
	}

	return false; // failed

Another way is to specify a small heap like:
	java -Xmx1M Test
we get OutOfmemeoryException in about 5 minutes with out test, you can 
reduce the heap or make bigger objects in question to speed up.
###@###.### 2005-1-28 18:27:40 GMT


the escalation engineer for 1.4.2 asked me to write up the above test.  i believe the following approximates what is described above:

import java.lang.ref.WeakReference;

import java.util.WeakHashMap;

import java.security.cert.Certificate;

import javax.security.auth.Subject;
import javax.security.auth.SubjectDomainCombiner;
import javax.security.auth.x500.X500Principal;

import java.security.Principal;
import java.security.BasicPermission;
import java.security.CodeSource;
import java.security.Permission;
import java.security.Permissions;
import java.security.ProtectionDomain;

public class Test {

    public static void main(String[] args) throws Exception {

        // construct a PD key for the WeakHashMap inside SubjectDomainCombiner
        //
        // add an instance of MyPerm to the PD key's Permissions object.
        // MyPerm has a strong reference back to the PD key.

        Permissions perms = new Permissions();
        MyPerm perm = new MyPerm("foo");
        perms.add(perm);

        // create a strong reference to the PD key

        ProtectionDomain key = new ProtectionDomain
                                (new CodeSource(null, (Certificate[])null),
                                perms,
                                null,
                                null);
        perm.key = key;

        // clear out all our other references to the PD key

        perms = null;
        perm = null;

        // get a strong reference to MapMaker.
        //
        // MapMaker references
        // SubjectDomainCombiner references
        // WeakKeyValueMap references
        // PD key, new PD value
        //      - new PD value has MyPerm that references PD key

        MapMaker mapMaker = new MapMaker();

        // get a WeakHashMap with the PD key stored as a WeakReference.
        // use this map to determine whether the PD key gets GC'd properly
        // inside the SubjectDomainCombiner cache.

        WeakHashMap myMap = mapMaker.makeMap(key);

        boolean status = false;
        for (int i=0; i<300; i++) {
            Thread.sleep(100);

            // clear our sole strong reference to the PD key
            key = null;

            // now when we run GC, the only possible reason PD key
            // would not be reclaimed is because the SubjectDomainCombiner's
            // cache has a new Subject-based PD value that strongly
            // references the PD key.

            System.gc();

            if (myMap.size() < 1) {
                status = true; // success
                break;
            }
        }

        if (status == true) {
            System.out.println("test passed");
        } else {
            throw new SecurityException("test failed");
        }
    }

    private static class MapMaker {

        // strong reference to SubjectDomainCombiner
        private SubjectDomainCombiner sdc;

        public WeakHashMap makeMap(ProtectionDomain key) {

            Subject s = new Subject();
            s.getPrincipals().add(new X500Principal("cn=Test"));
            sdc = new SubjectDomainCombiner(s);
            ProtectionDomain[] pds = new ProtectionDomain[] { key };

            // store PD key in SDC cache, with new Subject-based PD as value.
            // new Subject-based PD contains MyPerm, which refers back to PD key

            sdc.combine(pds, null);

            WeakHashMap returnMe = new WeakHashMap();
            returnMe.put(key, "");
            return returnMe;
        }
    }


    private static class MyPerm extends BasicPermission {

        // strong reference to PD key
        public ProtectionDomain key;

        public MyPerm(String name) {
            super(name);
        }

        public MyPerm(String name, String action) {
            super(name, action);
        }
    }
}

###@###.### 2005-04-08 00:08:48 GMT
                                     
2005-01-19



Hardware and Software, Engineered to Work Together