I have following code which seems to be causing some memory leaks. This code snippet(i.e. hasPermissions()) get executed every time when a user performs an action.
So from what I understood, since Map permissions is a static object, all permissionsList objects that are created inside hasPermission() method have references to global static object(i.e. permissions); therefore, is not eligible to be garbage collected?
Following is the Leak Suspect showing for its heap dump in Eclipse Memory Analyzer tool. When I navigate to details, it shows the class with following code snippet. I found Java List.addAll() function internally creates LinkedList. I'm still trying to comprehend what's exactly happening here. Appreciate your thoughts.
public class AccessManager {
private static Map <Integer, List> permissions;
public static void init()
{
//initiate permissions and add values
}
public static boolean hasPermissions(List<Integer> accessLevels, String action)
{
if (permissions == null)
init();
List permissionsList = null;
for (Integer a : accessLevels) {
if (permissionsList == null) {
permissionsList = permissions.get(a);
} else {
permissionsList.addAll(permissions.get(a));
}
}
if(permissionsList.contains(action)){
return true;
}
return false;
}
}
That code is quite severely broken. In adding new permissions to a List
referenced by permissions
, the hasPermission
method not only creates a memory leak, but may cause subsequent access checks to be incorrectly granted, which might be a quite serious security vulnerability.
For instance, if permissions
contains
Key | Value |
---|---|
1 | [a] |
2 | [b] |
and you execute hasPermissions(Arrays.asList(1,2), "b")
, permissions
will be updated to contain
Key | Value |
---|---|
1 | [a, b] |
2 | [b] |
causing a subsequent call to hasPermissions(Arrays.asList(1), "b")
to return true.
Also each further call to hasPermissions(Arrays.asList(1,2), "b")
causes b to be added again:
Key | Value |
---|---|
1 | [a, b, b] |
2 | [b] |
causing that list to get longer and longer until memory is exhausted. This extremely long list full of duplicates will slow down access checks such as hasPermissions(Arrays.asList(1), "c")
to a crawl.
Oh, and by the way, the lazy initialization that calls init
on first invocation of hasPermissions
is not thread safe, but potentially accessed by multiple threads. You should either declare permissions volatile
(or even synchronize init
if init
is not safe for use by multiple threads), or invoke init
in a static initializer.