java static list causes memory leak

Question

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;
        }
    
    }

Lacrimatory

Why are you writing List instead of List<Integer>? The List addAll method only adds to the existing list. It will only be a LinkedList if you started with one. Can you explain what these permissions are for and why you're keeping them in a static variable? – Gaudette

The lists will definitely not have a reference to the map that holds them. But the map has references to the lists which it holds. You don't show how any of the data gets into the map in the first place, nor do you tell us anything about what the expected lifespan is of this data or what, if anything, would ever clear it out. Assuming you just add more and more to it, of course it keeps growing. – Gaudette

Answer 1

I'm going to change the code to below, so that it won't create local object: permissionList inside hasPermission(). Wondering if it would fix the issue.

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();
            
        for (Integer a : accessLevels) {
            
            if (permissions.get(a).contains(action)) {
                return true;
            }
        }
        
        return false;
    }

}

Lacrimatory

preferably you'd only post this as an answer once you've confirmed that it solves the problem – Gaygaya

This code snippet worked as it no longer invokes addAll() which previously added items to static list over and over again. – Lacrimatory

permissions.get(a).contains(action) will throw a NullPointerException when the key is not in the map. You might use permissions.getOrDefault(a, List.of()).contains(action) instead. Or Collections.emptyList() instead of List.of() if you’re using an older Java version. – Kit

Answer 2

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.

Parkins